Fix for custom element with attribute 'type'#811
Fix for custom element with attribute 'type'#811scpeters merged 2 commits intochapulina/12/plugin_typefrom
Conversation
…added fix in ParamPrivate::ValueFromStringImpl instead Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Codecov Report
@@ Coverage Diff @@
## chapulina/12/plugin_type #811 +/- ##
===========================================================
Coverage ? 90.70%
===========================================================
Files ? 78
Lines ? 12432
Branches ? 0
===========================================================
Hits ? 11276
Misses ? 1156
Partials ? 0 Continue to review full report at Codecov.
|
scpeters
left a comment
There was a problem hiding this comment.
this does look like a better fix; I just have one question about the order of some changes
| if (elemXml->GetText() != nullptr) | ||
| { | ||
| const std::string attributeName(attribute->Name()); | ||
| // TODO(chapulina) This isn't a good place to parse pose-specific | ||
| // attributes | ||
| if (elemName == "pose" && attributeName == "type") | ||
| typeName = attribute->Value(); | ||
|
|
||
| element->AddAttribute(attributeName, "string", "", 1, ""); | ||
| element->GetAttribute(attributeName)->SetFromString( | ||
| attribute->Value()); | ||
| element->AddValue("string", elemXml->GetText(), true); | ||
| } | ||
|
|
||
| if (elemXml->GetText() != nullptr) | ||
| for (const tinyxml2::XMLAttribute *attribute = elemXml->FirstAttribute(); | ||
| attribute; attribute = attribute->Next()) | ||
| { | ||
| if (typeName.has_value()) | ||
| element->AddValue(typeName.value(), elemXml->GetText(), true); | ||
| else | ||
| element->AddValue("string", elemXml->GetText(), true); | ||
| element->AddAttribute(attribute->Name(), "string", "", 1, ""); | ||
| element->GetAttribute(attribute->Name())->SetFromString( | ||
| attribute->Value()); |
There was a problem hiding this comment.
it looks like you've reversed the order of some logic in parser.cc, calling element->AddValue before / after setting the attribute values. Does this order matter? If not, can you reverse it so that the diff will be easier to read?
There was a problem hiding this comment.
This order does not matter as in this case the value is always added as type string.
Only with specific types like pose, where the attributes like degrees or rotation_format are read, will it matter. But it still makes sense to have attributes handled before the value, regardless.
Reversed in c630d30.
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org> * Add line with unrecognized type Signed-off-by: Steve Peters <scpeters@openrobotics.org> * Fix for custom element with attribute 'type' (#811) * Reverted changes to parser for custom element with attribute 'type', added fix in ParamPrivate::ValueFromStringImpl instead Signed-off-by: Aaron Chong <aaronchongth@gmail.com> Co-authored-by: Steve Peters <scpeters@openrobotics.org> Co-authored-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong aaronchongth@gmail.com
🦟 Bug fix
Related to #809.
Summary
Reverted changes to parser for custom element with attribute 'type', added fix in ParamPrivate::ValueFromStringImpl instead.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.