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

Unsupported merges #41

Open
GoogleCodeExporter opened this issue Mar 30, 2015 · 22 comments
Open

Unsupported merges #41

GoogleCodeExporter opened this issue Mar 30, 2015 · 22 comments

Comments

@GoogleCodeExporter
Copy link

Current version of otherwise great yaml-cpp does not seem to support merges
in a way described for example here:
http://en.wikipedia.org/wiki/YAML#Data_merge_and_references

I don't have a deep knowledge of yaml-cpp internals (I am just its user),
but maybe this is somehow linked with the cloning problem - not yet
sufficiently solved.

I have tested the simple form of a merge in yaml-cpp 0.2.0 on RedHat Linux
3.4.6-9 using gcc 3.4.6 (20060404) - it doesn't work, but at least it
doesn't hurt the parser. With that gcc I am unable to compile 0.2.1 because
of new conversion operators in nodeimpl.h (it reported an error on lines
with "node.Read<T>" and the thing that caused that error was very probably
the templated read).

Do you plan to support merges in some future version?

Original issue reported on code.google.com by pole...@gmail.com on 8 Sep 2009 at 10:38

@GoogleCodeExporter
Copy link
Author

To be even more precise - I just found the definition of merge right on 
yaml.org in
the following document: http://yaml.org/type/merge.html

Such a functionality is very useful if a YAML file is used as a configuration 
file,
because it allows creation of configuration groups and either overriding some 
values
or adding another values in different keys being aliases of a group.

Original comment by pole...@gmail.com on 8 Sep 2009 at 11:11

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Another clarification:

"With that gcc I am unable to compile 0.2.1 because of new conversion operators 
in
nodeimpl.h ..." - I thought (templated) comparison operators, of course. Sorry 
for
the mistake.

Maybe I will raise another issue regarding this problem later, but for now 
0.2.0 is
enough for me and there's nothing so important for me in 0.2.1 (and brand new 
0.2.2)
that I can't live without ;-)

Original comment by pole...@gmail.com on 8 Sep 2009 at 11:39

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Two things:

1. I actually didn't realize that merge was officially a part of YAML. I read 
it on
the wikipedia page, but the spec doesn't have any reference to a merge. Given 
that it
does seem to be officially supported, I may add it in, but I'm not 100% sure 
what
status the yaml.org/type/* files should have. I do understand that this is very
useful, though (the so-called "prototype" pattern, according to Steve Yegge).

2. What do you mean by you're unable to compile it? Does the library (by 
itself) not
compile on gcc 3.4.6? Or is it some extension code you've written that doesn't
compile? If it's the latter, what does the code look like? In either case, what 
is
the error message?

Original comment by jbe...@gmail.com on 9 Sep 2009 at 1:47

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Original comment by jbe...@gmail.com on 9 Sep 2009 at 1:47

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

1. Thank you for your positive opinion. Because of it this issue hopefully 
remains
open until you implement the merge.

2. Of course the library itself cannot be compiled. I made a mistake to run gcc 
-v,
because it is of some other version than g++32 actually used for compilation. 
See
http://code.google.com/p/yaml-cpp/issues/detail?id=42 for details. But I must 
say,
that with the following g++ (not some older g++32) everything is OK, even 
#pragma once:

$ g++ -v
Reading specs from /usr/lib/gcc/x86_64-redhat-linux/3.4.6/specs
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info --enable-shared --enable-threads=posix 
--disable-checking
--with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions
--enable-java-awt=gtk --host=x86_64-redhat-linux
Thread model: posix
gcc version 3.4.6 20060404 (Red Hat 3.4.6-3)

Original comment by pole...@gmail.com on 9 Sep 2009 at 7:59

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Yeah, I'll definitely leave this open until I implement it.

As for the gcc version, I do most of my testing with gcc 4.0.1 or 4.1.2. It's 
good to
hear it works with *something* in the 3.4.* range. It's very likely a bug in 
3.2.3
that's been fixed - why are you using 3.2.3?

Original comment by jbe...@gmail.com on 9 Sep 2009 at 4:52

  • Changed state: Accepted
  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

As a comment I added to the Issue 42 says, the reason for using such 
prehistoric GCC
was Oracle 10g compatibility. Nowadays we are using Oracle 11g and are happy 
with GCC
3.4.6 ;-)

Original comment by pole...@gmail.com on 1 Oct 2009 at 10:47

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

I'm missing merges too, is there any update on the subject?
Can you estimate when it will be implemented (if you have such plans)?
In case I'll have time to contribute, can you estimate the amount of work 
involved?
Thanks.

Original comment by oranagra...@gmail.com on 21 Mar 2010 at 1:19

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

The problem with merges is that I think I'd like to restructure the 
architecture of 
Nodes a bit. It wouldn't be that hard to "glue" merges onto the current 
structure, but 
it would be a big wart, and I want to do it the "right way".

As-is, there are a number of other things I'm working on now, so it probably 
will be 
at least several months before I get to this.

If you're interested in contributing, I'd be happy to see your patch, but I 
probably 
wouldn't immediately apply it (though I probably *would* take some hints from 
your 
code).

Original comment by jbe...@gmail.com on 21 Mar 2010 at 4:01

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Technically, the merge specification come from the types repository for YAML 
1.1,
while yaml-cpp is based on YAML 1.2.  I can understand how it would be a nice 
feature
to have, but there is a fundamental issue regarding equality of scalar values 
that
needs to be resolved first.  After all, you can't do a merge unless you can 
decide
whether the strings "17" and "0x11" (when expressed as plain scalars) are equal.

Without implementing a (extensible) mechanism for resolving scalars to canonical
form, I'm not sure how this can be implemented inside the yaml-cpp library and, 
IMO,
the client code should only get this functionality if it specifically requests 
it.

Original comment by rtweeks21 on 26 Mar 2010 at 11:44

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Richard, you're right, but we actually do that kind of resolution in yaml-cpp 
already - 
it's just merging raises more questions about it. For example,

0x11: foo
17: bar

is a legal yaml-cpp node, but `node[17]` will only retrieve one (unspecified 
which 
one, and you'd have to iterate through to get the whole thing, or ask for 
`node["0x11"]`). But the corresponding

0x11: foo
>>:
  17: bar

will also be a legal node, but in this case `node[17]` will retrieve `foo` (and 
to get 
'bar', I'm really not sure what you'd get if you iterate through the node).

I do completely agree that you only get this functionality if you specifically 
request it.

Original comment by jbe...@gmail.com on 28 Mar 2010 at 1:18

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Hi, I need merges too, so I did a small fix to support this. It works fine for 
me, but I do not know project's architecture, so afraid if some side affects 
can occur.
The change is to insert the lines below into FindValueForKey template (between 
for-loop and return 0):

        const Node *pValueMerge = FindValueForKey(std::string("<<"));
        if(pValueMerge)
        {
            return pValueMerge->FindValueForKey(key);
        }

Could you please comment?

Original comment by barma...@gmail.com on 31 Jan 2011 at 7:12

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

@barma:

That would work for merges with a single dictionary, but the spec allows

>>: [*dict1, *dict2]

to merge multiple dictionaries.

Original comment by jbe...@gmail.com on 31 Jan 2011 at 5:52

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Any updates to this?  I have a overridable configuration situation which would 
be handled cleanly if there was this merge support in yaml-cpp.

Original comment by nev...@gmail.com on 12 Nov 2012 at 2:31

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Sorry, no updates to this. It's pretty low priority, to be honest, especially 
given all of the difficulties in making it "correct" (detailed above).

Original comment by jbe...@gmail.com on 12 Nov 2012 at 5:20

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Any updates this year?

Original comment by ben.far...@gmail.com on 7 Mar 2014 at 5:26

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Nope :(

Original comment by jbe...@gmail.com on 7 Mar 2014 at 5:32

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

I have implemented this feature in my fork of the 0.5.1 code. It's gotten 
slightly behind the core here, so maybe it's not a straight patch, and the 
coding style is inconsistent, but it's still better than nothing.

The revision which implements this feature can be viewed here: 

https://github.com/WrinklyNinja/yaml-cpp/commit/c55b40b5ea42e337bb1c3b1870d1508f
f3a99966

I hope that helps!

Original comment by Oliver.H...@gmail.com on 23 Oct 2014 at 2:29

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

I applied the patch to my copy of 0.5.1, and it worked as expected (at least 
for the use case that I tried). Thanks!

Original comment by sho...@gmail.com on 3 Nov 2014 at 7:17

  • Added labels: ****
  • Removed labels: ****

@SimplyKnownAsG
Copy link

looks like this is probably a duplicate of #300 now

@agirault
Copy link

There are many duplicates to this feature request: #353, #617, #638, #683.

@Ortham proposed an implementation in #300 back in 2015 (!) that was garnering a lot of support, then closed in 2021 because it's not officially in the spec: #300 (comment)

Now:

  • this issue is still open, and mentioned by many projects
  • almost every online documentation of YAML that discusses "anchors" will introduce the << merge key capability
  • @Ortham did address the concern regarding the lack of spec here: Add support for merge keys. #300 (comment)

Based on this, can we re-open the discussion to consider merge keys as a feature (even if optionally built) of yaml-cpp? Or should this issue be closed for the same reason that #300 was closed?

@jbeder
Copy link
Owner

jbeder commented Jan 17, 2023

This is a good point, and I'm not sure.

This is a pretty big feature, and would need thorough review. I'm the only maintainer, and I unfortunately don't have time for it. I also (as you can see by this history of this feature) never really thought much of this feature.

I've been unable to find someone to help maintain this project (any takers?). If I found someone who I trusted and is willing to support and review this feature, then I'd be open to accepting this feature.

Also, thanks for the links to the other feature requests, and they should at least be closed as dupes of this one.

nlescoua added a commit to Broadpeak-tv/yaml-cpp that referenced this issue Nov 8, 2023
Support for YAML Merge keys ( <<: [*dict1, *dict2] ) is added. The merge
key is a specific scalar with value << (and tag !!merge) that implies
that during node construction, the map (or sequence of maps) are merged
into the current map. The priority rules are that each key from maps
within the value associated with  << are added iff the key is not yet
present in the current map (and first map gets higher priority). Test
cases have been added accordingly.
nlescoua added a commit to Broadpeak-tv/yaml-cpp that referenced this issue Nov 8, 2023
Support for YAML Merge keys ( <<: [*dict1, *dict2] ) is added. The merge
key is a specific scalar with value << (and tag !!merge) that implies
that during node construction, the map (or sequence of maps) are merged
into the current map. The priority rules are that each key from maps
within the value associated with  << are added iff the key is not yet
present in the current map (and first map gets higher priority). Test
cases have been added accordingly.
azat added a commit to azat-ch/yaml-cpp that referenced this issue Apr 16, 2024
… update-lib-and-merge-operator

* github.com:jbeder/yaml-cpp:
  Fix merge-key handling in case the dictionary contains a sub-dictionary
  Adding support for handling YAML Merge Key (jbeder#41)
azat added a commit to azat-ch/yaml-cpp that referenced this issue May 3, 2024
* merge-operator:
  Fix merge operator support (that can be visible by iterating through the node)
  Fix merge-key handling in case the dictionary contains a sub-dictionary
  Adding support for handling YAML Merge Key (jbeder#41)
azat added a commit to azat-ch/yaml-cpp that referenced this issue May 3, 2024
* merge-operator:
  Fix order for merging iterator
  Remove cast to const from MergeMapCollection()
  Fix merge operator support (that can be visible by iterating through the node)
  Fix merge-key handling in case the dictionary contains a sub-dictionary
  Adding support for handling YAML Merge Key (jbeder#41)
azat added a commit to azat-ch/yaml-cpp that referenced this issue May 3, 2024
* merge-operator:
  Fix order for merging iterator
  Fix merge operator support (that can be visible by iterating through the node)
  Fix merge-key handling in case the dictionary contains a sub-dictionary
  Adding support for handling YAML Merge Key (jbeder#41)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants