Skip to content

ensure clippy runs on all targets #262

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Dec 29, 2021
Merged

ensure clippy runs on all targets #262

merged 6 commits into from
Dec 29, 2021

Conversation

danieleades
Copy link
Contributor

note that there are clippy false positives due to rust-lang/rust-clippy#8140

matthiasbeyer
matthiasbeyer requested changes Dec 26, 2021
View reviewed changes
Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

Can you please split the commits so that we have only one commit per type of change?

@danieleades danieleades force-pushed the chore/clippy-all-targets branch from d228e99 to 7fc90e9 Compare December 27, 2021 11:13
@danieleades danieleades mentioned this pull request Dec 28, 2021
Merged
@danieleades danieleades force-pushed the chore/clippy-all-targets branch 2 times, most recently from 1f820ff to 4575e9b Compare December 29, 2021 10:50
@danieleades
Copy link
Contributor Author

note that there is some remaining use of deprecated code in the examples. It's not clear to me how these use cases should be supported using the builder API. I've allowed these for now, but they should be addressed in a future PR

@danieleades danieleades mentioned this pull request Dec 29, 2021
Closed
@matthiasbeyer
Copy link
Member

You're adding #[must_use] f7899fd in and removing it again in 48f49c6 - was this by accident? 😆

@danieleades
Copy link
Contributor Author

You're adding #[must_use] f7899fd in and removing it again in 48f49c6 - was this by accident? laughing

suspect that snuck in during a rebase. I'll take a look

@danieleades danieleades force-pushed the chore/clippy-all-targets branch 2 times, most recently from f9bb99d to 2aeffe3 Compare December 29, 2021 11:48
@matthiasbeyer
Copy link
Member

For the "global" example you can cherry-pick matthiasbeyer@1eff7b3 to this PR if you want (or wait until the actions succeed) 😆

@danieleades
Copy link
Contributor Author

For the "global" example you can cherry-pick matthiasbeyer@1eff7b3 to this PR if you want (or wait until the actions succeed) laughing

this change doesn't look equivalent to the original to me. If i'm reading it right, the original example is mutable global config, while the updated version is immutable global config.

in any case, I think I'd rather leave it out of this PR. Don't want to over-complicate the review!

@matthiasbeyer
Copy link
Member

Okay!

@matthiasbeyer matthiasbeyer mentioned this pull request Dec 29, 2021
Closed
@danieleades danieleades force-pushed the chore/clippy-all-targets branch from 2aeffe3 to a880573 Compare December 29, 2021 12:13
@matthiasbeyer
Copy link
Member

Do you consider this review/merge ready? 😄 I think I lost track a bit!

@danieleades
Copy link
Contributor Author

Do you consider this review/merge ready? smile I think I lost track a bit!

yes, i'm not planning to do any more on this PR

@matthiasbeyer matthiasbeyer merged commit 918c52e into rust-cli:master Dec 29, 2021
@danieleades danieleades deleted the chore/clippy-all-targets branch December 29, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasbeyer matthiasbeyer matthiasbeyer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danieleades @matthiasbeyer