Switch dyn.Mapping implementation to use map underneath #2737
Conversation
There was a problem hiding this comment.
This looks reasonable.
While this removes the ability to iterate in definition order, that can always be reconstructed using location information if it turns out we need it.
Can you link to #1301 from the PR summary as well?
What is the significance of []dyn.Location{} vs nil? Does this trip up equality checks?
| @@ -214,3 +232,26 @@ func (v Value) eq(w Value) bool { | |||
| return v.v == w.v | |||
| } | |||
| } | |||
|
|
|||
| func (v Value) DropKeyLocations() Value { | |||
There was a problem hiding this comment.
Please include a comment to explain why this function exists.
| @@ -25,6 +25,7 @@ func loadExample(t *testing.T, file string) dyn.Value { | |||
| func TestYAMLSpecExample_2_1(t *testing.T) { | |||
| file := "testdata/spec_example_2.1.yml" | |||
| self := loadExample(t, file) | |||
| self = self.DropKeyLocations() | |||
There was a problem hiding this comment.
You can probably fold this call into the loadExample function.
| @@ -801,6 +822,7 @@ func TestYAMLSpecExample_2_27(t *testing.T) { | |||
| func TestYAMLSpecExample_2_28(t *testing.T) { | |||
| file := "testdata/spec_example_2.28.yml" | |||
| self := loadExample(t, file) | |||
| self = self.DropKeyLocations() | |||
There was a problem hiding this comment.
Could you include one or two tests that confirm that map keys actually have a location?
Requires a bit more work on constructing the expected value, but worth it to include it. Can be a follow up.
| Value: m.data[k], | ||
| }) | ||
| } | ||
| return pairs |
There was a problem hiding this comment.
Curious: the iteration is identical for Pairs/Keys/Values. Might be a nice use case for https://go.dev/blog/range-functions#range-over-some-function-types.
## Why - This starts to matter once we replace dynassert with regular assert #2737 - We agreed before to use nils consistently instead of empty slices.
Changes
Follow up to #1301
Why
Tests
Existing tests.