Skip to content

Conversation

cgwalters
Copy link
Collaborator

Just a run of cargo clippy --fix plus some manual bits.

I keep going back and forth on clippy...it's really a spectrum between useful and noisy.

@bootc-actions-token bootc-actions-token bot requested a review from jeckersb August 26, 2025 12:25
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request applies a series of fixes suggested by cargo clippy. The changes are mostly stylistic and idiomatic improvements, such as using more concise language features and removing redundant code. The changes look good and improve the overall code quality. I've found one minor opportunity for improvement by removing a redundant .clone() call.

@@ -201,7 +201,7 @@ async fn handle_layer_progress_print(
subtask = SubTaskBytes {
subtask: layer_type.into(),
description: format!("{layer_type}: {short_digest}").clone().into(),
id: format!("{short_digest}").clone().into(),
id: short_digest.to_string().clone().into(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The .clone() here is redundant. short_digest.to_string() creates a new String, and .into() can consume it directly to create the Cow<'t, str>. You can simplify this line. The same applies to the description field on the line above.

Suggested change
id: short_digest.to_string().clone().into(),
id: short_digest.to_string().into(),

Copy link
Collaborator

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So close to having a bare cargo clippy be clean! Last two things maybe for a followup?

warning: this function has too many arguments (8/7)
   --> crates/tmpfiles/src/lib.rs:318:1
    |
318 | / fn convert_path_to_tmpfiles_d_recurse<U: uzers::Users, G: uzers::Groups>(
319 | |     out_entries: &mut BTreeSet<String>,
320 | |     out_unsupported: &mut Vec<PathBuf>,
321 | |     users: &U,
...   |
326 | |     readonly: bool,
327 | | ) -> Result<()> {
    | |_______________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
    = note: `#[warn(clippy::too_many_arguments)]` on by default

Would be easy enough to refactor this to take a struct of options instead, I agree with clippy that 8 arguments is a bit much.

    Checking bootc-sysusers v0.1.0 (/var/home/jeckersb/git/bootc/crates/sysusers)
warning: `bootc-tmpfiles` (lib) generated 1 warning
error: use of a disallowed method `str::len`
   --> crates/sysusers/src/lib.rs:132:75
    |
132 |                 let idx = s.find(|c: char| c.is_whitespace()).unwrap_or(s.len());
    |                                                                           ^^^
    |
    = note: use <str>.as_bytes().len() instead
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
    = note: requested on the command line with `-D clippy::disallowed-methods`

error: could not compile `bootc-sysusers` (lib) due to 1 previous error

We explicitly check for this via clippy.toml, should fix. Also an existing bug that the target for validate-rust does not include -D clippy::disallowed-methods which has let this sneak past CI.

@jeckersb jeckersb merged commit ea360d6 into bootc-dev:main Aug 26, 2025
25 of 27 checks passed
@cgwalters
Copy link
Collaborator Author

Would be easy enough to refactor this to take a struct of options instead, I agree with clippy that 8 arguments is a bit much.

I don't have a strong opinion here, but I just allowed it for now.

We explicitly check for this via clippy.toml, should fix. Also an existing bug that the target for validate-rust does not include -D clippy::disallowed-methods which has let this sneak past CI.

Done in #1552

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants