search

ros2 / demos

Apache License 2.0
502 stars 330 forks source link

Added README.md for image_tools #571

Closed cardboardcode closed 1 year ago

cardboardcode commented 2 years ago

Purpose of Pull Request

This pull request adds a README.md to image_tools as part of efforts to address the issue highlighted in https://github.com/ros2/demos/issues/531.

Edits Made:

  1. Added instructional README.md under /image_tools directory.
  2. Added supplementary .png picture for verifying that the ROS2 package works.
cardboardcode commented 1 year ago

Apologies for the long delay. I now have the bandwidth to see this Pull Request through now that I am on holiday.

Will respond to requests to edit/improve the added README.md promptly.

cardboardcode commented 1 year ago

Hi @adityapande-1995 and @clalancette, thank you for the prompt review.

I have updated the current README.md draft based on your feedbacks in the following commits: 1 . b33a13c 2 . 9dad8f4

However, I have also further edited the draft to adhere to the style of instructions similar to the approved Pull Request 572.

Enquiries

  1. May I request for another quick 5-minute review?

  2. Would it be okay for me to force-push the commits as instructed by the DCO bot since it seems the signed commits are failing the checks?

clalancette commented 1 year ago

  • Would it be okay for me to force-push the commits as instructed by the DCO bot since it seems the signed commits are failing the checks?

Yes, force-pushing is fine; it is pretty common practice, exactly to fix the DCO type issues.

cardboardcode commented 1 year ago

I think that this PR should be rebased on a newly pulled rolling branch. That way, the PR doesn't have a bunch of unrelated commits.

Will do. @audrow Should I create a new Pull Request for this added README.md?

Asking because it seems it is not possible to rebase it cleanly in the current Pull Request.

audrow commented 1 year ago

I think that this PR should be rebased on a newly pulled rolling branch. That way, the PR doesn't have a bunch of unrelated commits.

Will do. @audrow Should I create a new Pull Request for this added README.md?

Asking because it seems it is not possible to rebase it cleanly in the current Pull Request.

That sounds okay to me. You can mention this PR in the new PR so that it's easier to trace the feedback.

I think it would also be possible to start from the rolling branch, cherry-pick your commits here on to the rolling branch, and then force push to this branch, but whatever you think is easier.

cardboardcode commented 1 year ago

Created a new pull request with a cleaner set of commits that has been rebased from the updated rolling branch.

Closing. Please refer to Pull Request https://github.com/ros2/demos/pull/596 for continuation of this thread.