Skip to content

ci: fix clippy check spelling and labeler failures - #779

Closed
mcattamoredhat wants to merge 1 commit into
fdo-rs:mainfrom
mcattamoredhat:fix-clippy-spelling-devcontainer
Closed

ci: fix clippy check spelling and labeler failures#779
mcattamoredhat wants to merge 1 commit into
fdo-rs:mainfrom
mcattamoredhat:fix-clippy-spelling-devcontainer

Conversation

@mcattamoredhat

@mcattamoredhat mcattamoredhat commented Jul 7, 2025

Copy link
Copy Markdown
Collaborator

In order to fix Clippy issue, the strict warning treatment has been removed.

Some exceptions have been added to .github/spellcheck-ignore file.

.github/labeler.yml file has been updated to align with changes of Pull Request labeler v5 actions/labeler@v5 (add missing quotes and use the changed-files structure)

@mcattamoredhat
mcattamoredhat marked this pull request as draft July 7, 2025 15:04
@mcattamoredhat
mcattamoredhat force-pushed the fix-clippy-spelling-devcontainer branch from 2f17706 to 1974e1a Compare July 7, 2025 15:08
@miabbott

miabbott commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

@mcattamoredhat This is probably annoying, but could you split the changes into separate commits for each CI check you have fixed?

Comment thread .github/workflows/ci.yml
toolchain: stable
components: clippy
- run: cargo clippy -- -D warnings -D clippy::panic -D clippy::todo
- run: cargo clippy -- -D clippy::panic -D clippy::todo

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we don't have Rust veterans in the team I would encourage to maintain clippy failing on warnings, we need more learning opportunities

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok @7flying I understand. These warnings are related to the older format string syntax usage in rs files

I.e. https://github.com/fdo-rs/fido-device-onboard-rs/blob/main/util/src/servers/mod.rs#L73
format!("{}.yml", guid)
Instead of
format!("{guid}.yml")

There is more than one file using this syntax. Thus, should we change all of them (despite those changes wouldn't be related to Clippy command) in a separated PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mcattamoredhat I would run cargo clippy -- -D clippy::panic -D clippy::todo (as we do in the CI job), find all the warnings/panics and fix them. Then repeat the process until all the errors are fixed.

@mcattamoredhat

Copy link
Copy Markdown
Collaborator Author

@mcattamoredhat This is probably annoying, but could you split the changes into separate commits for each CI check you have fixed?

Sure, no problem @miabbott!

Comment thread .github/spellcheck-ignore
ot
marshalling
te
blacklist

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be a separate commit.

Comment thread .github/labeler.yml
possible stability impact:
- libfdo-data/fdo_data.h
- libfdo-data/libfdo-data-go.doc
"possible stability impact":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again a separate commit, generally each fix should be independent, make it easier for bisect and review.

@mcattamoredhat

Copy link
Copy Markdown
Collaborator Author

Closing this PR because the work has been splitted into separated commits.

@miabbott

miabbott commented Jul 8, 2025

Copy link
Copy Markdown
Contributor

Closing this PR because the work has been splitted into separated commits.

@mcattamoredhat For future reference, you can bundle multiple commits into a single PR.

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.

4 participants