Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapSlice does not support anchors/merges #184

Closed
Mange opened this issue Jul 7, 2016 · 13 comments
Closed

MapSlice does not support anchors/merges #184

Mange opened this issue Jul 7, 2016 · 13 comments

Comments

@Mange
Copy link

Mange commented Jul 7, 2016

Hi! I seem to have found a bug in MapSlice:

import (
    "gopkg.in/yaml.v2"
    "reflect"
    "testing"
)

func TestMapMerging(t *testing.T) {
    data := `---
world: &world
  greeting: Hello
earth:
  << : *world`

    config := map[string]interface{}{}
    err := yaml.Unmarshal([]byte(data), &config)
    if err != nil {
        t.Errorf("Failed to parse\n%v\n\ninto a %T: %v", data, config, err)
    }

    earth := config["earth"]
    expected := map[interface{}]interface{}{
        "greeting": "Hello",
    }

    if !reflect.DeepEqual(earth, expected) {
        t.Errorf("Merging does not work. Expected %v (%T), got %v (%T)", expected, expected, earth, earth)
    }
}

func TestMapSliceMerging(t *testing.T) {
    data := `---
world: &world
  greeting: Hello
earth:
  << : *world`

    config := make(yaml.MapSlice, 0)
    err := yaml.Unmarshal([]byte(data), &config)
    if err != nil {
        t.Errorf("Failed to parse\n%v\n\ninto a %T: %v", data, config, err)
    }

    earth := config[1].Value
    expected := yaml.MapSlice{{Key: "greeting", Value: "Hello"}}

    if !reflect.DeepEqual(earth, expected) {
        t.Errorf("Merging does not work. Expected %v (%T), got %v (%T)", expected, expected, earth, earth)
    }
}

Output:

--- FAIL: TestMapSliceMerging (0.00s)
    foo_test.go:264: Merging does not work. Expected [{greeting Hello}] (yaml.MapSlice), got [] (yaml.MapSlice)

Note how the test with the normal map[string]interface{} works properly. I'm still a beginner in Go, so I'm not sure where to start to fix this one. I have a really hard time understanding the decoding code, but a small pointer in the right direction might help me find some way of fixing this.

@bradrydzewski
Copy link

@niemeyer if we send a patch for this what is the process for getting it merged? Is the v2 branch frozen or are you accepting changes? If not, is there a timeline for v3?

cc @donny-dont

@vinzenz
Copy link

vinzenz commented Nov 2, 2016

@bradrydzewski could you please try with #220
This issue might be related to #91

vinzenz pushed a commit to vinzenz/yaml that referenced this issue Nov 3, 2016
Signed-off-by: Vinzenz Feenstra <evilissmo@redhat.com>
vinzenz pushed a commit to vinzenz/yaml that referenced this issue Nov 3, 2016
Signed-off-by: Vinzenz Feenstra <evilissmo@redhat.com>
@vinzenz
Copy link

vinzenz commented Nov 3, 2016

@bradrydzewski @Mange Its fixed by #220 I took the liberty to add the above test cases to the tests

@Mange
Copy link
Author

Mange commented Nov 3, 2016

Thank you!

@bradrydzewski
Copy link

@vinzenz yep, that did the trick. I'm vendoring your patch at the moment, but hopefully the pull request can get merged in the near future. Thanks!

vinzenz pushed a commit to vinzenz/yaml that referenced this issue Mar 27, 2017
Signed-off-by: Vinzenz Feenstra <evilissmo@redhat.com>
vinzenz pushed a commit to vinzenz/yaml that referenced this issue Sep 20, 2017
Signed-off-by: Vinzenz Feenstra <evilissmo@redhat.com>
vinzenz pushed a commit to vinzenz/yaml that referenced this issue Sep 20, 2017
Signed-off-by: Vinzenz Feenstra <evilissmo@redhat.com>
@niemeyer
Copy link
Contributor

niemeyer commented Mar 9, 2018

Sorry for the lack of feedback here. I can't just merge the PR fixing this as there are a number of unrelated changes in it. That said, MapSlice is about to die. yaml.v3 is on the way and a more comprehensive way to decode content partially and preserve style will be introduced.

I'll keep this issue open and close it when it's ready.

@lox
Copy link

lox commented Mar 10, 2018

Thanks for the update @niemeyer. Given lots of us are likely to be on v2 for some time, would you still be amenable to accepting a patch for this specific issue in v2?

@fredbi
Copy link

fredbi commented Mar 10, 2018

Thank you @niemeyer. I am also watching this one.

@niemeyer
Copy link
Contributor

No problem, @fredbi.

@lox Sure, as long as it's indeed fixing the issue alone and the patch is clean and tested, happy to apply.

@niemeyer
Copy link
Contributor

I'd like to put a tombstone on this issue. 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.

v3 is on the way and coming soon.

@gsemet
Copy link

gsemet commented Nov 15, 2018

Damned, still no support of anchor in yo-yaml? Still is it a pretty standard way of using yaml ...

when is v3 expected?

@lox
Copy link

lox commented Nov 16, 2018

We maintain a fork that we use for Buildkite at https://github.com/buildkite/yaml that includes the patches for anchors with MapSlice @gsemet

@ghost
Copy link

ghost commented Feb 10, 2019

for anyone tired of waiting - these tools give correct output:

laszlocph added a commit to laszlocph/yaml that referenced this issue Nov 14, 2019
* v/fix-for-issue-91: (40 commits)
  Add test cases from go-yaml#184
  Fix for issue go-yaml#91
  Fixes go-yaml#214 - New option to allow setting strict boolean mode
  Fix for issue go-yaml#144
  Always use the pointer mechanism, but only allow recursion per option
  Applied API changes as suggested in another PR and fixed outstanding problems
  Removed introduced shadowing bug
  Make aliases share the same memory address as the anchor ( go-yaml#215 )
  Replace LICENSE text with actual license (go-yaml#274)
  Make tag scanning code slightly cleaner.
  move embedded struct example into godoc
  Add UnmarshalStrict returning error if yaml has fields that do not exist in structure
  correct misspell on yamlh.go
  fix misspell on emmiterc.go
  Remove unreachable code to fix go vet (go-yaml#249)
  Fix dead URL for yaml specification (go-yaml#240)
  Tighten restrictions on float decoding (go-yaml#171)
  Fix decode test for Go 1.8 (go-yaml#217)
  Fix unmarshaler handling of empty strings.
  new license in the README file (go-yaml#189)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants