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

[image_geometry] Migrated to ROS2 #260

Open
wants to merge 8 commits into
base: rolling
Choose a base branch
from

Conversation

akhileshmoghe
Copy link

Changes include:

  1. Enabled pytests in image_geometry package.
  2. Code modified for ROS2 colcon build & test errors, all tests passed.
  3. Cosmetic tests - test_copyright, test_pep257, test_flake8 passed.
  4. Merged Approved changes from PR [image_geometry] Enable python package installation #257.

@mjcarroll
Please review this PR, Thanks.

@akhileshmoghe
Copy link
Author

@gaoethan Will you please review this PR?
I have tested these changes on Crystal release also, all above mentioned tests have passed.

@mjcarroll
Copy link
Contributor

@akhileshmoghe Can you go ahead and rebase this on current ros2 branch and I'll kick off CI, thanks.

Lalit Begani and others added 4 commits January 14, 2019 16:22
@akhileshmoghe
Copy link
Author

Hi @mjcarroll
I have resolved the conflicts and the build error introduced after accepting PR #257 changes.
Kindly review changes.

@mjcarroll mjcarroll self-assigned this Jul 10, 2019
@mjcarroll mjcarroll added the ros2 label Jul 10, 2019
@klintan
Copy link

klintan commented Nov 2, 2019

Looks good to me, we have some duplicated effort and concerns/thoughts here:

#295

Copy link
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

Sorry @akhileshmoghe I have not been the maintainer of this package for a long time and don't have authority here.
I believe @mjcarroll can review or point to the current maintainer of this.


It seems that a much younger PR with essentially the same source changes got merged as pointed out by @klintan

Here's a review based on what I remember from the time of the original ROS 2 port:
In the end I think only the necessary code changes in directed.py should be kept and rebased on top of the latest state, the rest of the changes seem IMHO unnecessary or unwanted

  • style was not updated when porting from ROS 1 to keep the diff with the ROS 1 branch minimal. Following that logic, the linter tests and style changes should be removed from this PR
  • The copyright notice and license changes to the existing files seems wrong. These files were written before 2018, also before OSRF existed and under a different license (most likely BSD).
  • The content of the migration readme and PR title is misleading, as this package's C++, package.xml, CMake etc was already ported to ROS 2. At the end of the day, this PR is only updating message field names which I believe doesn't need a dedicated README

@ijnek ijnek mentioned this pull request Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants