Use inferred union variant names in more cases #894
Conversation
arlyon
force-pushed
the
arlyon/instance-type-inference
branch
from
ac77c4b to
a76393e
Compare
| #[serde(untagged)] | ||
| pub enum JankNames { | ||
| Variant0(::std::string::String), | ||
| Variant1(::std::collections::HashMap<::std::string::String, ::std::string::String>), |
There was a problem hiding this comment.
These clash and so stay as Variant0 and Variant1, however the object variant will be given a descriptive name.
There is an argument to be made to call them <INFERRED_NAME>0 and <INFERRED_NAME>1 instead. It is fairly easy to add. Will let you make the call.
There was a problem hiding this comment.
This is surprising. Variants 0 and 1 have the same title; variants 1 and 2 have the same type (object).
There was a problem hiding this comment.
The clash is resolved at the name level, and the heuristic currently only produces a single name (rather than a ranked list of Variant1 or else Object for example) and so there is a clash for the first 2 but not the third.
Will be irrelevant when I revert the best-effort name resolution in favour of all-or-nothing.
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
Thanks very much for this submission. Please see my other comments; happy to discuss.
typify-impl/src/enums.rs
Outdated
| // Store seen, un-stripped names here. Any item for which their | ||
| // name appears more than once, ALL the items with that clashing | ||
| // name will be converted to the `VariantN` scheme. |
There was a problem hiding this comment.
I realize this is a matter of aesthetics, but I don't think we should mix naming schemes: either we have a "good" name that we derive from the schema or the type and is unique, or we use Variant{n}. I think that mix and match is worse.
There was a problem hiding this comment.
Your call. I will revert that part of the functionality but leave it in a separate commit in case the opinion changes.
| #[serde(untagged)] | ||
| pub enum JankNames { | ||
| Variant0(::std::string::String), | ||
| Variant1(::std::collections::HashMap<::std::string::String, ::std::string::String>), |
There was a problem hiding this comment.
This is surprising. Variants 0 and 1 have the same title; variants 1 and 2 have the same type (object).
|
Feedback addressed. Thank you for the prompt reply 🥇 |
arlyon
force-pushed
the
arlyon/instance-type-inference
branch
from
a76393e to
83a6da7
Compare
arlyon
force-pushed
the
arlyon/instance-type-inference
branch
from
83a6da7 to
347a338
Compare
arlyon
force-pushed
the
arlyon/instance-type-inference
branch
2 times, most recently
from
40669ea to
f8b3c71
Compare
|
Ok, pared back the impl and added support for format which as added DateTime, Uuid, and Uri. 🎉 |
arlyon
force-pushed
the
arlyon/instance-type-inference
branch
from
f8b3c71 to
a5f0ed8
Compare
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
Nice improvement; thanks for the contribution!
2 participants
This is a breaking change.
This PR achieves two things:
change the logic to allow for combinations of named an unnamed (VariantN) variants in enums, rather than an all-or-nothing approachYou can see the outcome on the codegen from the snapshot tests, and I have included a new snapshot test that exercises the partially-named case.
I decided to keep the Variant number as its index in the overall type list, not as its index amongst unnamed types such that its name is stable under the addition or removal of metadata earlier in the schema that would add or remove inferred names.
This also changes the behaviour upon encountering duplicates. Previously, there was a later check to assert the code did not produce them. Now, in the case that the inferred name is the same for two or more variants, we just eject the affected enum back to
VariantN