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

adds const to moveit_core/distance_field issue 879 #2394

Conversation

danielcorona19
Copy link

@danielcorona19 danielcorona19 commented Sep 28, 2023

Description

Added const keyword to function parameters in moveit_core/distance_field. This PR is related to #879

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 Sep 29, 2023

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

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (9418678) 50.87% compared to head (3bac249) 50.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2394      +/-   ##
==========================================
- Coverage   50.87%   50.87%   -0.00%     
==========================================
  Files         386      386              
  Lines       31983    31982       -1     
==========================================
- Hits        16269    16268       -1     
  Misses      15714    15714              
Files Coverage Δ
...eld/include/moveit/distance_field/distance_field.h 100.00% <ø> (ø)
...moveit/distance_field/propagation_distance_field.h 88.10% <100.00%> (-0.27%) ⬇️
...t_core/distance_field/src/find_internal_points.cpp 100.00% <100.00%> (ø)
.../distance_field/src/propagation_distance_field.cpp 93.84% <100.00%> (ø)
...e_field/include/moveit/distance_field/voxel_grid.h 95.32% <91.67%> (ø)
moveit_core/distance_field/src/distance_field.cpp 29.56% <42.86%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Can you run pre-commit run -a on this branch to fix the formatting failure? Looks great otherwise!

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Quick hold-up before merging. Do we event want to add cost for per-value parameters? There has been a related discussion here #2119 and we should follow a consistent style.

@henningkayser henningkayser added the discuss Highlight for discussion in Standup or Maintainer Meeting label Oct 10, 2023
@sea-bass
Copy link
Contributor

Quick hold-up before merging. Do we event want to add cost for per-value parameters? There has been a related discussion here #2119 and we should follow a consistent style.

Good call -- particularly the opinion in #2119 (comment) is that const for simple value parameters doesn't really do much. I'm in favor of closing this for consistency and to reduce change for the sake of change?

@danielcorona19
Copy link
Author

I just did this PR because I am a new contributor and I would like to understand the procedure of contributing to the MoveIt project. I wasn't able to correct the clang-format because the pre-commit run -a wasn't automatically correcting the format.

@sea-bass
Copy link
Contributor

No worries @danielcorona19 -- I think we'll go ahead and close this one because of #2119 (comment).

Thank for your working on this and we look forward to your next contiribution(s) -- here are many other "good first issue" type issues to try out!

@sea-bass sea-bass closed this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Highlight for discussion in Standup or Maintainer Meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants