Lower bumpy_road_function detection threshold to 2.5#192
Open
Lower bumpy_road_function detection threshold to 2.5#192
Conversation
…r detecting bumps - Updated the default threshold in Settings, dylint.toml, and related tests to 2.5. - Adjusted documentation to reflect lowered threshold, improving detection of legitimate complexity bumps in match expressions with nested conditionals. - Added new UI test fixtures to validate lint behavior with nested conditionals and single cluster scenarios. This change enhances sensitivity for detecting bumpy road patterns while preserving configurability for users. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideLowers the default bumpy_road_function moving-average threshold from 3.0 to 2.5, aligns tests, UI fixtures, and docs with the new behavior, adds nested-match UI coverage, and updates nextest’s UI test filter to catch additional ui test binaries. Flow diagram for bumpy_road_function analysis with 2.5 threshold and new UI fixturesflowchart TD
A[Start lint: analyze function body] --> B[Compute line_complexity for each line]
B --> C[Apply moving average with window = 3]
C --> D[Threshold smoothed signal at T = 2.5]
D --> E[Identify contiguous bumps where value >= 2.5]
E --> F[Filter bumps shorter than min_bump_lines = 2]
F --> G{Number of remaining bumps}
G -->|< 2 bumps| H[No warning: function passes]
G -->|>= 2 bumps| I[Emit bumpy_road_function warning]
subgraph UI_fixtures
J[fail_match_with_nested_if.rs<br>match with two arms, each with nested conditionals<br>produces two separated bumps]
K[pass_single_match_cluster.rs<br>match with one complex arm<br>produces a single contiguous bump]
end
J --> B
K --> B
I --> Z[End]
H --> Z
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
OverviewThis PR lowers the ChangesCore threshold update
Test updates
New UI test fixtures
Documentation updates
Test infrastructure
OutcomeCloses WalkthroughReduces the default threshold for the bumpy_road_function lint from 3.0 to 2.5, alongside new UI test fixtures demonstrating nested conditional detection. Updates test expectations, test infrastructure, configuration files, and documentation to reflect the lower sensitivity threshold. Changes
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The Mode enum visibility was changed from private to public to allow use outside the module. Additionally, a new stderr file was added capturing warnings about nested conditional logic complexity in the key_from_file function, suggesting refactoring to reduce complexity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Complex Conditionalcrates/bumpy_road_function/ui/pass_single_match_cluster.rs: categorise What lead to degradation?categorise has 1 complex conditionals with 3 branches, threshold = 2 Why does this problem occur?A complex conditional is an expression inside a branch such as an if-statmeent which consists of multiple, logical operations. Example: if (x.started() && y.running()).Complex conditionals make the code even harder to read, and contribute to the Complex Method code smell. Encapsulate them. How to fix it?Apply the DECOMPOSE CONDITIONAL refactoring so that the complex conditional is encapsulated in a separate function with a good name that captures the business rule. Optionally, for simple expressions, introduce a new variable which holds the result of the complex conditional. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_conditional.js
function messageReceived(message, timeReceived) {
- // Ignore all messages which aren't from known customers:
- if (!message.sender &&
- customers.getId(message.name) == null) {
+ // Refactoring #1: encapsulate the business rule in a
+ // function. A clear name replaces the need for the comment:
+ if (!knownCustomer(message)) {
log('spam received -- ignoring');
return;
}
- // Provide an auto-reply when outside business hours:
- if ((timeReceived.getHours() > 17) ||
- (timeReceived.getHours() < 8)) {
+ // Refactoring #2: encapsulate the business rule.
+ // Again, note how a clear function name replaces the
+ // need for a code comment:
+ if (outsideBusinessHours(timeReceived)) {
return autoReplyTo(message);
}
pingAgentFor(message);
+}
+
+function outsideBusinessHours(timeReceived) {
+ // Refactoring #3: replace magic numbers with
+ // symbols that communicate with the code reader:
+ const closingHour = 17;
+ const openingHour = 8;
+
+ const hours = timeReceived.getHours();
+
+ // Refactoring #4: simple conditional rules can
+ // be further clarified by introducing a variable:
+ const afterClosing = hours > closingHour;
+ const beforeOpening = hours < openingHour;
+
+ // Yeah -- look how clear the business rule is now!
+ return afterClosing || beforeOpening;
} |
This comment was marked as resolved.
This comment was marked as resolved.
…function Changed the key file path from an incorrect '/tmp/key' to 'key' to align with expected file location and fix file reading errors. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extracted conditional logic for small eligible numbers into the helper function `is_small_eligible` to improve clarity and maintainability in the single cluster match arm. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Update the nextest filter expression to catch crates where the integration test binary is named `ui` with a top-level test function `ui()`. This change ensures such tests are correctly matched and run, addressing cases not previously caught by the substring match `test(ui::ui)`. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The default threshold value (2.5) is now duplicated across
Settings::default, the feature test expectation, the UIdylint.toml, and docs snippets—consider centralizing it in a single constant or helper (and referencing it from tests where practical) to reduce the chance of drift on future threshold changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default threshold value (2.5) is now duplicated across `Settings::default`, the feature test expectation, the UI `dylint.toml`, and docs snippets—consider centralizing it in a single constant or helper (and referencing it from tests where practical) to reduce the chance of drift on future threshold changes.
## Individual Comments
### Comment 1
<location path=".config/nextest.toml" line_range="34" />
<code_context>
+# integration binary is `tests/ui.rs` with a top-level `fn ui()` — the test
+# name reported to nextest is plain `ui`, not `ui::ui`, so the substring
+# match `test(ui::ui)` does not capture them (e.g. bumpy_road_function).
+filter = "test(driver::ui::) | test(tests::ui::) | test(ui::ui) | (binary(ui) & test(=ui))"
test-group = "serial-dylint-ui"
retries = { backoff = "exponential", count = 2, delay = "5s" }
</code_context>
<issue_to_address>
**issue (review_instructions):** Add or document an automated test that demonstrates the nextest filter bug and verifies this new `(binary(ui) & test(=ui))` clause fixes it.
Treat this filter change as a bug fix: previously, certain `ui` tests (e.g. `tests/ui.rs` with top-level `fn ui()`) were not executed, and this clause is intended to correct that. Introduce an automated test (or test harness invocation) that fails without this filter change and passes with it, so that regressions in the filter expression are caught in CI.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Bug fixes *must* be demonstrated by a test.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # integration binary is `tests/ui.rs` with a top-level `fn ui()` — the test | ||
| # name reported to nextest is plain `ui`, not `ui::ui`, so the substring | ||
| # match `test(ui::ui)` does not capture them (e.g. bumpy_road_function). | ||
| filter = "test(driver::ui::) | test(tests::ui::) | test(ui::ui) | (binary(ui) & test(=ui))" |
There was a problem hiding this comment.
issue (review_instructions): Add or document an automated test that demonstrates the nextest filter bug and verifies this new (binary(ui) & test(=ui)) clause fixes it.
Treat this filter change as a bug fix: previously, certain ui tests (e.g. tests/ui.rs with top-level fn ui()) were not executed, and this clause is intended to correct that. Introduce an automated test (or test harness invocation) that fails without this filter change and passes with it, so that regressions in the filter expression are caught in CI.
Review instructions:
Path patterns: **/*
Instructions:
Bug fixes must be demonstrated by a test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/bumpy_road_function/ui/fail_match_with_nested_if.rs`:
- Around line 9-13: The public enum Mode (variants Debug and Release) lacks
rustdoc comments; add concise Rustdoc comments above the Mode enum and each
variant using /// to describe the purpose of the enum and what each variant
represents (e.g., Debug: run with debug assertions and extra checks; Release:
optimized production build behavior) so the public API is documented for cargo
doc generation and consumers can understand intended semantics of Mode, Debug,
and Release.
- Line 5: Remove the crate-level #[expect(dead_code, ...)] attribute and instead
add #[expect(dead_code, reason = "UI test fixture; function analysed but not
invoked")] (or #[allow(dead_code)] if preferred) directly to each unused fixture
item in this file (the specific unused functions/structs/enums that implement
the UI test cases), e.g. annotate each test fixture function/struct rather than
the whole crate so dead-code is scoped to those symbols only.
In `@crates/bumpy_road_function/ui/pass_single_match_cluster.rs`:
- Around line 15-17: Update the rustdoc example for the function categorise:
change the expected assertion from "medium" to "other" so the example matches
actual behaviour (replace the line assert_eq!(categorise(42), "medium"); with
assert_eq!(categorise(42), "other"); in the doc test for categorise).
- Line 5: Remove the crate-level suppression by deleting the top-level attribute
'#![expect(dead_code, reason = "UI test fixture; functions are analysed but not
invoked")]' and instead apply item-level attributes to only the unused test
fixture items in this file (e.g., prepend '#[expect(dead_code, reason = "UI test
fixture; function not invoked")]' to each unused function, struct, or static
used solely for the UI test fixture). Ensure you only annotate the specific
fixture symbols (the unused functions/structs/statics declared in
pass_single_match_cluster.rs) so other unintended dead_code warnings remain
active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94142d16-06aa-4759-9a4f-636959a5ec95
📒 Files selected for processing (10)
.config/nextest.tomlcrates/bumpy_road_function/src/analysis.rscrates/bumpy_road_function/tests/features/bumpy_road.featurecrates/bumpy_road_function/ui/dylint.tomlcrates/bumpy_road_function/ui/fail_match_with_nested_if.rscrates/bumpy_road_function/ui/fail_match_with_nested_if.stderrcrates/bumpy_road_function/ui/fail_two_clusters_legacy.stderrcrates/bumpy_road_function/ui/pass_single_match_cluster.rsdocs/adr-002-dylint-expect-attribute-macro.mddocs/whitaker-dylint-suite-design.md
| //! | ||
| //! This variant uses a match expression with two arms, each containing a nested | ||
| //! conditional block. The two conditional clusters form separated bumps. | ||
| #![expect(dead_code, reason = "UI test fixture; functions are analysed but not invoked")] |
There was a problem hiding this comment.
Replace blanket dead-code suppression with tightly scoped expectations.
Move #![expect(dead_code, ...)] from crate scope to the specific unused fixture items. Avoid hiding unrelated dead-code mistakes.
Patch
-#![expect(dead_code, reason = "UI test fixture; functions are analysed but not invoked")]
@@
+#[expect(dead_code, reason = "UI test fixture API analysed by lint tests")]
pub fn key_from_file(mode: Mode, allow_fallback: bool) -> Result<Vec<u8>, String> {
@@
+#[expect(dead_code, reason = "UI test fixture marker")]
fn dead_code_fixture_marker() {}As per coding guidelines: “Lints must not be silenced except as a last resort … No lint groups, no blanket or file-wide suppression.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/bumpy_road_function/ui/fail_match_with_nested_if.rs` at line 5, Remove
the crate-level #[expect(dead_code, ...)] attribute and instead add
#[expect(dead_code, reason = "UI test fixture; function analysed but not
invoked")] (or #[allow(dead_code)] if preferred) directly to each unused fixture
item in this file (the specific unused functions/structs/enums that implement
the UI test cases), e.g. annotate each test fixture function/struct rather than
the whole crate so dead-code is scoped to those symbols only.
Comment on lines
+9
to
+13
| #[derive(Clone, Copy, PartialEq, Eq)] | ||
| pub enum Mode { | ||
| Debug, | ||
| Release, | ||
| } |
There was a problem hiding this comment.
Document the public Mode API.
Add rustdoc for the enum and its variants. Keep public API docs complete for generated documentation.
Patch
#[derive(Clone, Copy, PartialEq, Eq)]
+/// Selects validation strictness for key loading.
pub enum Mode {
+ /// Allow debug-friendly fallback behaviour.
Debug,
+ /// Enforce strict release validation behaviour.
Release,
}As per coding guidelines: “Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/bumpy_road_function/ui/fail_match_with_nested_if.rs` around lines 9 -
13, The public enum Mode (variants Debug and Release) lacks rustdoc comments;
add concise Rustdoc comments above the Mode enum and each variant using /// to
describe the purpose of the enum and what each variant represents (e.g., Debug:
run with debug assertions and extra checks; Release: optimized production build
behavior) so the public API is documented for cargo doc generation and consumers
can understand intended semantics of Mode, Debug, and Release.
| //! | ||
| //! This example uses a match expression where all conditional complexity is | ||
| //! concentrated in a single arm, forming only one contiguous cluster. | ||
| #![expect(dead_code, reason = "UI test fixture; functions are analysed but not invoked")] |
There was a problem hiding this comment.
Scope the dead_code expectation to specific items.
Replace the crate-level suppression with item-level #[expect(dead_code, reason = "...")] on only the unused fixture items. Prevent unrelated dead-code regressions from being masked in this file.
Patch
-#![expect(dead_code, reason = "UI test fixture; functions are analysed but not invoked")]
@@
+#[expect(dead_code, reason = "UI test fixture entry point for lint analysis")]
pub fn categorise(input: i32) -> &'static str {
@@
+#[expect(dead_code, reason = "UI test fixture marker")]
fn dead_code_fixture_marker() {}As per coding guidelines: “Lints must not be silenced except as a last resort … No lint groups, no blanket or file-wide suppression.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/bumpy_road_function/ui/pass_single_match_cluster.rs` at line 5, Remove
the crate-level suppression by deleting the top-level attribute
'#![expect(dead_code, reason = "UI test fixture; functions are analysed but not
invoked")]' and instead apply item-level attributes to only the unused test
fixture items in this file (e.g., prepend '#[expect(dead_code, reason = "UI test
fixture; function not invoked")]' to each unused function, struct, or static
used solely for the UI test fixture). Ensure you only annotate the specific
fixture symbols (the unused functions/structs/statics declared in
pass_single_match_cluster.rs) so other unintended dead_code warnings remain
active.
Comment on lines
+15
to
+17
| /// ```ignore | ||
| /// assert_eq!(categorise(42), "medium"); | ||
| /// ``` |
There was a problem hiding this comment.
Fix the rustdoc example result.
Update the example expectation to match actual behaviour: categorise(42) returns "other", not "medium".
Patch
/// ```ignore
-/// assert_eq!(categorise(42), "medium");
+/// assert_eq!(categorise(42), "other");
/// ```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// ```ignore | |
| /// assert_eq!(categorise(42), "medium"); | |
| /// ``` | |
| /// |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/bumpy_road_function/ui/pass_single_match_cluster.rs` around lines 15 -
17, Update the rustdoc example for the function categorise: change the expected
assertion from "medium" to "other" so the example matches actual behaviour
(replace the line assert_eq!(categorise(42), "medium"); with
assert_eq!(categorise(42), "other"); in the doc test for categorise).
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
1 participant
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.
Summary
Changes
Core
Tests
UI Fixtures
Documentation
Notes
Test plan
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/b91af476-364b-4b17-bd66-1c1b24c5ae89
📝 Closes #133
Summary by Sourcery
Lower the default bumpy_road_function detection threshold and expand coverage for nested match scenarios while updating configuration, tests, and documentation accordingly.
Enhancements:
CI:
uiwith a top-levelfn ui().Documentation:
Tests: