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

remove underscore from public member in MotionPlanResponse #1939

Merged
merged 5 commits into from
Feb 14, 2023

Conversation

AlexWebb03
Copy link
Contributor

@AlexWebb03 AlexWebb03 commented Feb 9, 2023

Description

Fixes this. Pretty self-explanatory, one question I had is whether the same changes should be applied ExecutableMotionPlan, since this struct also uses the suffix notation inappropriately.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@mergify
Copy link

mergify bot commented Feb 9, 2023

This pull request is in conflict. Could you fix it @AlexWebb03?

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Base: 50.30% // Head: 50.29% // Decreases project coverage by -0.00% ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1939      +/-   ##
==========================================
- Coverage   50.30%   50.29%   -0.00%     
==========================================
  Files         374      374              
  Lines       31358    31358              
==========================================
- Hits        15772    15769       -3     
- Misses      15586    15589       +3     
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%> (ø)
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 78.04% <0.00%> (-1.13%) ⬇️

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.

@sjahr
Copy link
Contributor

sjahr commented Feb 9, 2023

@AlexWebb03 Thanks for addressing this issue! This looks good.

Can you fix the CI failures? You can format the code by running pre-commit run -a

one question I had is whether the same changes should be applied ExecutableMotionPlan, since this struct also uses the suffix notation inappropriately.

It would be wonderful if you could address this as well 👍, you can either add it to this PR or open a new one.

@AlexWebb03
Copy link
Contributor Author

I've done the extra struct in this MR, and have run the pre-commit hook. Thanks for the advice and quick response!

@sjahr sjahr merged commit 8bfe782 into moveit:main Feb 14, 2023
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Feb 15, 2023
@rhaschke
Copy link
Contributor

Merging breaking API changes without a deprecation period is bad practice. Now, many downstream packages require an immediate fix.

tylerjw pushed a commit that referenced this pull request Feb 15, 2023
henningkayser pushed a commit to moveit/moveit_task_constructor that referenced this pull request Feb 17, 2023
Required to align with changes introduced by moveit/moveit2#1939

Co-authored-by: JafarAbdi <[email protected]>
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.

Remove _ suffix from struct member variables
4 participants