Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #756 +/- ##
==========================================
- Coverage 77.97% 77.94% -0.03%
==========================================
Files 22 22
Lines 8108 8113 +5
==========================================
+ Hits 6322 6324 +2
- Misses 1370 1372 +2
- Partials 416 417 +1 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Adds support for non-string map keys in the YAML encoder and enhances test coverage for various key types
- Uses
encodeValueto generate aMapKeyNodefor non-string keys, falling back tofmt.Sprint - Updates
encodeMaplogic inencode.goto handle dynamic key types - Adds unit tests for
map[int]string,map[float64]string,map[bool]string,map[any]stringwith array and struct keys
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| encode.go | Introduced logic to attempt encodeValue on each map key and fallback to string encoding |
| encode_test.go | Added tests for integer, float, boolean, array, and struct keys |
Comments suppressed due to low confidence (1)
encode.go:716
- [nitpick] The variable name
knis ambiguous. Consider renaming it to something more descriptive likekeyNodeValueto improve readability.
kn, err := e.encodeValue(ctx, reflect.ValueOf(key), column)
|
@shuheiktgw In this PR, only the encoding side is modified, but isn't it necessary to address the decoding side as well ? |
|
@goccy Decoding seems to be working fine on my end. Is there any specific part you're concerned about? 👀 func Test1(t *testing.T) {
content := `
1: one
2: two
`
var m map[int]string
if err := yaml.Unmarshal([]byte(content), &m); err != nil {
t.Fatal(err)
}
if expected := map[int]string{1: "one", 2: "two"}; !reflect.DeepEqual(m, expected) {
t.Fatalf("got %#v, want %#v", m, expected)
}
} |
|
@shuheiktgw It seems that cases where content := `
"{a: b}": one
`
type T struct {
A string `yaml:"a"`
}
var m map[T]string
if err := yaml.Unmarshal([]byte(content), &m); err != nil {
t.Fatal(err)
}Ah, However, I just realized that the encoding side is also incorrect. When a slice or struct is used as a key, the output is no longer a valid YAML string. Presumably, when values with structures like slice or struct are specified as keys, the e.g.) If it is difficult to address this in this PR, I think it would be fine to limit it to int, float, string, and bool. |
|
@goccy Thanks for the information! Wow, I didn’t know about the ? keyword... I took a quick look at the code, and it seems like supporting encoding/decoding of complex map keys (like maps or sequences) wouldn’t be straightforward. How about we:
What do you think? |
|
I think the encoding of struct values has always been incorrect. I've included the string-encoding in the original PR to be "backwards-compatible" with the old non-compliant way. Since the encoding was always non-compliant, it may be a reasonable decision to ignore and break it anyway. |
| encoded = anchorNode | ||
| } | ||
|
|
||
| kn, err := e.encodeValue(ctx, reflect.ValueOf(key), column) |
There was a problem hiding this comment.
Variable k already holds reflect.ValueOf(key).
|
When keys are numeric, how should they be sorted? |
|
LGTM 👍 |
* fix map keys misencoding to string * Add unit tests * Add tests for decoder with unconventional map keys --------- Co-authored-by: Zsolt Herczeg <herczegzsolt@herczegzsolt.hu>
* fix map keys misencoding to string * Add unit tests * Add tests for decoder with unconventional map keys --------- Co-authored-by: Zsolt Herczeg <herczegzsolt@herczegzsolt.hu>
Closes #680 & #331
I took over #681, resolved conflicts and added some unit tests. The original implementation looks very good to me.