Fix and test merges with MapSlice

[hjdr4]

This patch adds tests and working code when decoding merges into MapSlice.

[lox]

Hi @hjdr4! Are you able to resolve the conflicts?

Is this a fix for #91?

[hjdr4]

Hello,

I have resolved the conflicts.
I have tested the snippet in #91 and it outputs what is expected.
It also fixes #184.

[lox]

Thanks! Could you explain what the issue was? Might this change effect anything else?

[hjdr4]

MapSlice allows to keep ordering when decoding YAML and this is sometimes required.
The problem is already described in the referenced issues : it does not work correctly at the moment.

So first I wrote a test that shows it's broken (using already defined test data) and code that does what is expected.

I didn't touch the others tests and they are all passing so I guess this won't break anything.

[lox]

@rogpeppe any thoughts on this one?

[niemeyer]

@lox Any thoughts from you on it? :)

[lox]

I'm still getting my head around the impact of the addition of the recursive merge, but it seems to be a clean patch with tests that addresses these specific issues.

[lox]

I'm just in the process of testing it.

[lox]

👍 from me.

[niemeyer]

There are assumptions about typing in the general logic of merge which may not be fulfilled by the structure, in the sense that slices won't necessarily have that type simply because their kind is a slice.

All, I appreciate your help with this, but I'd like to put a tombstone on this issue. This is adding logic to general code paths in v2 which may break down even when not using MapSlice, and I don't want to look back into that. MapSlice will die in v3, and be replaced by a much more flexible intermediate representation. I want to focus on that instead, and anyone that really wants such MapSlices and merges, which is not quite common, can move forward and migrate into it.

Again, thanks for your time here.

[lox]

No problems @niemeyer, looking forward to v3!

[hjdr4]

Those assumptions are already done https://github.com/go-yaml/yaml/blob/v2/decode.go#L585 and https://github.com/go-yaml/yaml/blob/v2/decode.go#L653.

If you want a stronger test, it's just a matter of changing the merge() test to

out.Kind() == reflect.Slice && out.Type().Elem() == mapItemType 

[niemeyer]

@hjdr4 The logic in merge() itself is general, and called from multiple places. Nothing in there checks the type of the slice.

To be very clear, though, I'm not blaming you and this is a normal issue that we could easily sort out in reviews. But given yaml.v3 is close by and all of that will be dead, I'd indeed prefer to just not touch the logic that is there for several years and instead support any use cases on v3.