freeconf / yang

Standards-based management for Golang microservices
Apache License 2.0
38 stars 15 forks source link

Issue-77 Add support for shorhand/implicit case in augment statement … #90

Closed davidmat50 closed 11 months ago

davidmat50 commented 1 year ago

Add support for shorhand/implicit case in augment statement when augment target is a choice

dhubler commented 1 year ago

https://datatracker.ietf.org/doc/html/rfc7950#section-7.9.2

As a shorthand, the "case" statement can be omitted if the branch contains a single "anydata", "anyxml", "choice", "container", "leaf", "list", or "leaf-list" statement. In this case, the case node still exists in the schema tree, and its identifier is the same as the identifier of the child node. Schema node identifiers (Section 6.5) MUST always explicitly include case node identifiers.

So it would be only .../y/q/q according to spec if case statement was missing.

davidmat50 commented 12 months ago

Infact the fix i have provided in this pull request is for alllowing a shorhand case ( or implicit case) under a choice if the augment target was a choice. It was not about the validity of a augment target string which represents a case.

As i understand https://datatracker.ietf.org/doc/html/rfc7950#section-7.9.2, in relation with our context, it may apply as augment target should be explicitly have additional case node identifier (if the target was a case) (.../y/q/q) , since augment targets are represented as Schema node identifiers ( as per section-6.5 and section-14).

i hope you are also in sync with my above undertanding, Do you have an any suggestion/fix on how we can restrict/reject (.../y/q) for augment target if the target is a case, and only allow .../y/q/q .

dhubler commented 11 months ago

I see now, your solution looks good. It can be simplified a bit

_, targetIsChoice := target.(*Choice)

and then 2nd part:

if _, isCase := d.(*ChoiceCase); !isCase && targetIsChoice {
... rest is the same as you have it

Normally I would request a unit test as well but I'll add one after but i'll add one to TestParseSamples after this gets into master

davidmat50 commented 11 months ago

Thank you for the response and the suggestions. I have done suggested changes and pushed. Please check.