feat(fmt,cli-common): add fgumi-fmt and fgumi-cli-common leaf crates - #672
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR centralizes workspace dependencies, adds ChangesShared CLI foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLIArgument
participant parse_memory
participant resolve_memory_budget
CLIArgument->>parse_memory: memory configuration string
parse_memory->>parse_memory: parse_memory_size
parse_memory-->>CLIArgument: MemoryLimit or parse error
CLIArgument->>resolve_memory_budget: limit, reserve, threads, per_thread
resolve_memory_budget-->>CLIArgument: concrete byte budget or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-runall #672 +/- ##
===============================================
- Coverage 93.94% 93.94% -0.01%
===============================================
Files 178 180 +2
Lines 108058 108459 +401
===============================================
+ Hits 101518 101893 +375
- Misses 6540 6566 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ec5954d to
edb01bc
Compare
edb01bc to
35a1c82
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-cli-common/src/lib.rs`:
- Around line 498-505: Update the debug logging around the auto-memory budget
calculation to conditionally describe allocations based on the per_thread mode.
When per_thread is true, retain the per-thread amount and thread-count framing;
when false, log the shared budget without implying it is split into independent
per-thread allocations.
- Around line 24-38: Keep the `Command` trait and its `enum_dispatch`-generated
command enum in the same crate so dispatch implementations are generated
correctly. Update the current `Command` arrangement in `fgumi-cli-common` and
the corresponding enum location, or replace this path with a cross-crate
dispatch mechanism while preserving each command’s `execute` behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d719e12a-e585-4d03-92c4-62ef31345bff
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/fgumi-cli-common/Cargo.tomlcrates/fgumi-cli-common/src/lib.rscrates/fgumi-fmt/Cargo.tomlcrates/fgumi-fmt/src/lib.rssrc/lib/commands/common.rssrc/lib/system.rssrc/lib/validation.rs
35a1c82 to
3c5e92a
Compare
|
Both findings fixed in the amended commit.
Removed it from Left the umbrella's own Per-thread debug log — fixed as suggested, and applied to the sibling too: Verification: |
3c5e92a to
a3660af
Compare
First of a crate-by-crate series landing the runall / unified-pipeline work. Adds the two dependency-free leaf crates, wired into the workspace but not yet called by any command. fgumi-fmt hosts the pure formatting helpers -- format_count, format_duration and format_rate -- with no dependencies at all, so any layer can reach them without inheriting clap or sysinfo. fgumi-cli-common holds the shared CLI types and re-exports those three; it depends only on fgumi-fmt. Both crates arrived with per-crate version pins; those are converted to [workspace.dependencies] entries to match the workspace's existing centralization. That required promoting enum_dispatch, num_cpus and sysinfo, each now shared by two members. sysinfo matters most: it is held at 0.38 because >= 0.39 requires rustc 1.95, above the pinned 1.93 MSRV, so a per-crate pin could drift and silently raise the effective MSRV. Extends fgumi-cli-common's tests to cover the paths the crate arrived without: the OperationTimer, validate_file_exists, the parse_memory_size error arms, parse_memory/parse_memory_reserve, the binary-unit Display impls, and the non-per-thread and over-commit branches of the memory-budget resolver. Also fixes three defects in the umbrella crate that the incoming copies had already corrected, so the live code and the future replacement agree: - system.rs: detect_total_memory saturated at usize::MAX, which made the downstream `budget > total` guard in resolve_memory_budget unreachable on 32-bit targets. Now saturates at usize::MAX / 2. - validation.rs: the too-large plain-number error suggested `mb_value / 1000` labelled GB. The input is MiB, so the figure was wrong -- 2,000,000 MiB was reported as "2000GB" when the true value is 2097 GB, or 1953 GiB. Now divides by 1024 and labels it GiB. - commands/common.rs: CompressionOptions derived Default, yielding compression_level 0 (uncompressed BGZF) while the clap default is 1, so programmatic callers silently wrote uncompressed BAM. Now hand-written to match. Reachable today only from tests and doc examples. Each of the three is pinned by a test that fails if the fix is reverted.
a3660af to
8a4d064
Compare
…672) First of a crate-by-crate series landing the runall / unified-pipeline work. Adds the two dependency-free leaf crates, wired into the workspace but not yet called by any command. fgumi-fmt hosts the pure formatting helpers -- format_count, format_duration and format_rate -- with no dependencies at all, so any layer can reach them without inheriting clap or sysinfo. fgumi-cli-common holds the shared CLI types and re-exports those three; it depends only on fgumi-fmt. Both crates arrived with per-crate version pins; those are converted to [workspace.dependencies] entries to match the workspace's existing centralization. That required promoting enum_dispatch, num_cpus and sysinfo, each now shared by two members. sysinfo matters most: it is held at 0.38 because >= 0.39 requires rustc 1.95, above the pinned 1.93 MSRV, so a per-crate pin could drift and silently raise the effective MSRV. Extends fgumi-cli-common's tests to cover the paths the crate arrived without: the OperationTimer, validate_file_exists, the parse_memory_size error arms, parse_memory/parse_memory_reserve, the binary-unit Display impls, and the non-per-thread and over-commit branches of the memory-budget resolver. Also fixes three defects in the umbrella crate that the incoming copies had already corrected, so the live code and the future replacement agree: - system.rs: detect_total_memory saturated at usize::MAX, which made the downstream `budget > total` guard in resolve_memory_budget unreachable on 32-bit targets. Now saturates at usize::MAX / 2. - validation.rs: the too-large plain-number error suggested `mb_value / 1000` labelled GB. The input is MiB, so the figure was wrong -- 2,000,000 MiB was reported as "2000GB" when the true value is 2097 GB, or 1953 GiB. Now divides by 1024 and labels it GiB. - commands/common.rs: CompressionOptions derived Default, yielding compression_level 0 (uncompressed BGZF) while the clap default is 1, so programmatic callers silently wrote uncompressed BAM. Now hand-written to match. Reachable today only from tests and doc examples. Each of the three is pinned by a test that fails if the fix is reverted.
…672) First of a crate-by-crate series landing the runall / unified-pipeline work. Adds the two dependency-free leaf crates, wired into the workspace but not yet called by any command. fgumi-fmt hosts the pure formatting helpers -- format_count, format_duration and format_rate -- with no dependencies at all, so any layer can reach them without inheriting clap or sysinfo. fgumi-cli-common holds the shared CLI types and re-exports those three; it depends only on fgumi-fmt. Both crates arrived with per-crate version pins; those are converted to [workspace.dependencies] entries to match the workspace's existing centralization. That required promoting enum_dispatch, num_cpus and sysinfo, each now shared by two members. sysinfo matters most: it is held at 0.38 because >= 0.39 requires rustc 1.95, above the pinned 1.93 MSRV, so a per-crate pin could drift and silently raise the effective MSRV. Extends fgumi-cli-common's tests to cover the paths the crate arrived without: the OperationTimer, validate_file_exists, the parse_memory_size error arms, parse_memory/parse_memory_reserve, the binary-unit Display impls, and the non-per-thread and over-commit branches of the memory-budget resolver. Also fixes three defects in the umbrella crate that the incoming copies had already corrected, so the live code and the future replacement agree: - system.rs: detect_total_memory saturated at usize::MAX, which made the downstream `budget > total` guard in resolve_memory_budget unreachable on 32-bit targets. Now saturates at usize::MAX / 2. - validation.rs: the too-large plain-number error suggested `mb_value / 1000` labelled GB. The input is MiB, so the figure was wrong -- 2,000,000 MiB was reported as "2000GB" when the true value is 2097 GB, or 1953 GiB. Now divides by 1024 and labels it GiB. - commands/common.rs: CompressionOptions derived Default, yielding compression_level 0 (uncompressed BGZF) while the clap default is 1, so programmatic callers silently wrote uncompressed BAM. Now hand-written to match. Reachable today only from tests and doc examples. Each of the three is pinned by a test that fails if the fix is reverted.
…672) First of a crate-by-crate series landing the runall / unified-pipeline work. Adds the two dependency-free leaf crates, wired into the workspace but not yet called by any command. fgumi-fmt hosts the pure formatting helpers -- format_count, format_duration and format_rate -- with no dependencies at all, so any layer can reach them without inheriting clap or sysinfo. fgumi-cli-common holds the shared CLI types and re-exports those three; it depends only on fgumi-fmt. Both crates arrived with per-crate version pins; those are converted to [workspace.dependencies] entries to match the workspace's existing centralization. That required promoting enum_dispatch, num_cpus and sysinfo, each now shared by two members. sysinfo matters most: it is held at 0.38 because >= 0.39 requires rustc 1.95, above the pinned 1.93 MSRV, so a per-crate pin could drift and silently raise the effective MSRV. Extends fgumi-cli-common's tests to cover the paths the crate arrived without: the OperationTimer, validate_file_exists, the parse_memory_size error arms, parse_memory/parse_memory_reserve, the binary-unit Display impls, and the non-per-thread and over-commit branches of the memory-budget resolver. Also fixes three defects in the umbrella crate that the incoming copies had already corrected, so the live code and the future replacement agree: - system.rs: detect_total_memory saturated at usize::MAX, which made the downstream `budget > total` guard in resolve_memory_budget unreachable on 32-bit targets. Now saturates at usize::MAX / 2. - validation.rs: the too-large plain-number error suggested `mb_value / 1000` labelled GB. The input is MiB, so the figure was wrong -- 2,000,000 MiB was reported as "2000GB" when the true value is 2097 GB, or 1953 GiB. Now divides by 1024 and labels it GiB. - commands/common.rs: CompressionOptions derived Default, yielding compression_level 0 (uncompressed BGZF) while the clap default is 1, so programmatic callers silently wrote uncompressed BAM. Now hand-written to match. Reachable today only from tests and doc examples. Each of the three is pinned by a test that fails if the fix is reverted.
…672) First of a crate-by-crate series landing the runall / unified-pipeline work. Adds the two dependency-free leaf crates, wired into the workspace but not yet called by any command. fgumi-fmt hosts the pure formatting helpers -- format_count, format_duration and format_rate -- with no dependencies at all, so any layer can reach them without inheriting clap or sysinfo. fgumi-cli-common holds the shared CLI types and re-exports those three; it depends only on fgumi-fmt. Both crates arrived with per-crate version pins; those are converted to [workspace.dependencies] entries to match the workspace's existing centralization. That required promoting enum_dispatch, num_cpus and sysinfo, each now shared by two members. sysinfo matters most: it is held at 0.38 because >= 0.39 requires rustc 1.95, above the pinned 1.93 MSRV, so a per-crate pin could drift and silently raise the effective MSRV. Extends fgumi-cli-common's tests to cover the paths the crate arrived without: the OperationTimer, validate_file_exists, the parse_memory_size error arms, parse_memory/parse_memory_reserve, the binary-unit Display impls, and the non-per-thread and over-commit branches of the memory-budget resolver. Also fixes three defects in the umbrella crate that the incoming copies had already corrected, so the live code and the future replacement agree: - system.rs: detect_total_memory saturated at usize::MAX, which made the downstream `budget > total` guard in resolve_memory_budget unreachable on 32-bit targets. Now saturates at usize::MAX / 2. - validation.rs: the too-large plain-number error suggested `mb_value / 1000` labelled GB. The input is MiB, so the figure was wrong -- 2,000,000 MiB was reported as "2000GB" when the true value is 2097 GB, or 1953 GiB. Now divides by 1024 and labels it GiB. - commands/common.rs: CompressionOptions derived Default, yielding compression_level 0 (uncompressed BGZF) while the clap default is 1, so programmatic callers silently wrote uncompressed BAM. Now hand-written to match. Reachable today only from tests and doc examples. Each of the three is pinned by a test that fails if the fix is reverted.
First in a crate-by-crate series landing the
runall/ unified-pipeline work ontomain-runall. Each PR targetsmain-runall, which is kept rebased onmain;mainitself is untouched until the whole series lands.What
Adds the two dependency-free leaf crates, wired into the workspace as members:
fgumi-fmt(102 lines, 2 public items) — human-readable count and duration formatting. No workspace dependencies at all.fgumi-cli-common(817 lines, 21 public items) — shared CLI types and helpers. Depends only onfgumi-fmt.Nothing calls into either crate yet. This is purely additive: no existing file changes behaviour, and both crates are unreachable from any command until later PRs wire them up.
Why these two first
They are the only two of the six incoming crates that are already clean under
cargo ci-lint(-D warnings -W clippy::pedantic). The remaining four need a let-chain lint sweep first — the source branch targets MSRV 1.87, this workspace is on 1.93 with let-chains adopted, soclippy::collapsible_iffires on code written against the older toolchain. Per the repo's guidance those get rewritten as let-chains rather than suppressed, and keeping that work out of this PR keeps both halves reviewable.Starting small is also deliberate: this PR is really about agreeing the conventions — workspace wiring, dependency handling, lockfile discipline — that the rest of the series will repeat.
Dependency centralization
Both crates arrived with per-crate version pins. I converted them to
{ workspace = true }to match this workspace's existing centralization, which required promoting three dependencies into[workspace.dependencies]:enum_dispatchnum_cpussysinfosysinfois the one that matters: it is held at 0.38 because >= 0.39 requires rustc 1.95, above our pinned 1.93. A per-crate pin could drift onto 0.39 and silently raise the effective MSRV — precisely the failure the centralization rule exists to prevent. Its explanatory comment moved to the workspace entry.Verification
cargo ci-fmtcargo ci-lintcargo ci-testcargo ci-doctestcargo ci-docwithRUSTDOCFLAGS="-D warnings"cargo nextest run -p fgumi-fmt -p fgumi-cli-commonCargo.lockis regenerated in the same commit — three CI aliases run--locked, so a stale lockfile would fail on this commit specifically.A test-inventory check (name-keyed, so module moves are not miscounted as loss) confirms no test present on
mainis absent here: 5964 before, 6005 after, 0 lost. Raw#[case],#[values]and#[ignore]counts are also non-decreasing, since those are invisible to a name-keyed comparison.For review
enum_dispatch/num_cpus/sysinfointo[workspace.dependencies]the right call to make here, or would you rather this PR stay minimal and centralization land separately?nh/runall-NN-<topic>so the series is orderable. Say if you want the<issue>/<user>/<type>-<desc>form instead.Update (force-pushed): the first push failed
codecov/patchat 74.9 % against the repo's 90 % gate, with all 13 other checks green.Cause:
fgumi-cli-commonarrived from the source branch at 73.1 % line coverage on the changed lines. Its own tests cover the parsing happy paths, but theOperationTimer,validate_file_exists, severalparse_memory_sizeerror arms,parse_memory/parse_memory_reserve, the binary-unitDisplayimpls, and two branches of the memory-budget resolver arrived untested.fgumi-fmtwas already at 100 %.Rather than pad for the metric, I added tests for each of those paths — as
#[rstest]case tables with descriptive labels, matching the repo convention.fgumi-cli-commongoes from 33 to 63 tests; patch coverage is now 95.9 %.The 15 lines still uncovered are all
log::info!/warn!/debug!macro arguments, which are never evaluated because the test binary initializes no logger, plus one defensively-unreachablechecked_mularm inparse_memory_size(its input is already bounded to ≤ 1,000,000 a few lines earlier). Both seemed like the wrong things to chase.Amended into the original commit rather than stacked as a fixup, so the PR stays a single logical change.
tempfileis added as a dev-dependency for thevalidate_file_existstests.Summary by CodeRabbit
New Features
Bug Fixes