Uh oh!
There was an error while loading. Please reload this page.
Switch dyn.Mapping implementation to use map underneath - #2737
Conversation
pietern
left a comment
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?
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| func (v Value) DropKeyLocations() Value { |
There was a problem hiding this comment.
Please include a comment to explain why this function exists.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
| 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
This PR has not received an update in a while. If you want to keep this PR open, please leave a comment below or push a new commit and auto-close will be canceled. |
## 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.