Conversation
thaJeztah
force-pushed
the
bump_yamlv3
branch
2 times, most recently
from
047a479 to
6e68e64
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5752 +/- ##
==========================================
- Coverage 59.51% 59.47% -0.05%
==========================================
Files 346 346
Lines 29376 29367 -9
==========================================
- Hits 17483 17465 -18
- Misses 10923 10929 +6
- Partials 970 973 +3 |
thaJeztah
force-pushed
the
bump_yamlv3
branch
from
6e68e64 to
2167436
Compare
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
bump_yamlv3
branch
from
2167436 to
58bf0f1
Compare
thaJeztah
added
status/2-code-review
area/stack
kind/refactor
|
overall LGTM.. |
| return nil, err | ||
| } | ||
| cfgMap, ok := cfg.(map[any]any) | ||
| _, ok := cfg.(map[string]any) |
There was a problem hiding this comment.
it seems strange that many tests in the previous PR were failing with top-level object must be a mapping because the tests haven't changed and the compose file used at the time seem ok to me. Even the versions of the module are the same
So this was the reason; YAML v2 returned a map[any]any whereas v3 always returns a map[string]any.
Honestly, I half-expected map[string]any to ALSO be a map[any]any (because string is also an any ? But that's not how Go asserts these.
There was a problem hiding this comment.
So actually, to be more correct; it returns a map[string]interface{} but Go is happy considering interface{} == any
| func convertToStringKeysRecursive(value any, keyPrefix string) (any, error) { | ||
| if mapping, ok := value.(map[any]any); ok { | ||
| if mapping, ok := value.(map[string]any); ok { | ||
| dict := make(map[string]any) | ||
| for key, entry := range mapping { | ||
| str, ok := key.(string) | ||
| if !ok { | ||
| return nil, formatInvalidKeyError(keyPrefix, key) | ||
| } |
There was a problem hiding this comment.
But while it makes sense too use map[string]any (i.e., YAML keys would be a string, always, even it it looks like an int or a weird string like {object}:, that also means that we can't produce custom errors for those ("you used an integer as key, did you mean something else?").
| func formatInvalidKeyError(keyPrefix string, key any) error { | ||
| var location string | ||
| if keyPrefix == "" { | ||
| location = "at top level" | ||
| } else { | ||
| location = "in " + keyPrefix | ||
| } | ||
| return errors.Errorf("non-string key %s: %#v", location, key) | ||
| } |
There was a problem hiding this comment.
So these errors could no longer work, unless we would reverse the direction ("string" -> "but is it an integer?")
| func TestNonStringKeys(t *testing.T) { | ||
| // FIXME(thaJeztah): opkg.in/yaml.v3, which always unmarshals to a map[string]any, so we cannot produce a customized error for invalid types. | ||
| t.Skip("not supported by gopkg.in/yaml.v3, which always unmarshals to a map[string]any") | ||
| _, err := loadYAML(` |
There was a problem hiding this comment.
So, I kept the test for our custom errors (this test runs with badly formed YAML files to test the custom errors).
But mostly as a "reminder" in case someone would like to bite their teeth into implementing something like that.
(but in all honesty, we can probably just remove the test and call it a day; just write proper YAML)
4 participants
This was attempted some time ago in #5629, but I don't recall what the issue was causing problems, so let's try and see what CI says
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)