feat: format v3 #249
feat: format v3 #249
Conversation
Format v3 uses a map instead of a list for `essential` and deprecates the field `v3-essential` which is only used during the transition from v2 to v3.
| err := dec.Decode(&yamlPkg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot parse package %q slice definitions: %v", pkgName, err) | ||
| if format == "v1" || format == "v2" { |
There was a problem hiding this comment.
[Note for reviewer]: This is the simplest way I found to support all formats without having to duplicate all the logic. We could, for example, use interfaces and have two implementations one for v1/v2 and one for v3, but I didn't like the extra layer of indirection.
There was a problem hiding this comment.
It's quite surprising that the implementation when we support the three formats in a compatible fashion seems cleaner than the one where we're breaking compatibility with the past to clean it up. Even more when the new format is exactly the same as the old format under a different field name. What am I missing?
There was a problem hiding this comment.
I don't fully understand your comment, what do you mean by?
Even more when the new format is exactly the same as the old format under a different field name.
That is true but we need to also support v2 which has a different set of fields (i.e. different type for essential and v3-essential is used).
EDIT: After re-reading it I understood the comment. This is about how we marshal back into yaml (presumably).
| Essential map[string]*yamlEssential `yaml:"essential,omitempty"` | ||
| // It is only used to present an error message to the user if the field | ||
| // is used in v3. | ||
| V3Essential map[string]*yamlEssential `yaml:"v3-essential,omitempty"` |
There was a problem hiding this comment.
[Note for reviewer]: We can remove this field completely. I do think the UX is better if we tell the user the field they had been using is now deprecated, this can act as a hint that they need to look at the documentation to see what changed.
There was a problem hiding this comment.
Sure, but it may have "any" as the type instead, as we don't care about what it holds, and it shouldn't be touched if yamlEssential changes.
|
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
On a first pass, as detailed below it's surprising and unfortunate that this PR seems to make things significantly worse almost everywhere it touches. This was supposed to be a cleanup PR ideally.
| Essential map[string]*yamlEssential `yaml:"essential,omitempty"` | ||
| // It is only used to present an error message to the user if the field | ||
| // is used in v3. | ||
| V3Essential map[string]*yamlEssential `yaml:"v3-essential,omitempty"` |
There was a problem hiding this comment.
Sure, but it may have "any" as the type instead, as we don't care about what it holds, and it shouldn't be touched if yamlEssential changes.
| err := dec.Decode(&yamlPkg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot parse package %q slice definitions: %v", pkgName, err) | ||
| if format == "v1" || format == "v2" { |
There was a problem hiding this comment.
It's quite surprising that the implementation when we support the three formats in a compatible fashion seems cleaner than the one where we're breaking compatibility with the past to clean it up. Even more when the new format is exactly the same as the old format under a different field name. What am I missing?
| Archive string `yaml:"archive,omitempty"` | ||
| Slices map[string]v1v2YAMLSlice `yaml:"slices,omitempty"` | ||
| Essential []string `yaml:"essential,omitempty"` | ||
| V3Essential map[string]*yamlEssential `yaml:"v3-essential,omitempty"` |
There was a problem hiding this comment.
Similar here.. I don't understand why we're introducing significant duplication for something that before was easier, now that we can break compatibility. It's almost like the hack was cleaner than the straightforward version.
There was a problem hiding this comment.
What do you mean by breaking compatibility? The new version of Chisel should be able to read v1, v2 and v3. I am probably missing something about what you meant in the comments here. Maybe you are talking about the shape of the code and you had something specific in mind.
There is another alternative, I can have only one type definition and use any / yaml.Node as the type for Essential both in package and in slice and then do the parsing manually depending on the format version.
| return slice, nil | ||
| } | ||
|
|
||
| type v1v2YAMLSlice struct { |
There was a problem hiding this comment.
If/when we do need such types and functions, it's better to spell this as yamlSliceV2 and yamlSliceV3, instead of v1v2YAMLSlice and yamlSlice. Same for functions, etc.
2 participants
Format v3 uses a map instead of a list for
essentialand deprecates the fieldv3-essentialwhich is only used during the transition from v2 to v3.