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 clippy lint #1181

Merged
merged 25 commits into from
Dec 13, 2022
Merged

Fix clippy lint #1181

merged 25 commits into from
Dec 13, 2022

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Dec 6, 2022

Fixes clippy lints.

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 just want to see the eq stuff backed out into codegen and have one qusetion

tonic/src/transport/service/io.rs Show resolved Hide resolved
Comment on lines 18 to 21
let type_attributes = vec![(
".".to_string(),
"#[allow(clippy::derive_partial_eq_without_eq)]".to_string(),
)];
Copy link
Collaborator Author

@tottoto tottoto Dec 7, 2022

Choose a reason for hiding this comment

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

As adding attributes might only be configured via prost api, it seems like we might need to configure in this way. However I'm not sure that this is an appropriate approach.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh I wonder if we should just fix this in prost then? Somewhere in this macro impl https://github.com/tokio-rs/prost/blob/master/prost-derive/src/lib.rs#L21 since really the problem is I don't want code generated stuff to impl Eq or we can put this in prost-build?

Copy link
Collaborator Author

@tottoto tottoto Dec 8, 2022

Choose a reason for hiding this comment

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

I published tokio-rs/prost#775 so as to try moving this to prost-build.

@tottoto
Copy link
Collaborator Author

tottoto commented Dec 7, 2022

I'll remove clippy attributes from each fille which includes proto (they were added in this pull request), and update generated codes.

@LucioFranco
Copy link
Member

The ci failure is interesting I think maybe you didn't run it with the clean up feature enabled?

@tottoto tottoto marked this pull request as draft December 9, 2022 15:37
@tottoto
Copy link
Collaborator Author

tottoto commented Dec 9, 2022

I marked this pull request as draft because of using unreleased prost-build (tokio-rs/prost#775). I'll remove this and update prost-build when it is released.

I fixed the codegen in 23ebd27, and updated generated codes in 68b61d0. Thanks for your advice.

@LucioFranco
Copy link
Member

Working on that now tokio-rs/prost#777

@LucioFranco
Copy link
Member

ok its been published as v0.11.4

@tottoto
Copy link
Collaborator Author

tottoto commented Dec 10, 2022

I updated prost-build to 0.11.4 in e7efd33 and fixed some warnings in 7c455f7.

@tottoto tottoto marked this pull request as ready for review December 10, 2022 04:08
@tottoto
Copy link
Collaborator Author

tottoto commented Dec 10, 2022

I reverted 7fe1d32 in 728b040 as it was taken over by prost-build 0.11.4.

@@ -1,4 +1,4 @@
#[derive(Clone, PartialEq, ::prost::Message)]
#[derive(Clone, PartialEq, Eq, ::prost::Message)]
Copy link
Member

Choose a reason for hiding this comment

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

This should get an allow with the new version of prost right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This looks like my mistake. I'll fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

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.

Thanks!

@LucioFranco LucioFranco merged commit 2dd09c0 into hyperium:master Dec 13, 2022
@tottoto tottoto deleted the fix-clippy-lint branch December 13, 2022 16:32
gaius-qi added a commit to dragonflyoss/api that referenced this pull request Apr 3, 2023
Signed-off-by: Gaius <gaius.qi@gmail.com>
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.

2 participants