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

Add enum variant to string mapping #659

Merged
merged 4 commits into from
Jun 8, 2022
Merged

Add enum variant to string mapping #659

merged 4 commits into from
Jun 8, 2022

Conversation

vriesk
Copy link
Contributor

@vriesk vriesk commented May 27, 2022

No description provided.

@vriesk
Copy link
Contributor Author

vriesk commented May 30, 2022

@LucioFranco friendly ping :)

@LucioFranco
Copy link
Member

Sorry for the delay on this was out on vacation and still suffering from some jetlag. I took a cursory review but would like to spend some more time looking at this. Just wanted to let you know what the status was.

As for one thing, since this adds a method on the generated codegen types. I am leaning that we should just add this not as an opt-in but as a on by default without the config. This would also mean that prost-types would need to be updated and I would like to see that since it will show case some of the codegen.

@vriesk
Copy link
Contributor Author

vriesk commented Jun 7, 2022

Ack, I can remove the config guard. Do you have some specific method to regenerate the prost-types or is it just output of prost-build run?

@LucioFranco
Copy link
Member

Its done via a test in bootstrap.rs, basically, when you make a change that changes the output of prost-types generated types the tests fails and updates it. So when you run the test for the second time it will pass. So this will just fail on CI when you haven't checked in the new code.

@vriesk
Copy link
Contributor Author

vriesk commented Jun 7, 2022

Done.

@LucioFranco
Copy link
Member

error[E0658]: arbitrary expressions in key-value attributes are unstable
Error: --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/libz-sys-1.1.8/src/lib.rs:115:19
|
115 | #[link_name = zng_prefix!(adler32)]
| ^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #78835 rust-lang/rust#78835 for more information

Looks like libz update is causing msrv issues, will need to investigate this

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 changed the title Add enum variant to string mapping, guarded by a config option. Add enum variant to string mapping Jun 8, 2022
@LucioFranco LucioFranco merged commit 26388a7 into tokio-rs:master Jun 8, 2022
@vriesk
Copy link
Contributor Author

vriesk commented Jul 3, 2022

@LucioFranco I have just noticed that clippy whines about the to_str_name() name:

warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value

Maybe let's change it to as_str_name() then before this gets released? WDYT?

@LucioFranco
Copy link
Member

@vriesk good idea, lets do that.

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