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

Support rename rule for union body members #751

Closed
wants to merge 1 commit into from

Conversation

kavoor
Copy link
Contributor

@kavoor kavoor commented Apr 13, 2022

I am trying to use C++ macros to generate accessors for the fields in a rust enum. Since they are converted to SnakeCase by default, its impossible to write a macro that does this using only the original enum variant name.

Can we add an option to support different rename rules for the union body members as well?

@kavoor
Copy link
Contributor Author

kavoor commented Apr 13, 2022

@emilio What's the time table for getting something like this into a release?

Copy link
Collaborator

@emilio emilio left a comment

This needs a test, and I think the option naming could be more consistent? But let me know if you feel strongly, I guess just rename_fields could be seen as renaming the foo in Bar { foo: i32, bar: i32 }, even though it wouldn't.

In terms of getting it to a release, once it's merged I'm happy to publish it right away.

@@ -559,6 +559,9 @@ impl StructConfig {
pub struct EnumConfig {
/// The rename rule to apply to the name of enum variants
pub rename_variants: RenameRule,
/// The rename rule to apply to the names of union body members.
/// Applied before rename_variants rename rule. Defaults to SnakeCase.
pub rename_union_body_members: RenameRule,
Copy link
Collaborator

@emilio emilio Apr 14, 2022

Choose a reason for hiding this comment

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

So this is not for Rust unions (as in union Foo {} but C/C++ unions in an enum). I think this should just be rename_fields for consistency with config.{struct,union}.rename_fields, wdyt?

Copy link
Collaborator

@emilio emilio Apr 14, 2022

Choose a reason for hiding this comment

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

Maybe rename_variant_name_field would be more explicit? I dunno.

@emilio
Copy link
Collaborator

emilio commented Apr 14, 2022

If you can elaborate a bit on your use case it'd be great, too (maybe you can post an example of what you want to do exactly and how the renaming interferes?)

@kavoor
Copy link
Contributor Author

kavoor commented Apr 15, 2022

Whenever I run 'cargo test' I get errors like " panicked at 'build should succeed: CargoExpand("expand-dep", Compile("" which seem unrelated to my changes. I looked at contributing.md and only saw something about installing cython, which I did.

@emilio
Copy link
Collaborator

emilio commented Apr 15, 2022

On which OS? Does running rustup override add nightly help?

@kavoor
Copy link
Contributor Author

kavoor commented Apr 19, 2022

Thanks, that ended up being the issue! I think this should work now. Let me know if I need other tests.

emilio
emilio approved these changes Apr 19, 2022
Copy link
Collaborator

@emilio emilio left a comment

Thanks, looks good!

@emilio
Copy link
Collaborator

emilio commented Apr 19, 2022

Ah, rustfmt is complaining about the trailing whitespace, but I can address manually, assuming all other tests pass.

@emilio emilio closed this in 5b418d9 Apr 19, 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