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

Fix all clippy warnings #652

Merged
merged 3 commits into from Dec 12, 2022
Merged

Fix all clippy warnings #652

merged 3 commits into from Dec 12, 2022

Conversation

gtsiam
Copy link
Contributor

@gtsiam gtsiam commented Dec 1, 2022

Fixes all clippy warnings.

Split from #649.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM but left some comments, we should back out some of the clippy changes and add them to a global allow.

src/frame/headers.rs Show resolved Hide resolved
src/frame/headers.rs Outdated Show resolved Hide resolved
src/frame/settings.rs Outdated Show resolved Hide resolved
src/frame/settings.rs Outdated Show resolved Hide resolved
src/hpack/decoder.rs Outdated Show resolved Hide resolved
src/proto/streams/streams.rs Outdated Show resolved Hide resolved
@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 1, 2022

Also while I'm at it: Would you like me to add a style check to the ci so there's no need to do this kind of large pr again?

@LucioFranco
Copy link
Member

Also while I'm at it: Would you like me to add a style check to the ci so there's no need to do this kind of large pr again?

You mean run clippy in ci? I personally don't like it since it adds a lot of noise.

@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 5, 2022

You mean run clippy in ci? I personally don't like it since it adds a lot of noise.

Yes. Though I really wouldn't call clippy warnings "noise". With the exception of the manual_range_contains lint, most of the changes in this pr should never have needed to be done in mass. Catching them earlier would be much easier for all involved.

But I'm not gonna push for it. You're the maintainer. If you don't want it, you don't want it and that's the end of that.

@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 5, 2022

Ok, everything should be resolved now

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@LucioFranco LucioFranco merged commit 07d20b1 into hyperium:master Dec 12, 2022
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.

None yet

2 participants