WIP: fix: use TryFrom for i32->u8 exit code conversion#1544
Draft
EffortlessSteven wants to merge 9 commits intomainfrom
Draft
WIP: fix: use TryFrom for i32->u8 exit code conversion#1544EffortlessSteven wants to merge 9 commits intomainfrom
EffortlessSteven wants to merge 9 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-c775afd5 - ADR: Use TryFrom for i32->u8 conversion, remove redundant clamp - Specs: Single-line fix in main.rs, 5 acceptance criteria - Fixes: main.rs:659 lossy cast warning (issue #259)
|
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
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 28 seconds. ⌛ 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 (49)
📒 Files selected for processing (34)
✨ 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 |
Work item: work-c775afd5 Replace the lossy 'as u8' cast with TryFrom to fix the clippy::cast_possible_truncation warning and remove the redundant clamp(0, 255) dead code. Before: code.clamp(0, 255) as u8 After: u8::try_from(code).unwrap_or(1) Fixes: main.rs:646 lossy cast (issue #259)
EffortlessSteven
changed the title
![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(); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf6dd7943
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+168
to
+171
| ignore_comments: other | ||
| .defaults | ||
| .ignore_comments | ||
| .or(base.defaults.ignore_comments), |
There was a problem hiding this comment.
This commit adds [defaults].ignore_comments/ignore_strings support in config merging, but the values are never propagated into RuleConfig before evaluation. compile_rules still reads only per-rule booleans (crates/diffguard-domain/src/rules.rs:152-153), which default to false when omitted, so setting these new defaults has no effect at runtime. In configs that rely on defaults (instead of repeating flags on every rule), comment/string masking will silently not happen even though the schema/example now documents these keys.
Useful? React with 👍 / 👎.
Work item: work-c775afd5 Add comprehensive docstring to main() explaining: - The exit code semantics (0=pass, 1=error, 2=fail, 3=warn) - Why TryFrom is used instead of as u8 cast - Why unwrap_or(1) is the defensive fallback Also add inline comment explaining the TryFrom idiom rationale.
Member
Author
|
Added a comprehensive docstring to the The docstring explains:
An inline comment also notes the rationale for the All 117 tests pass after the documentation changes. Clippy reports no warnings. Coverage assessment: All public items in scope are now documented. No further documentation gaps were identified in this review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #259
Summary
Replace the lossy
as u8cast with idiomaticTryFrominmain.rs:646. This eliminates theclippy::cast_possible_truncationwarning and removes the redundantclamp(0, 255)dead code.Before:
After:
ADR
.hermes/conveyor/work-c775afd5/adr.mdSpecs
.hermes/conveyor/work-c775afd5/specs.mdWhat Changed
crates/diffguard/src/main.rs:646— replaced lossy cast withTryFromunwrap_or(1)provides defensive fallback for impossible out-of-range valuesTest Results
cargo check -p diffguard— compiles without errorscargo run -p diffguard -- check --help— displays help correctlyFriction Encountered
Notes
duration_overflow_work_3010cb68tests are unrelated to this change