Style overhaul #64
Style overhaul #64
Conversation
There was a problem hiding this comment.
thanks @wjwwood for pounding on this. I did a first round of review with a few comments (mostly questions rather than request to change things).
Some of the comments are unrelated to this code change but can be used as "note to self" to be addressed in the near future if we consider them out of the scope of this PR
| std::vector<std::string> getPluginXmlPaths(); | ||
|
|
||
| /** | ||
| * @brief Returns a list of all available classes for this ClassLoader's base class type |
There was a problem hiding this comment.
any reason for keeping the @brief Description rather than using a /// Description like for the other functions above ?
There was a problem hiding this comment.
nvm I guess it's because these description fit on a single line so there was no need to switch the format
There was a problem hiding this comment.
Yeah, I only touched the ones that were too long. I'd like to come back and make them all uniform, but I was trying to minimize the changes.
| /** | ||
| * @brief Checks if the library for a given class is currently loaded | ||
| * @param lookup_name The lookup name of the class to query | ||
| * @return True if the class is loaded, false otherwise |
There was a problem hiding this comment.
I know it's not part of the cpplint/uncrustify rules, but it may be good to update the return descriptions to list the valid return values. something like @return 'true' if the class is loaded, 'false' otherwise maybe ? (Not sure what's good practice here, I'm looking for feedback not requesting a change)
There was a problem hiding this comment.
Happy to do that, but I want to do a separate documentation pr where I review each of them.
| bool isClassLoaded(const std::string & lookup_name); | ||
|
|
||
| /** | ||
| * @brief Checks if the class associated with a plugin name is available to be loaded |
There was a problem hiding this comment.
Question: Would it be more consistent to use a single space between keyword and description?
| */ | ||
| virtual bool isClassAvailable(const std::string & lookup_name); | ||
|
|
||
| /// Attempt to load the library containing a class with a given name. |
There was a problem hiding this comment.
Nit: Attempts (or use infinitive in every other description, I don't know which one is best, once decided we should update this PR to adopt it everywhere)
There was a problem hiding this comment.
I think the singular form is preferred, but again I only touched the lines which had to be moved.
I would convert all of them to singular in a documentation fixup pr.
| */ | ||
| virtual void refreshDeclaredClasses(); | ||
|
|
||
| /// Decrement the counter for the library containing a class with a given name. |
| tinyxml2::XMLDocument document; | ||
| document.LoadFile(package_xml_path.c_str()); | ||
| tinyxml2::XMLElement * doc_root_node = document.FirstChildElement("package"); | ||
| if (doc_root_node == NULL) { |
There was a problem hiding this comment.
Nit/Note to self: not sure how far we want to go down the ros2 style path. we could audit uses of == and make sure that the variable is always on the right side of the operator
include/pluginlib/class_loader_imp.h
Outdated
| ". Make sure that you are calling the PLUGINLIB_EXPORT_CLASS macro in the " | ||
| "library code, and that names are consistent between this macro and your XML. " | ||
| "Error string: " | ||
| + |
There was a problem hiding this comment.
this looks a bit weird, can we place the + on the previous line ?
| */ | ||
|
|
||
| #ifndef TEST_BASE_H_ | ||
| #define TEST_BASE_H_ |
There was a problem hiding this comment.
Shouldn't the header guard be prepended with the project name ? or is it not the case here because none of this lives in the pluginlib namespace ? (same comment for all test headers)
There was a problem hiding this comment.
I don't know I was just following the suggestion of ament_cpplint.
There was a problem hiding this comment.
Oh I see, it's because compared to most ROS2 repos, this repo doesnt have a pluginlib folder at the root of the repo, so the path from the root of the repository is test/test_base.h and not pluginlib/test/test_base.h hence the currently suggested header guard. That's something we may consider changing in the future but shouldn't impact this PR
| } | ||
| catch(pluginlib::PluginlibException& ex) | ||
| { | ||
| EXPECT_EQ(foo->result(), 100.0); |
There was a problem hiding this comment.
Nit/Note to self: expected value first (same applies to all gtests)
test/unique_ptr_test.cpp
Outdated
| { | ||
| FAIL() << "Library containing class should be loaded but isn't."; | ||
| ROS_INFO("Checking if plugin is loaded with isClassLoaded..."); | ||
| if (pl.isClassLoaded("pluginlib/foo") ) { |
There was a problem hiding this comment.
Nit: extra whitespace before closing parenthesis (same below)
There was a problem hiding this comment.
Yeah, I've noticed that uncrustify doesn't catch this always, I briefly investigated it but couldn't find a good setting to fix it.
I did this by running
ament_uncrustifyandament_cpplintover the project. The idea of this pr is to get the style of the project close to what we'd like to have even if we cannot easily add automated tests to ensure the style does not diverge. In the future if we can add these tests, the diff will be smaller due to this pr.I went through and tried to ensure there were no API or ABI changes, but the reviewers should watch closely for these.
I will make another pr to melodic-devel with ABI breaking style changes (using
expliciton single argument constructors for example).