-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Question&Proposition] What is the best way to edit nested structures in place YAML::Node ? #1266
Comments
Nodes are reference type objects, even though copies are returned. What code are you trying to do that doesn't work? |
Here is the code that unfortunately does not work (no edit in place is happening): void merge(YAML::Node base, const YAML::Node& node)
{
for (auto it = node.begin(); it != node.end(); ++it)
{
const std::string& key = it->first.as<std::string>();
if (base[key].IsDefined())
{
if (base[key].IsMap() && it->second.IsMap())
{
merge(base[key], it->second);
}
else
{
base[key] = it->second;
}
}
else
{
base[key] = it->second;
}
}
} Did I miss anything? |
to be specific: base is copied (as object), therefore, the base passed to merge is a new object … not a ref one… and all merge happens on this one… but that's only a local … If I try to pass a |
Interesting. My instinct is not to add this behavior, since I'm not certain it wouldn't have other negative effects. Honestly, I would have expected your code to work, since you can write Have you looked at #41? I've rejected that PR but you might get some ideas from them. |
IMO, just adding a reference accessor would allow C++ merge. #41 would be a different thing, as they discuss merge from the YAML language point of view, which is a totally different can of worm ^^; My question would be merge from the C++ API. For anybody stumbling with the same issue out there, my current workaround is to re-create the YAML node recursively every time … which works but is horribly slow and memory consuming … (at least it gets the job done for now). @jbeder would you be ok for me to post a PR just adding the reference accessor like mentioned above? Node& Node::GetNode(const Key& key) (please suggest any name you would rather have if that is of any concern). The side effect to the current code base would likely be None… as it is just an additional API… thanks for your help! |
bump? |
So I just tried your code, and it doesn't seem to do what it says:
This prints:
Isn't this what you want? If not, please write down what you want. |
ah, I think I found my problem… it actually does work in the end ^^; With that said, would you then be open to a PR to add the merge function as part of YAML::Node ? |
The merge function in this thread? Why? |
I am probably missing something, but think merging 2 trees is more of a common operation. Right now, one can use the above code to allow it, but I feel like having this as a normal operation supported by the YAML::Node would make more sense. (I would obviously transform this as a member function of YAML::Node and change the signature, such as) enum MergePolicy {
MergeMaps = 0,
ReplaceMapsContent = 1<<1,
MergeSequences = 0,
ReplaceSequenceContent = 1<<2,
};
YAML::Node& YAML::Node::merge(const YAML::Node& src, enum MergePolicy policy=MergeMaps|ReplaceSequencesContent); Would you be interested in a PR like so? (The reason I prefer discussing a PR first, is I can see ~45 pending PR on this project, just trying to make sure if merging is an option ;) ) |
It's such a small amount of code and for a somewhat niche case, I'd just as soon leave it out. |
roger that. thanks for the information then. |
I found some merge code from here and works fine: const YAML::Node mergeNodes(const YAML::Node& defaultNode, const YAML::Node& overrideNode)
{
if (!overrideNode.IsMap()) {
// If overrideNode is not a map, merge result is overrideNode, unless overrideNode is null
return overrideNode.IsNull() ? defaultNode : overrideNode;
}
if (!defaultNode.IsMap()) {
// If defaultNode is not a map, merge result is overrideNode
return overrideNode;
}
if (!defaultNode.size()) {
return YAML::Node(overrideNode);
}
// Create a new map 'newNode' with the same mappings as defaultNode, merged with overrideNode
auto newNode = YAML::Node(YAML::NodeType::Map);
for (auto node : defaultNode) {
if (node.first.IsScalar()) {
const std::string& key = node.first.Scalar();
if (overrideNode[key]) {
newNode[node.first] = mergeNodes(node.second, overrideNode[key]);
continue;
}
}
newNode[n.first] = node.second;
}
// Add the mappings from 'overrideNode' not already in 'newNode'
for (auto node : overrideNode) {
if (!node.first.IsScalar()) {
const std::string& key = node.first.Scalar();
if (defaultNode[key]) {
newNode[node.first] = mergeNodes(defaultNode[key], node.second);
continue;
}
}
newNode[node.first] = node.second;
}
return YAML::Node(newNode);
} |
Hi,
I am trying to implement a merge method to allow several yaml files to be merged as overlays. It works perfect if the structure is only on depth 1, but when I do the nested version, I realize that the
operator[]
returns a copy of the node, rather than a reference… therefore it can't be modified in place.For now, I do a full copy on each merge, which isn't really ideal… but works.
I would like to propose a new YAML::Node::GetNode method to get a rw reference to underlying object, allowing modifying in place:
(and all derivative depending on Key value type)
Would this PR be accepted? or is there a better way to edit nested structures inplace?
The text was updated successfully, but these errors were encountered: