search

openconfig / goyang

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

identities that have a base identity are not sorted Fixes #234 #235

Closed SeanCondon closed 2 years ago

SeanCondon commented 2 years ago

See issue #234

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.01%) to 84.021% when pulling 01c4f4c0044e4b3bbfc025fc6592da69b7a4c7ca on SeanCondon:scIssue234 into ee6fd6d28bab263ec1bb0307aafef67e994899cd on openconfig:master.

SeanCondon commented 2 years ago

Hi @wenovus - do you mind taking a look at this one please?

SeanCondon commented 2 years ago

Now this is technically a backwards-incompatible change. Only the order is affected, but I'm wary of just introducing this change now that we're at version 1.0.0.

Sorry about the very late comment -- is it inconvenient for you to sort them downstream on your own instead of depending on goyang to sort it?

Hi @wenovus - I can't really see how it is backward in-compatible - there used to be 2 outputs of enums in generated.go:

  1. the generated go code - the enums were always ordered alphabetically and now will not change
  2. the JSON schema - the enums were never ordered, and would change order every time you ran it - so every time you ran generate it was backward in-compatible. Now with this patch running generate the enums are ordered alphabetically it will be another variation of this order - just that it will be consistent with the previous runs order and with the generated code (1)

The only way I think it can be considered a problem is that someone was depending on the order to be different every time (while this was likely, there was no guarantee of this)

wenovus commented 2 years ago

Ah I see, this is actually a bug fix that makes the code deterministic. Sorry I should've read the issue you opened. Thanks for the clarification!

SeanCondon commented 2 years ago

@wenovus Thanks for merging. BTW - would it be possible to release this? Is that a manual process?