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

Limit underscore suffix to non-public members #1948

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Feb 15, 2023

Augments #1939 by enforcing the new rule by clang-tidy.
Here is a CI job running clang-tidy on all source files (not only the diff as usually done for PRs).

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 50.30% // Head: 50.30% // No change to project coverage 👍

Coverage data is based on head (2f60594) compared to base (bff9600).
Patch coverage: 74.25% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1948   +/-   ##
=======================================
  Coverage   50.30%   50.30%           
=======================================
  Files         374      374           
  Lines       31358    31358           
=======================================
  Hits        15772    15772           
  Misses      15586    15586           
Impacted Files Coverage Δ
..._core/planning_interface/src/planning_response.cpp 29.63% <37.50%> (ø)
...mpl_interface/src/model_based_planning_context.cpp 50.57% <73.92%> (ø)
...ning/robot_model_loader/src/robot_model_loader.cpp 77.09% <80.00%> (ø)
..._industrial_motion_planner/planning_context_base.h 100.00% <100.00%> (ø)
...strial_motion_planner/src/trajectory_generator.cpp 96.74% <100.00%> (ø)
...ude/moveit/robot_model_loader/robot_model_loader.h 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

Easy approve! Thank you.

@tylerjw tylerjw merged commit 694e1f1 into moveit:main Feb 15, 2023
@rhaschke
Copy link
Contributor Author

I was hoping you would wait for the linked CI job to finish before merging. Let's see whether clang-tidy finds more fixes... Sorry for not being clear enough initially.

@rhaschke rhaschke deleted the public-member-naming branch February 15, 2023 16:51
@tylerjw
Copy link
Member

tylerjw commented Feb 15, 2023

I was hoping you would wait for the linked CI job to finish before merging. Let's see whether clang-tidy finds more fixes... Sorry for not being clear enough initially.

Oh, I'm sorry. I through because CI passed here that this was fixed and this was just adding a check for future changes.

@tylerjw
Copy link
Member

tylerjw commented Feb 15, 2023

Oof, I see there are a bunch more of these changes to make. Maybe we should revert this change for now :(

@rhaschke
Copy link
Contributor Author

I have gone through most of these changes in ROS1 already: moveit/moveit#3317.
I will try to apply those changes here as well...

@rhaschke rhaschke mentioned this pull request Feb 15, 2023
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.

2 participants