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

Added support for excluding topics via regular expressions #1046

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

esteve
Copy link
Member

@esteve esteve commented Jul 19, 2022

This PR adds --exclude (and -x) as an option to ros2 bag play to exclude topics from being replayed.

@esteve esteve requested a review from a team as a code owner July 19, 2022 13:46
@esteve esteve requested review from gbiggs and jhdcs and removed request for a team July 19, 2022 13:46
Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

I'm not seeing any immediately obvious issues.

@esteve
Copy link
Member Author

esteve commented Jul 21, 2022

@jhdcs thanks for the prompt review. One thing that differs from ros2 bag record --exclude is that this PR accepts multiple regular expressions. Would it be better to keep it consistent and only accept one? It's the same case as with ros2 bag play --regex

@jhdcs
Copy link
Contributor

jhdcs commented Jul 22, 2022

I feel like keeping it consistent would be best, at least for the moment. If in future we decide that we need to have multiple regular expression support, we can open a new issue at that time and change everything over.

@esteve esteve marked this pull request as draft August 2, 2022 14:02
@esteve esteve marked this pull request as ready for review August 3, 2022 10:51
@esteve
Copy link
Member Author

esteve commented Aug 3, 2022

@jhdcs I've updated this PR so that ros2 bag play --exclude only takes one regex, so that it matches the behavior of ros2 bag record --exclude

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/4ddd09a6f289164fd88305e18d2c6c78/raw/06ed49f0a9fbb6fc94981ef7e363abac755edc6f/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10634

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

@esteve
Copy link
Member Author

esteve commented Sep 5, 2022

@jhdcs @MichaelOrlov the CI issues don't seem to be related to the changes in this PR, but I could be wrong. Did they happen in previous PRs?

@MichaelOrlov
Copy link
Contributor

@esteve Yes this is known issue and unrelated to the changes from this PR.
I am sorry I was on vacation and forgot to merge this PR.

@MichaelOrlov MichaelOrlov merged commit 8a77246 into ros2:rolling Sep 7, 2022
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