-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix clippy lint #1181
Conversation
There was a problem hiding this 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-build/src/prost.rs
Outdated
let type_attributes = vec![( | ||
".".to_string(), | ||
"#[allow(clippy::derive_partial_eq_without_eq)]".to_string(), | ||
)]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I'll remove clippy attributes from each fille which includes proto (they were added in this pull request), and update generated codes. |
The ci failure is interesting I think maybe you didn't run it with the clean up feature enabled? |
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. |
Working on that now tokio-rs/prost#777 |
ok its been published as v0.11.4 |
…xtern_path test" This reverts commit 7fe1d32.
tests/root-crate-path/src/main.rs
Outdated
@@ -1,4 +1,4 @@ | |||
#[derive(Clone, PartialEq, ::prost::Message)] | |||
#[derive(Clone, PartialEq, Eq, ::prost::Message)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Signed-off-by: Gaius <gaius.qi@gmail.com>
Fixes clippy lints.