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.

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

enable clippy for tower-http and fix current issues #407

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GlenDC
Copy link
Contributor

@GlenDC GlenDC commented Sep 10, 2023

Motivation

I maintain a fork of tower, including also tower-http in https://github.com/plabayo/tower-async.
Only now did I notice that tower-http does not yet enable clippy.

Nothing bad was spotted by enabling it, but still,
seems by now that the Rust community agrees that enabling clippy everywhere is useful?
So as such, here my first contribution upstream.

Solution

N/A

@jplatte
Copy link
Collaborator

jplatte commented Nov 8, 2023

Not sure why CI isn't running. I'll try closing and re-opening.

@jplatte jplatte closed this Nov 8, 2023
@jplatte jplatte reopened this Nov 8, 2023
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but I want to also wait for David's opinion of including Clippy in CI.

@@ -3,3 +3,4 @@ members = [
"tower-http",
"examples/*",
]
resolver = "2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing final newline. Also an unrelated change, so may be worth a separate commit (though I think David always squash-merges anyways..)

Copy link
Contributor

@Oliboy50 Oliboy50 Dec 11, 2023

Choose a reason for hiding this comment

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

this PR should be rebased

at least because resolver = "2" has been added to Cargo.toml meanwhile

so it would be easier to review for David when he will come here...

Probably, but the bigger issue is first to ensure maintainers want to merge this in to begin with

if he comes here, indeed

@jplatte jplatte requested a review from davidpdrsn November 8, 2023 09:20
@Oliboy50
Copy link
Contributor

this PR should be rebased

@GlenDC
Copy link
Contributor Author

GlenDC commented Dec 11, 2023

this PR should be rebased

Probably, but the bigger issue is first to ensure maintainers want to merge this in to begin with.
If this MR Is desired then I'll get the PR ready with pleasure.
But until there is consensus on what to do with this PR there is no sense to put more effort into it.

Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

The removal of dbg! and some &s as well as using a nested or pattern in a match arm and switching the allow()ed clippy lint name where a previous lint was deprecated/replaced are all things I consider uncontroversial enough to merge. If you want to split the PR in two, I'm happy to merge that part.

@@ -211,7 +210,7 @@
nonstandard_style,
missing_docs
)]
#![deny(unreachable_pub, private_in_public)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this lint got replaced by smaller individual lints? Probably worth enabling those / some of them.

@@ -183,7 +183,6 @@
clippy::todo,
clippy::empty_enum,
clippy::enum_glob_use,
clippy::pub_enum_variant_names,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lint deprecated?

@jplatte
Copy link
Collaborator

jplatte commented Nov 23, 2024

Took another look, happy to merge once feedback that has been posted so far (excl. splitting the PR) is adressed. David hasn't been active in quite a while.

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.

3 participants