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.

Sign up for GitHub

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

Jump to bottom

NodeStrategy supports node name argument. #941

Merged
merged 3 commits into from
Nov 7, 2024
Merged

NodeStrategy supports node name argument. #941

merged 3 commits into from
Nov 7, 2024

Conversation

fujitatomoya
Copy link
Collaborator

fujitatomoya
fujitatomoya commented Oct 21, 2024
View reviewed changes
Copy link
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

Since this is not tested yet, i will keep this as draft.

but if you have any objections to have node_name argument in NodeStrategy, please share your thoughts. I do not really see the downside for this parameter exntension.

@fujitatomoya fujitatomoya marked this pull request as ready for review October 25, 2024 20:19
@fujitatomoya fujitatomoya mentioned this pull request Oct 25, 2024
Closed
@fujitatomoya
Copy link
Collaborator Author

Pulls: #941
Gist: https://gist.githubusercontent.com/fujitatomoya/81fe8695f0df1f4dde75155a6d12ba30/raw/b83bb9f7cc34fe9fd1a22c17bac34b2dfd316e54/ros2.repos
BUILD args: --packages-above-and-dependencies ros2cli ros2topic
TEST args: --packages-above ros2cli ros2topic
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14732

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

@clalancette @ahcorde can you review this? i do not think there is any downside for this.

clalancette
clalancette requested changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I think this is fine. I've left one wording improvement.

ros2topic/ros2topic/verb/echo.py Outdated Show resolved Hide resolved
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/node-strategy-node-name branch from b296e3c to c4f7159 Compare November 6, 2024 18:46
@fujitatomoya
Copy link
Collaborator Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

@clalancette thanks for review!

@fujitatomoya fujitatomoya merged commit 775ea35 into rolling Nov 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@audrow audrow Awaiting requested review from audrow audrow is a code owner

@gbiggs gbiggs Awaiting requested review from gbiggs gbiggs is a code owner

@ahcorde ahcorde Awaiting requested review from ahcorde

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assign node name while using ros2 topic echo
2 participants
@fujitatomoya @clalancette