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

Feature/transform #17

Merged
merged 7 commits into from
Nov 23, 2021
Merged

Feature/transform #17

merged 7 commits into from
Nov 23, 2021

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented Nov 18, 2021

wep21 added 2 commits November 19, 2021 03:22
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

Add transform

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 requested a review from a team as a code owner November 18, 2021 18:28
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@emersonknapp
Copy link
Contributor

My major concern here is that the Python linters are not run on this script - probably because it doesn't end in .py. I'd like to make sure to apply standard ROS 2 linters to all code here, at the very least (an actual functional test would be nice but I won't block on that).

Could you make it so that the linters run on this new source? Probably you could strip the .py extension and do a chmod on the file as part of the CMake install step.

wep21 added 3 commits November 19, 2021 16:42
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21
Copy link
Contributor Author

wep21 commented Nov 19, 2021

@emersonknapp I use ament_cmake_python macro instead of pure cmake install and add entry points for transform executable. How about this modification?
Note: SCRIPTS_DESTINATION argument is only available for rolling.
https://github.com/ament/ament_cmake/blob/a51ed2214ce96fe0ef1ee00455cb03ba14bd52f1/ament_cmake_python/cmake/ament_python_install_package.cmake#L44
https://github.com/ament/ament_cmake/blob/a803f8534312a3b6dbe8a274bf788955d40d0eaa/ament_cmake_python/cmake/ament_python_install_package.cmake#L37

Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Thanks for making the change - I think this looks good to me

@emersonknapp emersonknapp merged commit 55f7f25 into ros-tooling:main Nov 23, 2021
@wep21 wep21 deleted the feature/transform branch November 23, 2021 20:20
@mateusz-lichota
Copy link
Contributor

@emersonknapp @wep21 , the fact that SCRIPTS_DESTINATION is used means that the package doesn't build correctly on galactic. Is that the desired behavior?

@wep21
Copy link
Contributor Author

wep21 commented Nov 26, 2021

@mateusz-lichota I created a PR to address the isseu you suggested. Please check it out. #18

@emersonknapp
Copy link
Contributor

Thanks for the work @wep21 - I have merged that PR

wep21 added a commit to wep21/topic_tools that referenced this pull request Dec 9, 2021
This reverts commit 4151102.

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
wep21 added a commit to wep21/topic_tools that referenced this pull request Oct 8, 2022
This reverts commit 4151102.

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
wep21 added a commit to wep21/topic_tools that referenced this pull request Oct 8, 2022
This reverts commit 4151102.

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
wep21 added a commit that referenced this pull request Oct 20, 2022
This reverts commit 4151102.

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
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