-
Notifications
You must be signed in to change notification settings - Fork 123
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
fuse -> ROS 2 fuse_publishers : Linting #305
fuse -> ROS 2 fuse_publishers : Linting #305
Conversation
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
fuse_core/include/fuse_core/util.hpp
Outdated
if (a.back() == '/') { | ||
a.pop_back(); | ||
} | ||
if (b.front() == '/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend forbidding b
from beginning with a /
or a ~
. Either of those characters at the start means b
is an absolute topic/service/action name which normally aren't affected by namespacing. If they normally aren't affected by namespacing, then I'd also expect them to not be able to be prepended with something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@@ -31,38 +31,40 @@ | |||
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE | |||
* POSSIBILITY OF SUCH DAMAGE. | |||
*/ | |||
|
|||
// Workaround ros2/geometry2#242 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is in the wrong spot, or maybe the workaround is not needed. The workaround was to include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
before <tf2/utils.h>
, but that's not what's happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up reordering them just in case
166b39a
@@ -31,66 +31,76 @@ | |||
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE | |||
* POSSIBILITY OF SUCH DAMAGE. | |||
*/ | |||
|
|||
// Workaround ros2/geometry2#242 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about comment being in wrong spot, or workaround not actually being needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up reordering them just in case
166b39a
@ros-pull-request-builder run tests please! |
Signed-off-by: methylDragon <methylDragon@gmail.com>
@ros-pull-request-builder retest this please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with green CI
Merging since the only instabilities are viz |
See: #276
Linting
Migrates .h to .hpp
Pinging @svwilliams for visibility.