WIP: Remove redundant Language::Json match arm (issue #452)#1556
WIP: Remove redundant Language::Json match arm (issue #452)#1556EffortlessSteven wants to merge 14 commits intomainfrom
Conversation
Work item: work-a140ddf6 Fixes clippy::unnecessary_wraps on cmd_doctor() which returns Result<i32> but never produces Err. The function always returns Ok(0) or Ok(1).
Add detailed docstring explaining what validate_config_rules checks: - Duplicate rule IDs - Empty pattern lists - Invalid regex patterns - Invalid multiline_window values - Unknown rule dependencies - Invalid path globs
Work item: work-3d8d9b32
- Remove unused doc comments inside proptest! blocks (clippy warning) - Fix formatting in property_test_string_syntax_invariant.rs - Add missing insta dependency to diffguard-analytics dev-dependencies These are pre-existing issues on the branch that block CI.
…duration_overflow test
Work item: work-5102a8b6 - Pin 7 GitHub Actions to verified SHA commits - Pin dtolnay/rust-toolchain to specific Rust version 1.85.0 - Scope: ci.yml, publish.yml, sarif-example.yml, diffguard.yml
…ntation Work item: work-5102a8b6
Work item: work-cd9db3ce
Fix copy-paste bug where detect_language() returns Some("xml")
instead of Some("html") for .html and .htm extensions.
- Split the combined match arm at line 223 so HTML extensions (html, htm)
return Some("html") instead of Some("xml")
- Updated unit tests in rules.rs to expect Some("html") for HTML extensions
- Updated property tests in properties.rs to expect "html" for HTML extensions
- Fixed CLAUDE.md API signature documentation
This is a copy-paste bug fix. The two-representation design maintains:
- detect_language() returns "html" for rule filtering (languages = ["html"])
- Language::Xml is used for preprocessing (HTML uses XML-style comments)
Fixes the inability to filter rules by HTML files via languages config.
Per work-5102a8b6: Pin all GitHub Actions to verified SHA commits to improve supply chain security and prevent tag manipulation attacks. Actions pinned: - actions/checkout@v4 -> 34e114876b0b11c390a56381ad16ebd13914f8d5 - dtolnay/rust-toolchain@stable -> 1.85.0 (specific version) - Swatinem/rust-cache@v2 -> 42dc69e1aa15d09112580998cf2ef0119e2e91ae - actions/upload-artifact@v4 -> ea165f8d65b6e75b540449e92b4886f43607fa02 - actions/download-artifact@v4 -> d3f86a106a0bac45b974a628896c90dbdf5c8093 - github/codeql-action/upload-sarif@v3 -> 865f5f5c36632f18690a3d569fa0a764f2da0c3e - softprops/action-gh-release@v2 -> 3bb12739c298aeb8a4eeaf626c5b8d85266b0e65 - actions/github-script@v7 -> f28e40c7f34bde8b3046d885e986cb6290c5673b Review quarterly for updated action versions.
Follow ADR-012 decision for issue #452: - Remove Language::Json from explicit match arm in string_syntax() - Keep Language::Yaml | Language::Toml explicit (not redundant) - JSON falls through to wildcard since it also uses C-style strings - Update comments to clarify JSON is handled by wildcard This aligns with comment_syntax() pattern where YAML/TOML are explicit and JSON falls through to CStyle wildcard. Refs: ADR-012, issue #452
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (27)
📒 Files selected for processing (45)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #[test] | ||
| fn language_from_str_yaml_toml_json_aliases() { | ||
| // YAML has 'yml' alias | ||
| assert_eq!("yml".parse::<Language>().unwrap(), Language::Yaml); |
| // YAML has 'yml' alias | ||
| assert_eq!("yml".parse::<Language>().unwrap(), Language::Yaml); | ||
| // JSON has 'jsonc' and 'json5' aliases (JSON with comments) | ||
| assert_eq!("jsonc".parse::<Language>().unwrap(), Language::Json); |
| assert_eq!("yml".parse::<Language>().unwrap(), Language::Yaml); | ||
| // JSON has 'jsonc' and 'json5' aliases (JSON with comments) | ||
| assert_eq!("jsonc".parse::<Language>().unwrap(), Language::Json); | ||
| assert_eq!("json5".parse::<Language>().unwrap(), Language::Json); |
| "Yaml", | ||
| "Yml", | ||
| ]) { | ||
| let lang: Language = alias.parse().unwrap(); |
| "Jsonc", | ||
| "Json5", | ||
| ]) { | ||
| let lang: Language = alias.parse().unwrap(); |
|
|
||
| #[test] | ||
| fn toml_parsed_returns_cstyle() { | ||
| let lang: Language = "toml".parse().unwrap(); |
Documentation Findings — work-1f927a4dReviewed the Documentation Added
Coverage AssessmentAll public items in The remaining internal complexity — the Verification
|
Green Test Builder Findings — work-1f927a4dRan 10 green tests against the `string_syntax()` implementation in `crates/diffguard-domain/src/preprocess.rs`. All tests pass. Edge Cases Covered
Implementation StatusThe fix correctly removes `Language::Json` from the explicit match arm while keeping `Language::Yaml | Language::Toml` explicit. All JSON variants, YAML variants, and TOML correctly parse and return CStyle strings. No friction was encountered — the implementation was already correct and the red tests were well-designed. Known Gaps
|
Property Test Findings — work-1f927a4d (PROVEN gate)Ran 13 property-based tests against the Core Invariants Verified
Regression Tests AddedFile: Contains 13 property-based tests covering exhaustive match coverage, core invariants, idempotence, group correctness, alias parsing, and explicit arm preservation. ConclusionThe property tests confirm that removing |
Integration Test Findings — work-1f927a4dRan 10 integration tests covering the Integration Tests Written
Component Handoffs Verified
Error PropagationNo error propagation changes — the preprocessor is designed for best-effort processing. Unknown languages fall through to wildcard, malformed input produces best-effort output, and the crate has no I/O dependencies. CLI FlowThe preprocessing change is transparent to CLI users: Results
|
2 participants
Closes #452
Summary
Remove Language::Json from the explicit match arm in string_syntax() per ADR-012. The arm now only handles Language::Yaml | Language::Toml explicitly, and JSON falls through to the wildcard since it also uses C-style strings.
This aligns with the comment_syntax() pattern where YAML/TOML are explicit and JSON falls through to CStyle wildcard.
ADR
Specs
What Changed
Test Results
All diffguard-domain tests pass (7 tests), including yaml_and_toml_have_explicit_arms_not_wildcard
Notes