Close 166 of the clippy::pedantic warnings - #444
Merged
Conversation
The progress counter accumulated a `f32` percentage, one addition per chunk, and then cast the result back to `usize`. The rounding error grew with the number of chunks, so a complete download reported 99% instead of 100%. The cast also triggered `cast_possible_truncation`, `cast_sign_loss`, and `cast_precision_loss` under `clippy::pedantic`. Track the number of written bytes as a `u64` and derive the percentage from the total on each chunk. The result is exact and no cast remains. The log normalization in the tests accepted two digits only, which was enough while the counter never reached 100. Widen it to three digits and normalize the snapshot lines that recorded the literal `100%`.
`clippy::pedantic` reports every `as` cast that can truncate or lose the sign. The remaining cases convert a block count or a buffer size, which a 32-bit target can truncate without any sign of failure. Replace each cast with `try_from`: - `Count::Limited` holds an `isize` that the deserializer keeps at zero or above, so a negative value can only come from code. Map it to zero, which copies nothing, instead of a very large block count. - `Pattern::buffer_size` is a `u64` read buffer capacity. Clamp it to `usize::MAX`. - The test helpers panic on a value that does not fit, because the test itself supplies it. Also replace `to_owned` and an assignment of a clone with `clone` and `clone_from`, and drop a `to_vec` call on a value that is already a `Vec`.
The standard library gained `LazyLock` in Rust 1.80, so the `lazy_static` macro no longer earns its place. `clippy::non_std_lazy_statics` reports each of the four remaining uses. Declare each static directly. The types stay the same, so every call site keeps working through `Deref`. Drop the `lazy_static` dependency from the updatehub crate.
This is a code style pass with no change in behavior. It closes every `clippy::pedantic` warning
that names a concrete rewrite:
- add the missing semicolon on a statement that returns the unit type, and drop the ones that add
nothing;
- replace a closure that only calls one method with the method itself, and call `to_string` on the
value instead of on a double reference;
- write `String::new` instead of an empty literal, and drop the raw string hashes that no string
needs;
- prefer `if let` and `let ... else` over a match with one meaningful arm, join the arms that share
a body, and name the last variant instead of a wildcard;
- move an item declaration above the statements of its function;
- take `Option<&T>` instead of `&Option<T>` in `fs::format`, `fs::chown`, and
`should_skip_install`, which removes one level of indirection at every call site.
Some warnings stay open on purpose:
- `unnecessary_debug_formatting` marks paths written with `{:?}`. The quotes it adds are what makes
a path with a space safe inside the shell commands the flash and raw-delta handlers build.
- `used_underscore_binding` marks the `_mount` field of `MountGuard`, whose name states that the
guard only holds the mount alive.
- `wildcard_imports` marks `use super::*` in a test module, which is the usual pattern.
- `unused_async` marks trait methods with a default body, the mock cloud client that must match the
real signature, a warp handler, and a public SDK function. The `async` is part of the contract in
each case.
- `too_many_lines` needs the functions to be split, which does not belong in a style pass.
`clippy::ignore_without_reason` reports a bare `#[ignore]`, because the attribute alone leaves the reader to guess whether the test is broken or whether it needs something the machine does not offer. Give a reason to the sixteen tests that stay ignored. Six of them attach a loop device and ten of them load the MTD simulator modules, so both groups need root. The comment above two of the filesystem tests said the same thing, so drop it. The two download tests need nothing beyond the mock cloud client, and they finish in under two seconds together. Run them by default.
`clippy::pedantic` asks three things of a public item: that prose names read as prose, that a function which returns `Result` says when it fails, and that a function whose result is the only reason to call it carries `#[must_use]`. Add an `# Errors` section to the ten functions of the two SDK crates and to the four entry points of the agent. Add a `# Panics` section where an `unwrap` can still fire: the HTTP client builder and the rename of a runtime settings file. Mark the eight pure accessors and constructors with `#[must_use]`. `doc_markdown` reported `UpdateHub` and `OpenAPI` nineteen times. Both are names, not code, so they take no backticks. Record them in a new clippy.toml instead. The three remaining reports name real functions inside an intra-doc link, so give those the backticks they ask for. The test helpers stay undocumented. `must_use_candidate` and `missing_panics_doc` on test scaffolding describe nothing a reader of the tests does not already see.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cargo clippy --all-features --all-targets -- -W clippy::pedanticreported 203 warnings onmaster. This branch closes 166 of them across six commits, one concern per commit. Thedefault clippy run was clean before this branch and stays clean.
One warning turned out to hide a real defect. The rest are readability and documentation.
The defect
save_body_toin the cloud SDK logged the download progress from af32that it grew by oneaddition per chunk, then cast back to an integer. The rounding error grew with the number of
chunks, so a download that finished logged
99%. It now counts bytes in au64and derives thepercentage from the total, which is exact and needs no cast at all.
The change surfaced a second, smaller problem: the integration tests normalized the percentage
with a regular expression that accepted two digits only. It never saw a three digit value before,
so
100%reached the snapshot as a literal. The expression now accepts one to three digits.The commits
fix: cloud-sdk: compute the download progress with integer arithmeticrefactor: remove the lossy casts and the implicit clonesascast that can truncate or lose the sign withtry_from, and removes three clones written in a roundabout way.refactor: replace lazy_static with std::sync::LazyLocklazy_staticdependency goes away.style: apply the mechanical clippy::pedantic code fixesif letover one-armed matches,Option<&T>over&Option<T>. No change in behavior.test: state why each ignored test stays out of the default run#[ignore]attributes now say what they need. Two more do not need anything, so they run by default.docs: document how the public functions fail, and mark the pure ones# Errorsand# Panicssections on the public API,#[must_use]on the pure accessors, and aclippy.tomlthat knowsUpdateHubis a name.What stays open, and why
37 warnings remain. Each one asks for a change that would make the code worse, or for work that
does not belong in a cleanup branch.
unused_asyncasyncis part of the contract.unnecessary_debug_formatting{:?}quotes a path. The flash and raw-delta handlers build shell commands, where the quotes are what makes a path with a space safe.too_many_linesmust_use_candidate,missing_panics_doc,needless_pass_by_valueused_underscore_binding_mountfield ofMountGuard. The name states that the guard only holds the mount alive.wildcard_importsuse super::*in a test module.How I verified it
cargo fmt --all -- --checkcargo clippy --locked --all-features --all --tests -- -D clippy::allcargo check --locked --release --all --bins --examples --tests, with and without--all-featurescargo test --locked --all --all-features --no-fail-fast: 96 unit tests, 13 successfulintegration tests, 7 failed-path integration tests, and the rest of the suite all pass. 16 tests
stay ignored because they need root.
The tests that need root and MTD hardware simulation were not run. They were already outside the
default run before this branch.