Skip to content
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

Merged

Conversation

methylDragon
Copy link
Collaborator

See: #276

Linting
Migrates .h to .hpp
Pinging @svwilliams for visibility.

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>
if (a.back() == '/') {
a.pop_back();
}
if (b.front() == '/') {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@methylDragon
Copy link
Collaborator Author

@ros-pull-request-builder run tests please!
Build Status

Signed-off-by: methylDragon <methylDragon@gmail.com>
@sloretz
Copy link
Collaborator

sloretz commented Jan 10, 2023

@ros-pull-request-builder retest this please!

@sloretz
Copy link
Collaborator

sloretz commented Jan 10, 2023

Build Status

Copy link
Collaborator

@sloretz sloretz left a 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

@methylDragon
Copy link
Collaborator Author

Merging since the only instabilities are viz

@methylDragon methylDragon merged commit dc252aa into locusrobotics:rolling Jan 10, 2023
@methylDragon methylDragon deleted the rolling-publishers-linting branch January 10, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants