Clean-break fork hygiene: CI rework, drop build.rs, remove nightly-std - #10
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR consolidates CI into ChangesCI and tooling restructuring
Unconditional compilation cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the codebase by leveraging the updated minimum supported Rust version (MSRV) of 1.86. It removes the build.rs script and the autocfg build dependency, as conditional compilation for stable features like total_cmp and Saturating is no longer necessary. Additionally, the unstable nightly-std feature and its associated format_into module have been removed, and the pre-commit configuration has been updated to use local cargo hooks. There are no review comments to address, and I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Prune upstream scaffolding and a fragile feature ahead of the first crates.io release of the fork. CI (pre-commit-centric): - .pre-commit-config.yaml is the single source of truth for the enforced gate (fmt, clippy on `std libm ct` and `no_std libm ct` with -D warnings, cargo test), via local cargo hooks rather than the stale pinned doublify/pre-commit-rust. Runs identically on `git commit` and in CI. - Consolidate pr.yaml / main.yaml / ci.yaml into one ci.yaml backstop (pull_request + push:main + merge_group + weekly schedule). Jobs: gate (pre-commit), test (MSRV/stable/beta feature combos), nightly (const surface, runs tests/const_nightly.rs), cross (i586 + thumbv6m incl. ct), success. Fixes the latent i586 `--all-features`-on-stable E0554. - Delete the verbatim-upstream ci/test_full.sh and ci/rustup.sh. - Repoint README build badge main.yaml -> ci.yaml. Drop build.rs + autocfg: - Both probes (has_total_cmp 1.62, has_num_saturating 1.74) are always true at MSRV 1.86. Delete build.rs, the build-dependency, and inline all cfg branches (float.rs total_cmp macro + 12 has_num_saturating sites in lib.rs and identities.rs). Dependents no longer run a build script. Remove nightly-std / format_into: - The feature name gated exactly one trait, and the module could only mirror std by naming NumBuffer/NumBufferTrait, which now require the compiler-internal `fmt_internals` feature. Delete the module, feature, attr, re-exports, and CI step. Plain `nightly` is now a pure const-ness upgrade with no library feature gates.
06c0ebd to
6916c55
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
11-91: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissions:block.None of the jobs (
gate,test,nightly,cross,success) declare apermissionsblock, so each inherits the repository/org defaultGITHUB_TOKENpermissions, which may be broader than needed (write access to contents, etc.). Static analysis (zizmor) flags this asexcessive-permissionsfor all four job ranges (15-30, 31-51, 52-61, 62-79). None of these jobs push, comment, or otherwise need write access — restrict to read-only.🔒 Proposed fix
on: pull_request: push: branches: - main merge_group: schedule: - cron: '0 0 * * 0' # 00:00 Sunday +permissions: + contents: read + jobs:🤖 Prompt for 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. In @.github/workflows/ci.yaml around lines 11 - 91, Add an explicit least-privilege permissions block to each job in the workflow so `gate`, `test`, `nightly`, `cross`, and `success` do not inherit broader default `GITHUB_TOKEN` access. Set the jobs in this workflow to read-only permissions since they only checkout, build, test, and summarize results. Update the job definitions in the CI workflow near the `gate`, `test`, `nightly`, `cross`, and `success` sections to include the restrictive `permissions:` setting.Source: Linters/SAST tools
🧹 Nitpick comments (3)
src/float.rs (1)
2286-2297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
<$T>::total_cmp(self, other)here.
selfalready has type&Self, so the extra borrow is unnecessary; the direct call is clearer.🤖 Prompt for 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. In `@src/float.rs` around lines 2286 - 2297, The TotalOrder implementation in totalorder_impl uses an unnecessary extra borrow when forwarding to the core total_cmp method. Update the TotalOrder::total_cmp implementation for $T to call the inherent <$T>::total_cmp(self, other) directly instead of reborrowing self; keep the change within the totalorder_impl macro so both f32 and f64 pick it up..github/workflows/ci.yaml (1)
2-10: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider adding a
concurrencygroup to cancel superseded runs.With
pull_requestandpushtriggers now combined into one workflow, repeated pushes to a PR branch will queue/run multiple full matrices (gate + test matrix + nightly + cross) concurrently. A concurrency group withcancel-in-progresswould save CI minutes on superseded commits.♻️ Suggested addition
on: pull_request: push: branches: - main merge_group: schedule: - cron: '0 0 * * 0' # 00:00 Sunday +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs:🤖 Prompt for 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. In @.github/workflows/ci.yaml around lines 2 - 10, Add a workflow-level concurrency group to the CI workflow so newer runs cancel superseded ones from the same branch/PR. Update the workflow configuration around the existing on triggers in the CI workflow to define a stable group key and enable cancel-in-progress, keeping repeated push and pull_request executions from running the full matrix concurrently. Use the top-level workflow settings near the current on block as the place to add this behavior..pre-commit-config.yaml (1)
32-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
--testsfor the no_std clippy hook.pre-commit-config.yaml:38-43examples/typestates.rsis std-only, so--all-targetswould pull in a target that can’t build underlibm ct;--testskeeps the no_std integration tests liketests/cast.rslinted without dragging in examples.🤖 Prompt for 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. In @.pre-commit-config.yaml around lines 32 - 43, Update the cargo-clippy-no-std hook so it lints tests only instead of all targets. In the no_std pre-commit entry, replace the target selection in the cargo clippy invocation to use --tests, since examples/typestates.rs is std-only and should not be pulled in by the libm ct configuration. Keep the existing no-default-features and --features "libm ct" settings, and make the change in the cargo-clippy-no-std hook definition.
🤖 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 @.github/workflows/ci.yaml:
- Line 19: The checkout steps are persisting the default GitHub credentials
unnecessarily, so update every actions/checkout@v4 use in the workflow to
disable credential persistence. Apply the same change to each checkout step used
by the gate, test, nightly, and cross jobs, and keep the fix localized to the
actions/checkout invocation by setting persist-credentials to false.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 11-91: Add an explicit least-privilege permissions block to each
job in the workflow so `gate`, `test`, `nightly`, `cross`, and `success` do not
inherit broader default `GITHUB_TOKEN` access. Set the jobs in this workflow to
read-only permissions since they only checkout, build, test, and summarize
results. Update the job definitions in the CI workflow near the `gate`, `test`,
`nightly`, `cross`, and `success` sections to include the restrictive
`permissions:` setting.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 2-10: Add a workflow-level concurrency group to the CI workflow so
newer runs cancel superseded ones from the same branch/PR. Update the workflow
configuration around the existing on triggers in the CI workflow to define a
stable group key and enable cancel-in-progress, keeping repeated push and
pull_request executions from running the full matrix concurrently. Use the
top-level workflow settings near the current on block as the place to add this
behavior.
In @.pre-commit-config.yaml:
- Around line 32-43: Update the cargo-clippy-no-std hook so it lints tests only
instead of all targets. In the no_std pre-commit entry, replace the target
selection in the cargo clippy invocation to use --tests, since
examples/typestates.rs is std-only and should not be pulled in by the libm ct
configuration. Keep the existing no-default-features and --features "libm ct"
settings, and make the change in the cargo-clippy-no-std hook definition.
In `@src/float.rs`:
- Around line 2286-2297: The TotalOrder implementation in totalorder_impl uses
an unnecessary extra borrow when forwarding to the core total_cmp method. Update
the TotalOrder::total_cmp implementation for $T to call the inherent
<$T>::total_cmp(self, other) directly instead of reborrowing self; keep the
change within the totalorder_impl macro so both f32 and f64 pick it up.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 747104fe-fca6-43f7-9de8-8606261e7fa0
📒 Files selected for processing (14)
.github/workflows/ci.yaml.github/workflows/main.yaml.github/workflows/pr.yaml.pre-commit-config.yamlCargo.tomlREADME.mdbuild.rsci/rustup.shci/test_full.shsrc/float.rssrc/identities.rssrc/lib.rssrc/ops/format_into.rssrc/ops/mod.rs
💤 Files with no reviewable changes (9)
- .github/workflows/main.yaml
- .github/workflows/pr.yaml
- build.rs
- src/ops/format_into.rs
- ci/test_full.sh
- ci/rustup.sh
- src/ops/mod.rs
- src/identities.rs
- src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.pre-commit-config.yaml (1)
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
--all-targetsscope between clippy hooks.The std clippy hook (Line 30) lints with
--all-targets, but the no_std variant (Line 36) omits it, so test/example/bench code isn't clippy-checked under thelibm ct(no_std) feature set. If this is intentional (e.g., avoiding std-only test harness conflicts under no_std), consider a brief comment; otherwise align the flag for consistent coverage.♻️ Proposed fix
- id: cargo-clippy-no-std name: cargo clippy (no_std libm ct, -D warnings) - entry: cargo clippy --no-default-features --features "libm ct" -- -D warnings + entry: cargo clippy --all-targets --no-default-features --features "libm ct" -- -D warnings language: system types: [rust] pass_filenames: false🤖 Prompt for 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. In @.pre-commit-config.yaml around lines 28 - 39, The two cargo-clippy pre-commit hooks are inconsistent because the no_std variant does not use the same target coverage as the std variant. Update the cargo-clippy-no-std entry in .pre-commit-config.yaml to match the cargo-clippy hook’s scope, or add a short explanatory comment if omitting --all-targets is intentional; use the hook names cargo-clippy and cargo-clippy-no-std to locate the entries..github/workflows/ci.yaml (1)
2-10: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a
concurrencygroup to cancel superseded runs.With
pull_request+push:main+ weeklyscheduletriggers, rapid successive pushes/PR updates will queue overlapping full matrix runs (gate, test×3, nightly, cross). A concurrency group withcancel-in-progresswould save CI minutes.+concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + on: pull_request:🤖 Prompt for 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. In @.github/workflows/ci.yaml around lines 2 - 10, Add a workflow-level concurrency configuration to cancel superseded CI runs. In the CI workflow, update the top-level settings alongside the existing on triggers so that rapid updates to pull_request and push on main do not keep overlapping matrix jobs queued. Use a stable concurrency group keyed to the workflow and ref, and enable cancel-in-progress for the jobs defined in this workflow.
🤖 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 @.github/workflows/ci.yaml:
- Around line 13-24: Add an explicit permissions block to the GitHub Actions
workflow so all jobs use least-privilege GITHUB_TOKEN access. Update the
workflow around the gate/test/nightly/cross job definitions to set read-only
permissions, since the jobs only use checkout and local tooling and do not need
write scopes. Keep the change at the workflow level so it applies consistently
across all jobs in the file.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 2-10: Add a workflow-level concurrency configuration to cancel
superseded CI runs. In the CI workflow, update the top-level settings alongside
the existing on triggers so that rapid updates to pull_request and push on main
do not keep overlapping matrix jobs queued. Use a stable concurrency group keyed
to the workflow and ref, and enable cancel-in-progress for the jobs defined in
this workflow.
In @.pre-commit-config.yaml:
- Around line 28-39: The two cargo-clippy pre-commit hooks are inconsistent
because the no_std variant does not use the same target coverage as the std
variant. Update the cargo-clippy-no-std entry in .pre-commit-config.yaml to
match the cargo-clippy hook’s scope, or add a short explanatory comment if
omitting --all-targets is intentional; use the hook names cargo-clippy and
cargo-clippy-no-std to locate the entries.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfbc49c2-a7f6-465c-9802-c3132bc0b083
📒 Files selected for processing (14)
.github/workflows/ci.yaml.github/workflows/main.yaml.github/workflows/pr.yaml.pre-commit-config.yamlCargo.tomlREADME.mdbuild.rsci/rustup.shci/test_full.shsrc/float.rssrc/identities.rssrc/lib.rssrc/ops/format_into.rssrc/ops/mod.rs
💤 Files with no reviewable changes (9)
- ci/rustup.sh
- .github/workflows/main.yaml
- ci/test_full.sh
- .github/workflows/pr.yaml
- build.rs
- src/ops/format_into.rs
- src/ops/mod.rs
- src/lib.rs
- src/identities.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/float.rs
- Cargo.toml
- ci.yaml: set `persist-credentials: false` on all checkout steps and add a top-level `permissions: contents: read` block; no job needs write-back git credentials or a broad token scope (zizmor artipacked / excessive- permissions). - ci.yaml: add a `concurrency` group with cancel-in-progress so superseded pushes don't run overlapping matrices. - float.rs: forward to `<$T>::total_cmp(self, other)` without the extra borrow. - .pre-commit-config.yaml: lint no_std integration tests by adding `--tests` to the no_std clippy hook.
1 participant
Prunes leftover upstream scaffolding and one fragile feature ahead of the first crates.io release of the fork. Three independent cleanups; no change to the numeric API surface except the
nightly-stdremoval.CI — pre-commit-centric, no shell driver
.pre-commit-config.yamlis now the single source of truth for the enforced gate:cargo fmt --check, clippy onstd libm ctandno_std libm ctwith-D warnings, andcargo test. Localcargohooks (dropped the stale pinneddoublify/pre-commit-rust) so they target the fork's real feature set. Runs identically ongit commitand in CI.pr.yaml/main.yaml/ci.yaml→ oneci.yamlbackstop (pull_request+push:main+merge_group+ weeklyschedule). Jobs:gate(pre-commit),test(MSRV/stable/beta feature combos, inline cargo),nightly(const surface — now actually runstests/const_nightly.rs),cross(i586 + thumbv6m incl.ct),success.cargo test --all-featureson stable, which enablesnightly→#![feature(...)]→ E0554. Now uses an explicit stable feature set. Theno_std/i586jobs were also previously orphaned behindmerge_group-only triggering.ci/test_full.shandci/rustup.sh; repointed the README badgemain.yaml→ci.yaml.Drop
build.rs+ autocfgBoth probes (
has_total_cmp1.62,has_num_saturating1.74) are unconditionally true at MSRV 1.86. Deletedbuild.rs, theautocfgbuild-dependency, and inlined every cfg branch (thetotal_cmpmacro infloat.rs+ 12has_num_saturatingsites acrosslib.rsandidentities.rs). Dependents no longer compile or run a build script at all.Remove
nightly-std/format_intoThe
nightly-stdfeature name implied a category but gated exactly one trait (FormatInto), and the module could only "mirror std" by namingNumBuffer/NumBufferTrait— which on current nightly moved behind the compiler-internalfmt_internalsfeature (whileint_format_intostabilized). Depending on a compiler-internal feature is a non-starter for a stable-oriented fork, so the module is removed rather than patched. Bonus: plainnightlyis now a pure const-ness upgrade with no library feature gates, restoring the invariant thatnightlynever changes which items exist.Verification
cargo build(default), clippy-D warningsonstd libm ctandno_std libm ct,cargo test— all green.const_nightly.rsimpl constproofs.pre-commitrunner.Summary by Sourcery
Consolidate CI around pre-commit, remove the now-unnecessary build script and autocfg gating, and drop the fragile nightly-std formatting API to prepare the fork for a stable crates.io release.
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Summary by CodeRabbit
nightly-stdfeature.TotalOrder) to rely on the built-intotal_cmpbehavior.Saturatingidentity and numeric parsing/number support available consistently across builds.