Skip to content
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

Fix clippy lints #477

Merged
merged 3 commits into from
Dec 21, 2019
Merged

Fix clippy lints #477

merged 3 commits into from
Dec 21, 2019

Conversation

silvanshade
Copy link
Contributor

This PR fixes all reported clippy lints. In most cases I have made the suggested changes. In a few cases (e.g., blacklisted_name and cognitive_complexity) I've just silenced the warning.

I can make stylistic changes or discard some of the lint fixes if preferred.

hawkw
hawkw requested changes Dec 18, 2019
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thanks for all the work! I have a few changes I'd like to make if possible.

examples/examples/fmt/yak_shave.rs Outdated Show resolved Hide resolved
examples/examples/sloggish/sloggish_subscriber.rs Outdated Show resolved Hide resolved
examples/examples/sloggish/sloggish_subscriber.rs Show resolved Hide resolved
examples/examples/sloggish/sloggish_subscriber.rs Outdated Show resolved Hide resolved
tracing-attributes/tests/destructuring.rs Outdated Show resolved Hide resolved
tracing/tests/support/span.rs Show resolved Hide resolved
tracing-subscriber/src/lib.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/registry/sharded.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/env/directive.rs Outdated Show resolved Hide resolved
examples/examples/counters.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Dec 18, 2019

Also, it looks like CI is failing because rustfmt needs to be run: https://dev.azure.com/tracing/tracing/_build/results?buildId=1303&view=logs&j=d58818d2-6188-5422-441f-033b6bb63300&t=5fc570b0-d721-5027-b19d-e5c09c36d0ce

I find that kind of amusing, it's like a battle of the linters.

@silvanshade
Copy link
Contributor Author

I find that kind of amusing, it's like a battle of the linters.

Yeah… 🙂 I thought I ran rustfmt but guess not. I'll do that when I make the suggested changes.

Thanks for the quick feedback!

@silvanshade
Copy link
Contributor Author

In the latest commits I've removed the global allows and instead added a clippy.toml. The file configures clippy to clear the blacklisted names (so that foo is allowed) and to also increase the cognitive complexity threshold high enough so that all of the examples pass without warning. I think this is probably the best compromise since it doesn't require rewriting any of the code and doesn't clutter the source files with clippy attributes.

@silvanshade
Copy link
Contributor Author

Just a note, I still plan to make the other suggested changes.

@silvanshade
Copy link
Contributor Author

Alright, I think I've made the suggested changes and removed as many of the clippy allows as possible. See the comment about the wrong_self_convention lint. I can squash these commits if it looks good.

@silvanshade
Copy link
Contributor Author

Looks like CI is still failing for some reason but I can't figure out why.

@silvanshade silvanshade force-pushed the fix-clippy-lints branch 3 times, most recently from a30f146 to 975384f Compare December 19, 2019 20:52
hawkw
hawkw approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Looks like CI is still failing for some reason but I can't figure out why.

I believe the CI failure is unrelated. It's due to a Cargo change in 1.40.0 that breaks the Azure Pipelines config: https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#changed-1

We should probably address that separately. Besides that, I think this PR looks good.

By including a clippy.toml and Clippy attributes in the project, I think we are now publicly committing to using Clippy — if that's the case, we should probably be running it on CI as well. However, I think we should do that in a separate branch. If you don't mind opening an issue for that, that would be great!

Again, thanks for sticking with this!

tracing-core/src/span.rs Show resolved Hide resolved
tracing-attributes/tests/support.rs Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Dec 20, 2019

@darinmorrison is this ready to merge? It looks like everything has been squashed down. Were you planning on making any additional changes?

For what it's worth, it's not actually necessary to squash in progress PRs; we squash everything automatically when merging: https://github.com/tokio-rs/tracing/blob/master/CONTRIBUTING.md#commit-squashing

@silvanshade
Copy link
Contributor Author

Going to add that one last comment about module inception then it's ready to go!

@silvanshade
Copy link
Contributor Author

@hawkw Okay I think it's ready now. Good to know about squashing commits for future reference.

@hawkw
Copy link
Member

hawkw commented Dec 20, 2019

I believe I've fixed all the CI issues that are causing builds to hang. @darinmorrison can I get you to trigger a new build by pushing an empty commit to this branch? You can do this using

git commit -m "restart CI" --allow-empty && git push

or similar.

Thanks!

@silvanshade
Copy link
Contributor Author

@hawkw sorry I missed your comment earlier. Just pushed to restart CI now.

@hawkw hawkw merged commit 048b85c into tokio-rs:master Dec 21, 2019
@hawkw
Copy link
Member

hawkw commented Dec 21, 2019

@darinmorrison Okay, now that you triggered a new build, CI is passing again, so I've gone ahead and merged this branch. Thanks again for working on this!

@silvanshade
Copy link
Contributor Author

No problem! I still plan to make the PR for clippy on CI too by the way.

@silvanshade silvanshade mentioned this pull request Dec 21, 2019
Merged
hawkw pushed a commit that referenced this pull request Jan 6, 2020
@silvanshade @hawkw
ci: run clippy-check action on CI (#491)
986c68d 
This is a follow up PR for #477 that runs clippy on CI.

* Switch from hecrj/setup-rust-action to actions-rs/toolchain

* Fix remaining clippy lints

* Switch to minimal rustup profile

* Replace warning step with clippy

* Remove #![feature(async_await)]
@silvanshade silvanshade deleted the fix-clippy-lints branch January 12, 2020 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw 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.
3 participants
@silvanshade @hawkw @darinmorrison