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

Rename --topics-regex #1045

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Rename --topics-regex #1045

merged 1 commit into from
Jul 26, 2022

Conversation

esteve
Copy link
Member

@esteve esteve commented Jul 19, 2022

This PR renames the --topics-regex option in ros2 bag play to --regex (and -e) to match ros2 bag record

@esteve esteve requested a review from a team as a code owner July 19, 2022 12:58
@esteve esteve requested review from emersonknapp and MichaelOrlov and removed request for a team July 19, 2022 12:58
@esteve
Renamed --topics-regex to --regex and -e
c93d3f8 
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve
Copy link
Member Author

esteve commented Jul 19, 2022

@MichaelOrlov this is the PR we talked about in #1039 However, ros2 bag record --regex takes only one regex, but this PR and --topics-regex may take multiple regexes. Would it make sense to make play and record behave the same? I don't know to what extent it's important to make both verbs behave similarly, but as a user, it'd be convenient. Same goes for other options in play/record

@esteve
Copy link
Member Author

esteve commented Jul 21, 2022

CI is failing, but I think it's unrelated to these changes, from what I've seen it's a test for services with fastrtps

MichaelOrlov
MichaelOrlov approved these changes Jul 24, 2022
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

LGTM.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 26, 2022 •
edited
Loading

Running CI:
Gist: https://gist.githubusercontent.com/MichaelOrlov/66e56242beda9e74164ce1dda766ba23/raw/5221865698cd60d7a3af1c24bc76661dc14c5328/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10573

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

@MichaelOrlov MichaelOrlov merged commit 210dc61 into ros2:rolling Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@emersonknapp emersonknapp Awaiting requested review from emersonknapp emersonknapp is a code owner automatically assigned from ros2/tooling-reviewers

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

Successfully merging this pull request may close these issues.
2 participants
@esteve @MichaelOrlov