Conversation
robshakir
left a comment
robshakir
left a comment
There was a problem hiding this comment.
Thanks for the change -- generally this LGTM -- please take a look at the comments and then we can get this merged.
Apologies for the delay reviewing the code.
pkg/yang/entry.go
Outdated
| return e | ||
| } | ||
|
|
||
| type YangVersion string |
There was a problem hiding this comment.
Can we add a docstring comment please?
| Reference *Value `yang:"reference" json:"-"` | ||
| Status *Value `yang:"status" json:"-"` | ||
| When *Value `yang:"when" json:",omitempty"` | ||
| Augment []*Augment `yang:"augment" json:",omitempty"` |
There was a problem hiding this comment.
Should we create a different field here, since this means that this change is backwards-incompatible for existing API users. There was some discussion historically about making such changes for YANG 1.1.
If we don't do this, then we need to mark this as goyang v2. I'd prefer to see whether we can group up other API changes that would be needed to fully support y1.1 into such a release.
There was a problem hiding this comment.
I'm unsure what exactly you want me to do here.
There was a problem hiding this comment.
I don't support changing Augment from *Augment to []*Augment since this is backwards incompatible. This means that we're going to need to all downstream clients to change.
I suggest this is implemented using a new field Augments which has the slice. If it's not, then I don't think we should merge this without discussing other cardinality changes that are needed to support YANG 1.1 in a goyang v2.
pkg/yang/entry_test.go
Outdated
| for n, m := range tt.inModules { | ||
| if err := ms.Parse(m, n); err != nil { | ||
| errs = append(errs, err) | ||
|
|
pkg/yang/entry_test.go
Outdated
| for _, pe := range p { | ||
| y, ok := x.Dir[pe] | ||
| if !ok { | ||
| t.Fatalf("expected module %s to contain path %s", m.Name, strings.Join(p, "/")) |
There was a problem hiding this comment.
Can we add more debugging output here -- e.g., "expected module %s to contain path %s, could not find element %s in parent, got children: %v"
pkg/yang/entry_test.go
Outdated
|
|
||
| var m *Entry | ||
| m, errs = ms.GetModule("mod-a") | ||
| x := m |
There was a problem hiding this comment.
Suggest to make these variable names a little less pithy -- curElem or something would be easier for a reader to understand.
| // grouping has a leafref that references outside the group. | ||
| e = ToEntry(g).dup() | ||
|
|
||
| switch determineYangVersion(g) { |
There was a problem hiding this comment.
We haven't added this kind of error checking elsewhere -- I wonder whether we should try and include the parsed YANG version somewhere in the output entry. WDYT?
There was a problem hiding this comment.
if we want to be strict then thats probably a good idea.
pkg/yang/entry.go
Outdated
| type YangVersion string | ||
|
|
||
| const ( | ||
| YangVersion10 YangVersion = "1.0" |
There was a problem hiding this comment.
s/YangVersion10/YANGVersion10/ per usual Go style guidelines.
|
This would be fantastic to have merged in. |
3 participants
Allow for the augmentation of groupings.