Enum dedup flag #285
Enum dedup flag #285
Conversation
robshakir
commented
May 10, 2019
robshakir
commented
* (M) ygen/*
- In some schemas, a single grouping with an enumerated leaf in
it is widely used across the schema (e.g., OpenHLTest), with
ygen's default behaviour this would result in a single enum
being output with the path of the first alphabetical place that
this was used in the schema being selected as the name for the
leaf. This pattern works well with OpenConfig, where this means
that enumerations that are in both config and state have the same
type, as well as those that are in, say, /bgp and /network-instance,
however, for other schema it doesn't work well -- with essentially
arbitrary naming of the enum being used. Naming after the grouping
seems suboptimal since in this case, we would imply that the
downstream developer knows about the actual YANG files (a core
principle of the project is that this ISN'T true!). So, add a
flag to allow this behaviour to be controlled. With the new flag
all enums that are from the same grouping can be selected to have
their own type generated in the code.
- Implement the above flag, and testcases.
- Misc. cleanup of code to meet bestpractices where the change was
in the general vicinity.
* (M) {generator,proto_generator}
- Expose the SkipEnumDeduplication flag to the generator binaries.
Handle corner case where compressed schemas contain an enum used in two places but have the same name. This is a case where the enum name should be deduped, as otherwise an underscore will be added for one of them, subtracting from the original goal of improving usability. This change required processing the skipEnumDedup flag when processing unions as well, as this change makes it so that resolveEnumName requires skipEnumDedup is always the same between different calls to it. More tests are added to test the combination of compress + skipDedup options.
|
Rob's changes LGTM, Rob please take a look at my commit: 812b889 If you're ok then I will approve and submit. |
|
There are a couple places |
ygen/genstate.go
Outdated
| // It is possible, given a particular enumerated leaf, for it to appear | ||
| // multiple times in the schema. For example, through being defined in | ||
| // a grouping which is instantiated in two places. In these cases, the |
There was a problem hiding this comment.
As above, why is it two usages will have the same yang.Node? Thanks.
There was a problem hiding this comment.
When goyang is parsing a YANG schema, then it goes through and finds the nodes that are defined in the schema - which is in relation to what is actually written in the YANG file. When it converts this tree to entries, it then builds the tree such that the schema tree is expanded out -- so if we have:
grouping foo {
leaf a { type enumeration { enum A; } }
}
container one { uses foo; }
container two { uses foo; }then there is exactly one yang.Node which represents the a defined in the foo grouping, but there are two entries for /one/a and /two/a.
Prior to this change we always de-duplicated these enumerated types, so that there is an enumeration that represents /foo/a -- this makes sense in OpenConfig for example, where we might have /foo/state/an-enum and /foo/config/an-enum, and we don't really want to refer to those with different types. In other schemas, like the example above, de-duplicating these means that we would choose one type for /one/a and /two/a. Since these types are named after the schema path, then this meant that we had very confusing naming for the enumerations.
There was a problem hiding this comment.
Thanks! I think I got it now. The most time-consuming parts were
- Understanding the tests.
- Finally deciding that some inputs are not worth testing since they're just pass-through parameters to lower-level functions.
|
Thanks for looking at this -- let me know if you'd prefer for me to look at the missing coverage. |
There was a problem hiding this comment.
LGTM, but note I made a change earlier to resolveEnumName to handle a corner case (812b889)
Note that the comment I wrote in that commit was so confusing it took me another 40 minutes to understand it today. I just updated the comment to make it clearer.
| s.uniqueEnumeratedLeafNames[identifierPath] = uniqueName | ||
|
|
||
| if skipDedup { | ||
| // If using compression and duplicating, then we add |
There was a problem hiding this comment.
Clarified this wording here "and duplicating" seemed odd to me -- so I expanded it out to say that we're explicitly outputting a type per leaf.
| }, | ||
| }, | ||
| }, { | ||
| name: "two enums with deduplication disabled, and where duplication occurs for both compressed and decompressed", |
There was a problem hiding this comment.
(comment) This is an interesting corner-case, since in a compressed schema, this enum type would never actually be referenced. For example:
grouping foo-config {
leaf a { type enumeration { enum ONE; } }
}
container foo {
container config { uses foo-config; }
contianer state { uses foo-state; }
}In this case, compression will remove /foo/state/a but /foo/state/a will still create Foo_State_A in the output code.
I don't think we need to worry about this - because we're explicitly doing what the user asked, it just doesn't really make a huge amount of sense :-)
|
Merging this PR based on discussion with @wenovus and another look. Thanks for the collaboration here Wen. |
