-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Replaces invalid enum identifiers with generated ones #103
Conversation
src/output.rs
Outdated
} else if syn::parse_str::<syn::Ident>(&m.name).is_err() { | ||
// name is not a valid rust identifier -> generate one |
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.
does this catch keywords? we currently have a hardcoded keyword list in main 🙃
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.
If it does, we need to move the keyword check in here first and handle that separately.
I think before we move on to a full Variant rename, we should see if underscore prefixing in a rawstring is a valid identifier (because that's going to be the majority case). If that's not the case, then we should do Variants (and we should probably do Variants for ALL members at that point because otherwise we'd have to check variant names against regexes and it feels brittle).
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.
does this catch keywords? we currently have a hardcoded keyword list in main 🙃
Good point, I'll have to check. Is the plan to move all the renaming then from main.rs
into the Container::rename
? I think it would be best to have everything in one place.
I think before we move on to a full Variant rename, we should see if underscore prefixing in a rawstring is a valid identifier (because that's going to be the majority case).
Makes sense.
If that's not the case, then we should do Variants (and we should probably do Variants for ALL members at that point because otherwise we'd have to check variant names against regexes and it feels brittle)
I don't understand what you mean here, if one Variant can't be prefixed with r#_
use the generic name for all (even the ones that can be escaped with a raw 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.
keywords refactor and
Container::rename
Yeah, agreed. if we are starting to cover keyword related things inside output.rs then we should factor all of that out of main if possible.
if one Variant can't be prefixed with r#_ use the generic name for all (even the ones that can be escaped with a raw string)?
Yeah. Because otherwise you could end up with enums like:
enum Blah {
SomeSmartName,
SomeOtherName,
Variant1
SomeFourthName
}
and devs would likely goto upstream docs only to find that Variant1
does not exist there and get confused. If we renamed ALL of them to Variant{i}
then it's a little more clear that these types do not match directly to upstream and are more likely to read the #serde(rename)]
attrs.
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 does make the loop a little more convoluted though... Maybe if we just used less generic names (like VariantAlias{i}
or KopiumAlias{i}
this wouldn't be a problem 🤔
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 think we should just go with Kopium*
and assume there wont be any collisions, this require less logic in the generating code and should be safe enough to never have collisions (until we have a Kopium CRD).
Needs the DCO applied, but change is otherwise great. Thank you!
I think this convention here is fine. We can't really guess. If users do not like it, they can use the new
I think we should try to call it kind of leaning towards bailing if users have both
Similar to the "" case, yeah, we could do some basic checking, but I think we should have an either all or nothing approach to variant renames (either all get |
We also can just do nothing and generate an enum with 2 Variants as the same name. This will fail to compile and be pretty obvious to fix. Bailing is a bit annoying because then you have to edit the CRD first in order to generate something at all. |
Yeah, it's true. We could try to rely on compile failures on the user end, but that means the output of kopium is not super reliable (even when it succeeds) and people will accidentally publish broken specs. (Maybe down the line I think doing some basic syntax check on the file would be a nice optional extra.)
Yeah, this is a good point. We should probably always generate something, but I think what we generate should be valid. If we can't do that, we should bail. |
I pushed a new version with the renaming completely moved from It also does not need the special casing via I decided to go with the |
Yeah, that is a very sensible starting point 👍 |
Signed-off-by: David Herberth <github@dav1d.de>
Fixed the panic message. |
Related: #92 and #93
Initial draft that simply replaces invalid enum variants with auto generated names.
Things to discuss:
""
variants, we could call themEmpty
(maybe a separate issue)Variant{i}
is a valid variant defined by the CRD (unlikely))I also wasn't sure where to do the renaming, some of the renaming is done in
Container::rename
and some is done inmain
and there is also a little bit of logic inanalyze_enum_properties
.This currently also changes integer enum generation:
But I think int enums need to be generated differently anyways using serde_repr e.g.: