Add support for enum and oneof field deprecated. #701
Conversation
| @@ -670,6 +679,10 @@ impl<'a> CodeGenerator<'a> { | |||
| self.depth += 1; | |||
|
|
|||
| for variant in variant_mappings.iter() { | |||
| if variant.deprecated { | |||
| self.push_indent(); | |||
| self.buf.push_str("#[allow(deprecated)]\n"); | |||
There was a problem hiding this comment.
Why is the allow statement now needed and not is previous version?
There was a problem hiding this comment.
This direct call will raise deprecated warning for every call of as_str_name, so we need a #[allow(deprecated)] for normal call.
For deprecated enum, direct call as_str_name will still raise warning.
prost-derive/src/lib.rs
Outdated
| @@ -304,6 +305,7 @@ fn try_enumeration(input: TokenStream) -> Result<TokenStream, Error> { | |||
| } | |||
| } | |||
|
|
|||
| #[allow(deprecated)] | |||
There was a problem hiding this comment.
Will this hide deprecation warnings in non deprecated fields?
| }; | ||
| #[allow(deprecated)] | ||
| let enum_ = deprecated_field::Test2::Outdated; |
There was a problem hiding this comment.
I would like to see an additional test: generate a proto with only deprecated fields and write that to file. This has two purposes:
- It adds a example of all
deprecatedstatements to the PR - It actually tests that the
deprecatedflag is added to the fields.
The repo has examples for a test writing to file and asserting nothing changes: test_generate_no_empty_outputs
There was a problem hiding this comment.
Thanks for creating that test
sonder-joker
force-pushed
the
master
branch
2 times, most recently
from
52e80f4
to
42afd88
Compare
| @@ -670,6 +679,10 @@ impl<'a> CodeGenerator<'a> { | |||
| self.depth += 1; | |||
|
|
|||
| for variant in variant_mappings.iter() { | |||
| if variant.deprecated { | |||
| self.push_indent(); | |||
| self.buf.push_str("#[allow(deprecated)]\n"); | |||
| let tempdir = tempfile::tempdir().unwrap(); | ||
|
|
||
| Config::new() | ||
| .service_generator(Box::new(gen)) |
There was a problem hiding this comment.
Is service_generator needed for this test?
| match discriminant { | ||
| Some((_, expr)) => variants.push((ident, expr)), | ||
| Some((_, expr)) => { | ||
| let deprecated_attr = if attrs.contains(&parse_quote! { #[deprecated] }) { |
There was a problem hiding this comment.
I don't have experience with syn, but this pattern is different from other uses.
In the existing code, the attrs are matched with is_ident. For example:
https://github.com/caspermeijn/prost/blob/87540c6bd00848a4f4591512e9b27e3dd5024398/prost-derive/src/lib.rs#L26-L29
What is the reason this works differently?
| @@ -460,6 +467,7 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> { | |||
| }); | |||
|
|
|||
| let expanded = quote! { | |||
| #[allow(deprecated)] | |||
There was a problem hiding this comment.
Why is this not conditional, but the other allow statements are?
| }; | ||
| #[allow(deprecated)] | ||
| let enum_ = deprecated_field::Test2::Outdated; |
There was a problem hiding this comment.
Thanks for creating that test
I notice it's not support enum and oneof field deprecated.