Skip to content

Fix mergYAMLNodes when the node is a sequence #128

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 16, 2025
Merged

Fix mergYAMLNodes when the node is a sequence #128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions src/creo2urdf/src/Utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,23 +269,34 @@ std::string extractFolderPath(const std::string& filePath) {


void mergeYAMLNodes(YAML::Node& dest, const YAML::Node& src) {
if (!src || src.IsNull()) return;

for (const auto& item : src) {
std::string key = item.first.as<std::string>();
if (!dest[key]) {
// If the key doesn't exist in the destination node, simply add it
dest[key] = item.second;
}
else {
// If the key exists in the destination node, merge the values
// If both values are maps, recursively merge them
if (dest[key].IsMap() && item.second.IsMap()) {
mergeYAMLNodes(dest[key], item.second);
// If src is a map
if (src.IsMap()) {
for (const auto& item : src) {
const YAML::Node& keyNode = item.first;
const YAML::Node& srcValue = item.second;
const std::string key = keyNode.as<std::string>();

if (!dest[key]) {
dest[key] = srcValue;
}
else {
// Otherwise, overwrite the value in the destination node
dest[key] = item.second;
YAML::Node& destValue = dest[key];
mergeYAMLNodes(destValue, srcValue);
}
}
}

// If both are sequences, append src items to dest
else if (src.IsSequence() && dest.IsSequence()) {
for (const auto& item : src) {
dest.push_back(item);
}
Comment on lines +292 to +295
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It's good that you're handling sequences. However, consider adding a check to ensure that the items being pushed back are compatible with the destination sequence's expected type. If they are not, this could lead to runtime errors or unexpected behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is too complex to do this check

}

// If dest and src are scalars or mismatched types, overwrite
else {
dest = src;
}
Comment on lines +298 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This else block overwrites the destination node if the types are mismatched or if they are scalars. While this behavior might be intended, it's worth documenting this explicitly with a comment to clarify the expected behavior for future maintainers.

Suggested change
// If dest and src are scalars or mismatched types, overwrite
else {
dest = src;
}
// If dest and src are scalars or mismatched types, overwrite
// This ensures that the destination node is updated with the source value
else {
dest = src;
}

}