Retain Extra fields over Uses,Augments #244
Conversation
pkg/yang/entry.go
Outdated
| if v.Extra[lk] == nil { | ||
| v.Extra[lk] = lv | ||
| continue | ||
| } |
There was a problem hiding this comment.
This if block can be omitted without impacting functionality.
| v.Extra[lk] = lv | ||
| continue | ||
| } | ||
| v.Extra[lk] = append(v.Extra[lk], oe.Extra[lk]...) |
There was a problem hiding this comment.
Can you add a test in TestIfFeature in entry_test.go?
| v.Extra[lk] = lv | ||
| continue | ||
| } | ||
| v.Extra[lk] = append(v.Extra[lk], oe.Extra[lk]...) |
There was a problem hiding this comment.
(optional) Note that currently only augments in uses statements are going to be picked up. Raw augments won't be. I'd also suggest adding this statement outside of the for loop as well and fixing the relevant test.
There was a problem hiding this comment.
Can you please give an example where this would occur?
There was a problem hiding this comment.
For what I see, augment defined in uses are ignored, regardless of if-feature field - could we open a new issue, approve this PR, and treat that corner case separately? What do you reckon @wenovus ?
There was a problem hiding this comment.
Actually my comment here was misled by my naive thought that if-features should be applied to the target node where they should only be applied to the nodes within the uses or augment, which is exactly what your PR does. This LGTM.
| v.Extra[lk] = lv | ||
| continue | ||
| } | ||
| v.Extra[lk] = append(v.Extra[lk], oe.Extra[lk]...) |
There was a problem hiding this comment.
Actually my comment here was misled by my naive thought that if-features should be applied to the target node where they should only be applied to the nodes within the uses or augment, which is exactly what your PR does. This LGTM.
| @@ -2269,6 +2276,15 @@ func TestIfFeature(t *testing.T) { | |||
| inIfFeatures: ms.Modules["if-feature"].Uses[0].IfFeature, | |||
| wantIfFeatures: []string{"ft-uses"}, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
If you can add a comment above this test block, something like "// Testing that if-features are merged correctly" I think that would help make this more readable.
|
Thanks! |
Hi,
In a YANG model, uses or augments can be used to extend modules. It's possible to define under those nodes an IfFeature attribute. But when turning those into an Entry (yang.ToEntry), IfFeature field disappears. It should become an attribute located under Extras, but when using Uses, it simply isn't copied hence some information is lost.
This PR adds support for moving all the Extra fields defined in Uses or Merge to the augment element.