openconfig / goyang

YANG parser and compiler to produce Go language objects
Apache License 2.0
220 stars 84 forks source link

Adding Extras to JSON - fixes #211 #212

Closed SeanCondon closed 3 years ago

SeanCondon commented 3 years ago

In this Patch I'm doing 2 things 1) Removing the exclusion of Extras from JSON, and excluding down the line the Parent and Source of Extra elements

2) Improving how items are added to the Extras slice. When the item is a Slice add its elements individually.

I can break this up in to 2 separate patches if necessary

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.06%) to 81.989% when pulling bd1680b601147a103263aeb28f2d36f304471898 on SeanCondon:scExtrasInJson into 66f2a97d4b288f12cc3862c382dcd919b9bac5d7 on openconfig:master.

SeanCondon commented 3 years ago

Hi @wenovus - I hope you will get a chance to look at this in the near future - best regards, Sean

SeanCondon commented 3 years ago

One concern here - should we be making Extras be something that is a public API that can be relied upon? It seems like the unimplemented keywords that we have specified should probably just be implemented as a use-case comes up for them.

Is there a specific case that this is being implemented for?

It was really the must statement I was interested in. So yes there could be a direct object containing the must statments.

As a YANG author though I'd like to see the statement I've written to be in the JSON somehow. This is why I thought exposing the Extras has value.

wenovus commented 3 years ago

I agree that if we introduced this change, that dependent users would see an incompatible change if we later add support for some of the statements.

As a compromise we could marshal the field into a JSON field with a name that is unambiguously unstable.

Extra map[string][]interface{} `json:"unsupported-unstable,omitempty"` 
SeanCondon commented 3 years ago
unsupported-unstable

Thanks @wenovus and @robshakir for reviewing. Re the unsupported-unstable tag - could I suggest extra-unsupported-unstable, so that it's clear it represents the extra section.

robshakir commented 3 years ago

I'm good with this as a solution - I think there is precedent for it in the "unknown fields" parsing in protobuf libraries. I suggest that we can just call it extras-unstable so that it is clear that this is what it is being used for.

In the longer-term we should investigate adding these fields as things that are supported as first class fields of Entry structs.

SeanCondon commented 3 years ago

Thanks again @robshakir for looking at this. I was reluctant to go with the plural extras- since the Go type is Extra, so I made it extra-unstable (singular).

SeanCondon commented 3 years ago

LGTM. @robshakir, does this look good to you?

Thanks @wenovus It would be great to get this merged if @robshakir is OK with it

robshakir commented 3 years ago

Thanks for the detailed co-ordination here, and thanks @SeanCondon for being so open to addressing our comments :-)

@wenovus - LGTM, please do merge.