style: ban inline imports and hoist all use statements to module level#299
Conversation
Add a convention to REVIEW.md banning inline (function-body) imports. Move all existing inline imports to the top of their file or module: - src/api/client.rs: hoist std::num::NonZeroU64 from three function bodies - src/lib.rs: hoist crate::api::types::BugReviewState from three test fns - src/config/storage.rs: hoist std::thread from a test fn Co-Authored-By: Sachin Iyer <siyer@detail.dev>
Original prompt from Sachin
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
🚩 Pre-existing non-top-of-file imports in src/api/client.rs
Lines 52-58 of src/api/client.rs contain use statements placed after the api_error function definition (lines 22-50), rather than grouped with the other imports at lines 1-9. The new REVIEW.md rule states 'Place all use imports at the top of the file or module.' While the PR correctly moves NonZeroU64 to line 2, these other imports remain in their mid-file position. This is a pre-existing pattern that predates this PR and is outside the diff hunks, but it's arguably inconsistent with the rule being introduced here.
(Refers to lines 52-58)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — moved these super:: imports up to the top of the file alongside the other imports. Fixed in the latest commit.
Co-Authored-By: Sachin Iyer <siyer@detail.dev>
## Summary Patch release v0.2.4. Bumps version in `Cargo.toml` from 0.2.3 → 0.2.4. Changes since v0.2.3: - fix: correct multi-status pagination in `bugs list` (#301) - style: ban inline imports and hoist all use statements to module level (#299) ## Review & Testing Checklist for Human - [ ] Verify `Cargo.toml` and `Cargo.lock` version is `0.2.4` - [ ] Confirm CI passes (fmt, clippy, tests, vendored artifact checks) ### Notes Q/A testing was performed before cutting this release — all features verified working: - `detail version`, `detail auth status`, `detail repos list` - `detail bugs list` with multi-status, time filters, author filters, scan-id, JSON output - `detail bugs show` (table and JSON formats) - `detail bugs close` / `detail bugs reopen` lifecycle - `detail scans list` with status/time filters - `detail rules list`, `detail rules requests list` - `detail completions bash` - Error handling (invalid bug IDs)  Link to Devin session: https://app.devin.ai/sessions/b5a6272c900f4fc58425f4c555bec1e6 Requested by: @sachiniyer <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/usedetail/cli/pull/302" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Bump `detail-cli` to v0.2.4 for a patch release. Includes a fix for multi-status pagination in `bugs list` and a style cleanup that hoists inline imports to module scope. <sup>Written for commit 76dbbb9. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/302?utm_source=github">Review in cubic</a></sup> <!-- End of auto-generated description by cubic. --> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>
Summary
Add a convention to REVIEW.md banning inline (function-body)
useimports, and clean up all existing violations:useimports must be at the top of the file or module, never inside function bodies. For test modules, imports go at the top ofmod tests.use std::num::NonZeroU64from three function bodies (list_bugs,list_scans,list_repos) to file-level imports.use crate::api::types::BugReviewStatefrom three test functions to themod testsblock.use std::threadfrom a test function to themod testsblock.Review & Testing Checklist for Human
usein non-test code)Notes
No clippy lint exists for banning inline imports, so this is enforced as a review convention. All existing tests (289) pass,
cargo fmt --checkandcargo clippy -- -D warningsare clean.Link to Devin session: https://app.devin.ai/sessions/f68c984b82e64cb1a373f29d68cfc52e
Requested by: @sachiniyer
Summary by cubic
Banned inline
useimports and hoisted all imports to the top of their file or module to standardize style and avoid hidden deps. Documented the rule inREVIEW.mdand fixed all violations.src/api/client.rs: movedstd::num::NonZeroU64imports out of three methods.src/api/client.rs: hoisted mid-fileuse super::generated::types::CreateRuleBodyand thesuper::typesimport block to the top.src/lib.rstests: movedcrate::api::types::BugReviewStatetomod tests.src/config/storage.rstests: movedstd::threadtomod tests.Written for commit d68246a. Summary will update on new commits. Review in cubic