Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
(moveit_py) Add Trajectory Execution Manager #2406
(moveit_py) Add Trajectory Execution Manager #2406
Changes from 4 commits
98a16dd
5c1ba00
d5601c7
9fc02e0
42abd13
bd2be40
b4a686a
9585809
22c61d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the python functions be defined in
snake_case
(camelCase
is used for cpp functions)? I think it is done like this everywhere else, so we should be consistent about it. Can you update the other function signatures as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update many of the existing cpp methods as there are discrepancies elsewhere in the library which may have lead to @MatthijsBurgh using different conventions.
Not sure if there is an easy way to add this to auto formatter (clang-format) but I agree with the above convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added issue: #2499
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that got covered by #2177 or do you mean something else @peterdavidfagan? That PR also added clang-tidy checks to warn about cpp function names that don't follow the conventions. But don't think there are checks to catch convention mistakes in
.def("...", ..)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
49052c1 ❤️.
My fork was outdated only seeing these updates now. Thanks so much.
As for
.def
I would personally need to become more familiar withclang-tidy
, maybe there is a way to get a rule which also flags.def
. For now I am happy to be more vigilant about this when pushing code or reviewing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for opening the issue though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the python functions will be snake_case?