Support presence field natively,implement refine statements, choice/case shorthand fixes#287
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
alexandernorth
changed the title
…es without cases were missing path elements
…plicitly there. Updated Choice struct to support yang 1.1, and the processing is based on the rules for 1.1.
…checks resulting in duplicate node errors (due to merge function)
* Added default refine for leaf * Changed defaults to list to support refine of leaf-list * fixed comments and renamed refine tests * added presence to refine * Added choice type for refine defaults and proper type checking for container in presence'
Co-authored-by: Fabian Schulz <fabian.schulz1@swisscom.com>
alexandernorth
force-pushed
the
feature/refine-and-choicecases
branch
from
d1ec13f to
b2a76ed
Compare
robshakir
left a comment
robshakir
left a comment
There was a problem hiding this comment.
Thanks for the contribution, and extending goyang's functionality -- apologies for the delay in getting to review this contribution.
Please take a look at the comments.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
| // Copyright 2025 Swisscom (Schweiz) AG |
There was a problem hiding this comment.
This reads like this whole file is (c) Swisscom, can you please adjust this to follow the first line and make it "additions copyright"? Alternatively, we can make this "(c) the OpenConfig contributors" and create a CONTRIBUTORS file.
| // grouping has a leafref that references outside the group. | ||
| e = ToEntry(g).dup() | ||
|
|
||
| for _, refine := range s.Refine { |
There was a problem hiding this comment.
Given that this is a relatively long block -- can we split it out into its own function that we can also test individually?
|
|
||
| // o A leaf, anydata, anyxml, or choice node may get a different | ||
| // "mandatory" statement. | ||
|
|
There was a problem hiding this comment.
(nit) remove blank like to match other cases.
| } | ||
| case "choice": | ||
| for _, a := range fv.Interface().([]*Choice) { | ||
| // https://datatracker.ietf.org/doc/html/rfc7950#section-7.9.2 |
There was a problem hiding this comment.
Again, can we pull this logic out into a separate function that we can test?
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
| // Copyright 2025 Swisscom (Schweiz) AG |
There was a problem hiding this comment.
Same comment here re: copyright statements as the other file. (No problem noting the copyright, but we should just make it compliant with usual styles.)
| Extensions []*Statement `yang:"Ext"` | ||
|
|
||
| Default *Value `yang:"default"` | ||
| Defaults []*Value `yang:"default"` |
There was a problem hiding this comment.
This is a backwards incompatible change, that needs to be handled by downstream consumers too.
We have had some discussion before on this: #51
The conclusion was that we would have a Default and Defaults field, and this would allow handling of these cases in a backwards compatible way. Can we do this here so that we don't break the existing users?
3 participants
This PR implements the logic to process parsed refine statements according to the Yang 1.1 RFC.
It also implements a fix for choice cases, where the case statement is not present (allowed as per RFC). It does this by inserting the case automatically, such that a choice always has a case and no shorthand is used. This is important as paths are defined as if there is always a case node in the tree, even if it is missing in the schema.
It also implements native presence fields in the
Entryas discussed in #226 and #286