Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR removes the auto-derived Default implementation for BuildTargets and provides a custom Default impl that returns an empty slice, along with clippy allowances and a justification comment to satisfy non-'static lifetime requirements. Class diagram for manual Default implementation on BuildTargetsclassDiagram
class BuildTargets {
+&[String] data
+as_slice() &[String]
+is_empty() bool
+len() usize
+Default::default() BuildTargets // now manually implemented
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughRemove the derived Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🔍 MCP Research (1 server)Deepwiki:
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Use of
#[allow]is forbidden; use narrowly scoped#[expect(lint, reason = "...")]instead. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/runner.rs:71` </location>
<code_context>
}
}
+#[allow(
+ clippy::derivable_impls,
+ reason = "Derive fails for non-'static lifetimes; manual impl returns empty slice."
</code_context>
<issue_to_address>
Use of `#[allow]` is forbidden; use narrowly scoped `#[expect(lint, reason = "...")]` instead.
The use of `#[allow(clippy::derivable_impls, ...)]` here violates the lint suppression policy. Please replace with a narrowly scoped `#[expect(lint, reason = "...")]` and provide a clear reason. Blanket or grouped allows are not permitted.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runner.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/runner.rs
🔍 MCP Research (1 server)
Deepwiki:
- The repository already supports default build targets via the NetsukeManifest.defaults field (Vec), which is converted into BuildGraph.default_targets by BuildGraph::process_defaults in the IR generation pipeline (process_rules → process_targets → process_defaults). (src/ir.rs)
- The term "manual default" in this codebase maps to listing target names in a Netsukefile's defaults section (example YAML provided in the tool response showing how to add a target to defaults). (tool response)
- Relevant source files called out by the search are src/ast.rs (NetsukeManifest with defaults) and src/ir.rs (BuildGraph with default_targets and process_defaults). (src/ast.rs, src/ir.rs)
🔇 Additional comments (1)
src/runner.rs (1)
71-79: Tighten the lint expectation reason and verify it actually firesTarget src/runner.rs lines 71–79:
Replace the attribute with:
#[expect( clippy::derivable_impls, reason = "Cannot derive Default because &'a [String] has no Default; manual impl uses a static empty slice." )]Run Clippy locally and confirm that
clippy::derivable_implsis emitted on this manualDefaultimpl.If the lint does not trigger, remove the
#[expect]attribute and retain the manual impl unchanged.
1 participant
Summary
Defaultderive onBuildTargetswith manual implementationcloses #85
Testing
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_6896817117bc83229dcf4b9e11b6aa31
Summary by Sourcery
Implement a manual Default implementation for BuildTargets to replace the derive and handle non-'static lifetimes while addressing Clippy warnings.
Enhancements: