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.
|
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.
internal/setup/yaml.go
Outdated
| }, | ||
| } | ||
|
|
||
| func parseSliceEssential(yamlPkg *yamlPackage, yamlSlice *yamlSlice, slice *Slice) error { |
There was a problem hiding this comment.
This refactoring makes it hard to track what changed compared to the previous implementation. Could we do the extraction in a dedicated function in another PR?
There was a problem hiding this comment.
I thought about that but it is a bit overkill because what happens if we end up changing the approach, then we merged a PR for a refactor that wasn't actually fully needed. As I told you offline, this function needs a bit of love to make it more readable but, if done properly, I think it is easy to see the logic just moved. I will make the changes and we can discuss.
There was a problem hiding this comment.
I think with the latest changes it is easier to see that the diff is the same.
|
As discussed with @niemeyer this week and the previous one, I have changed the PR. The current approach does not need two separate definitions of the same type and instead just changes how the conflicting field is parsed (in > v3 as a map, in v1, v2 as a list). This solution is not as strict as the previous one in regards to The complexity that still exists in the current approach is mainly because we need to produce good error messages and discern between all combinations of format + fields. For the record, Paul and me spent even more time this week thinking about the problem and we also tried:
|
| // 'v3-essential' to the shape expected by the v3 format. | ||
| // skip is set to true when an accurate translation of the test is not | ||
| // possible, for example having duplicates in the list. | ||
| func oldEssentialToV3(c *C, input []byte) (out string, skip bool) { |
There was a problem hiding this comment.
[nitpick]: This is a clever solution to avoid duplicating the test cases but given the complexity I find it a bit hard to know what the final input to the test is.
I also wonder if we really need to rerun this many tests when many of them are not testing the essential resolution logic. I guess we are better safe than sorry but I wonder how hard that will be to maintain, especially when adding new tests.
3 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.