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

gazebo_ros: fix cpplint and flake8 errors #1355

Open
wants to merge 3 commits into
base: ros2
Choose a base branch
from

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Feb 6, 2022

There are lots of cpplint errors since a recent change in rolling. This fixes the new build_order errors in the gazebo_ros package. gazebo_plugins will be addressed in a separate pull request.

@scpeters scpeters requested a review from jacobperron February 6, 2022 09:18
Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

#include <geometry_msgs/msg/point.hpp>
#include <geometry_msgs/msg/point32.hpp>
#include <geometry_msgs/msg/pose.hpp>
#include <geometry_msgs/msg/quaternion.hpp>
#include <geometry_msgs/msg/transform.hpp>
#include <geometry_msgs/msg/vector3.hpp>
#include <ignition/math/Pose3.hh>
#include <ignition/math/Quaternion.hh>
#include <ignition/math/Vector3.hh>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess cpplint thinks these are c-system headers? The other option is the use double-quotes for all includes not from the standard library. I don't have a preference either way though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the new version of cpplint thinks that .hh headers are C headers instead of C++

I can switch to double quotes if you think that's a better option

Copy link
Collaborator

@jacobperron jacobperron Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a preference; I don't think there's an objective reason to use double-quotes. If we use double-quotes, I think we can keep the .hh files below any standard library headers 🤷. The size of the diff might actually turn out to be larger if we switch to double-quotes.

@chapulina
Copy link
Contributor

@ros-pull-request-builder retest this please
@osrf-jenkins run tests please

scpeters added 3 commits May 3, 2022 13:09
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@jacobperron
Copy link
Collaborator

I've rebased the changes on ros2 now that ROS CI is unblocked.

@jacobperron
Copy link
Collaborator

In retrospect, I think cpplint is at fault for considering files with the .hh extension to be C headers instead of C++ headers. I've proposed a patch upstream: ament/ament_lint#374

Note, even with the upstream patch, there are still a number of include order lint errors to address.

@jacobperron
Copy link
Collaborator

flake8 fixes were merged in #1380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants