Added convenience constructor to plugin #911
Conversation
Signed-off-by: Nate Koenig <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf12 #911 +/- ##
==========================================
+ Coverage 87.63% 87.65% +0.02%
==========================================
Files 104 104
Lines 14996 15024 +28
==========================================
+ Hits 13142 13170 +28
Misses 1854 1854
Continue to review full report at Codecov.
|
src/CMakeLists.txt
Outdated
| @@ -118,6 +120,7 @@ target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME} | |||
| ignition-math${IGN_MATH_VER}::ignition-math${IGN_MATH_VER} | |||
| ignition-utils${IGN_UTILS_VER}::ignition-utils${IGN_UTILS_VER} | |||
| PRIVATE | |||
| ignition-common${IGN_COMMON_VER}::ignition-common${IGN_COMMON_VER} | |||
There was a problem hiding this comment.
we added ignition::common for USD I'm not really sure that you can use this dependency here
There was a problem hiding this comment.
The dependency doesn't seem bad to me. Here is the code I'd duplicate, which I could trim (haha) down:
/////////////////////////////////////////////////
void ignition::common::ltrim(std::string &_s)
{
_s.erase(_s.begin(), std::find_if(_s.begin(), _s.end(),
[](int c) {return !std::isspace(c);}));
}
/////////////////////////////////////////////////
void ignition::common::rtrim(std::string &_s)
{
_s.erase(std::find_if(_s.rbegin(), _s.rend(),
[](int c) {return !std::isspace(c);}).base(),
_s.end());
}
/////////////////////////////////////////////////
void ignition::common::trim(std::string &_s)
{
ignition::common::ltrim(_s);
ignition::common::rtrim(_s);
}
/////////////////////////////////////////////////
std::string ignition::common::ltrimmed(std::string _s)
{
ignition::common::ltrim(_s);
return _s;
}
/////////////////////////////////////////////////
std::string ignition::common::rtrimmed(std::string _s)
{
ignition::common::rtrim(_s);
return _s;
}
/////////////////////////////////////////////////
std::string ignition::common::trimmed(std::string _s)
{
ignition::common::trim(_s);
return _s;
}
Utilizing existing code rather than duplicating it seems like something we should do internally. Otherwise, we should be telling users to copy our code, and not use the libraries.
There was a problem hiding this comment.
I don't think we should add a dependency in a stable version. We already have sdf::trim that can be used instead.
There was a problem hiding this comment.
Good point, this is a stable version.
There was a problem hiding this comment.
Adding ign-common as a dependency to the core of SDF is something we should think carefully about. That brings a lot of dependencies. An alternative to consider would be to move some of this functionality to ign-utils.
src/Plugin.cc
Outdated
| { | ||
| this->SetFilename(_filename); | ||
| this->SetName(_name); | ||
| std::string trimmed = ignition::common::trimmed(_xmlContent); |
There was a problem hiding this comment.
you are only using ignition::common here, can you remove this dependency here ?
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig [email protected]
🎉 New feature
A non-trivial amount of code is required to programmatically construct a plugin that has content. For example, this plugin
Requires this code:
This pull request adds an
InsertContentfunction and an initialization constructor to thePluginclass. Now you'll be able to build the same plugin using:I copied the
copyChildrenfunction fromparser.cctoUtils.ccso that the Plugin class can use it. This shouldn't effect API/ABI.Test it
New tests have been added to
Plugin_TEST.ccChecklist
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.