-
Notifications
You must be signed in to change notification settings - Fork 191
Resolve, rework, explain clippy lints #1661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good!
As these are quite a few changes let's split this into two parts. I.e. one commit for the naming changes and one for the expect
/allow
changes.
Reviewed 44 of 46 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 2 LGTMs obtained, and 45 of 46 files reviewed, and 6 discussions need to be resolved
nativelink-service/src/bep_server.rs
line 205 at r2 (raw file):
type PublishBuildToolEventStreamStream = PublishBuildToolEventStreamStream; #[allow(clippy::blocks_in_conditions)]
btw why is this no longer necessary?
nativelink-config/src/stores.rs
line 79 at r2 (raw file):
/// ``` /// ExperimentalS3Store(S3Spec),
nit: Please double-check that this one is still valid as experimental_s3_config
. There is a chance that it could resolve to experimental_s_3_store
now which would be breaking.
nativelink-config/src/stores.rs
line 677 at r2 (raw file):
/// /// see: <https://lz4.github.io/lz4/> Lz4(Lz4Config),
nit: Same here. It'll probably be fine, but better to be sure.
nativelink-store/tests/filesystem_store_test.rs
line 670 at r2 (raw file):
#[nativelink_test] #[allow(clippy::await_holding_refcell_ref)]
btw if this lint didn't trigger, shouldn't clippy have raised a warning that this line is redundant?
nativelink-worker/src/running_actions_manager.rs
line 96 at r2 (raw file):
#[derive(Debug, Deserialize)] #[serde(rename_all = "snake_case")] enum SideChannelFailureReason {
I believe we can remove the serde rename here as it's not exposed to the config. The comment about the doc update seems to refer to the docstring here:
/// "failure": "timeout", |
nativelink-util/src/origin_event_publisher.rs
line 91 at r2 (raw file):
#[expect( clippy::drain_collect, reason = "retain capacity for the next batch, as `mem::take` would move the \
This seems to make the reasoning less informative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained, and 45 of 46 files reviewed, and 2 discussions need to be resolved
nativelink-config/src/stores.rs
line 79 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: Please double-check that this one is still valid as
experimental_s3_config
. There is a chance that it could resolve toexperimental_s_3_store
now which would be breaking.
verified it is experimental_s3_config
by viewing the macro expansion
nativelink-config/src/stores.rs
line 677 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: Same here. It'll probably be fine, but better to be sure.
also verified as lz4
nativelink-service/src/bep_server.rs
line 205 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
btw why is this no longer necessary?
Resolved by tokio-rs/tracing#2880
nativelink-store/tests/filesystem_store_test.rs
line 670 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
btw if this lint didn't trigger, shouldn't clippy have raised a warning that this line is redundant?
Nope — that's the difference between #[allow]
and #[expect]
.
nativelink-util/src/origin_event_publisher.rs
line 91 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
This seems to make the reasoning less informative.
I'll copy the previous text in full.
nativelink-worker/src/running_actions_manager.rs
line 96 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
I believe we can remove the serde rename here as it's not exposed to the config. The comment about the doc update seems to refer to the docstring here:
/// "failure": "timeout",
The reason I included it despite it not being necessary is in case variants were added in the future (despite the enum not being #[non_exhaustive]
. If it's not necessary at all, why was it lowercased previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Reviewable status: 0 of 2 LGTMs obtained, and 26 of 46 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-cc / ubuntu-24.04, Local / lre-rs / macos-15, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, Web Platform Deployment / macos-15, Web Platform Deployment / ubuntu-24.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, pre-commit-checks, ubuntu-24.04, ubuntu-24.04 / stable, vale, windows-2022 / stable, and 2 discussions need to be resolved
All remaining clippy lints are converted to `#[expect]` with reasons attached. The only exceptions are a handful of `#[allow]` attributes in testing due to the manner in which tests are compiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 46 files at r1, 1 of 30 files at r3, 37 of 37 files at r4, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, ubuntu-24.04 / stable, windows-2022 / stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, ubuntu-24.04 / stable, windows-2022 / stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Coverage, Installation / macos-13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 2 LGTMs obtained, and all files reviewed, and pending CI: Coverage, Installation / macos-13
Description
All remaining clippy lints are converted to
#[expect]
with reasons attached. The only exceptions are a handful of#[allow]
attributes in testing due to the manner in which tests are compiled.Checklist
bazel test //...
passes locally — not done;cargo
should be sufficient heregit amend
see some docsThis change is