Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change internal JSON Generation for OrderedMaps to be backwards-compatible #894

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Jul 21, 2023

I did not consider JSON internal format for OrderedMaps and was generating it the same way as RFC7951.

However, Internal JSON format for YANG lists uses a JSON object/map type instead of JSON list to represent a map.

Doing this destroys the ordering and defeats the purpose of ordered-by user lists but it's more important to keep backwards-compatibility for this legacy JSON format.

Also refactor code.

@wenovus wenovus requested review from robshakir and DanG100 July 21, 2023 02:13
@coveralls
Copy link

coveralls commented Jul 21, 2023

Coverage Status

coverage: 89.649% (-0.01%) from 89.663% when pulling 208e762 on ordered-map-json-fix into db25a8c on master.

@DanG100
Copy link
Contributor

DanG100 commented Jul 24, 2023

should this behavior be controlled by a flag?

@wenovus
Copy link
Collaborator Author

wenovus commented Jul 24, 2023

should this behavior be controlled by a flag?

I think this is not necessary because it preserves existing behaviour and AFAIK there is no user need for internal JSON format desiring the ordering property to be preserved. If there is a requirement then we can take a look.

Base automatically changed from fix-orderedmap-buildemptytree to master July 24, 2023 20:08
@DanG100
Copy link
Contributor

DanG100 commented Jul 24, 2023

should this behavior be controlled by a flag?

I think this is not necessary because it preserves existing behaviour and AFAIK there is no user need for internal JSON format desiring the ordering property to be preserved. If there is a requirement then we can take a look.

Should there be a TODO to remove this internal JSON? especially if it's not generated "incorrect" info

@wenovus
Copy link
Collaborator Author

wenovus commented Jul 24, 2023

should this behavior be controlled by a flag?

I think this is not necessary because it preserves existing behaviour and AFAIK there is no user need for internal JSON format desiring the ordering property to be preserved. If there is a requirement then we can take a look.

Should there be a TODO to remove this internal JSON? especially if it's not generated "incorrect" info

I think this might be reasonable. @robshakir do you think it's reasonable to mark as "deprecated" the ygot.Internal JSONFormat? The code comments mention that it is the format used by pyangbind so I'm not positive whether it should be marked as such, but it seems otherwise RFC7951 is becoming the default option rather than the internal format:

 const (
        // Internal is the custom JSON format that is output by the validation library, and
-       // by pyangbind. It is loosely specified - but is the default used by generator developers.
+       // by pyangbind. It is loosely specified - but is used by some generator developers.
+       //
+       // Deprecated: Prefer RFC7951 to preserve list ordering, among other
+       // advantages.
        Internal JSONFormat = iota

@wenovus
Copy link
Collaborator Author

wenovus commented Jul 24, 2023

I'll merge this now, but depending on Rob's comments I might add a deprecation notice.

@wenovus wenovus merged commit b7de07a into master Jul 24, 2023
@wenovus wenovus deleted the ordered-map-json-fix branch July 24, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants