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

Add filter verb #864

Closed
wants to merge 1 commit into from
Closed

Add filter verb #864

wants to merge 1 commit into from

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented Sep 4, 2021

I added filter verb to filter topics with python expression.

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 requested a review from a team as a code owner September 4, 2021 18:35
@wep21 wep21 requested review from emersonknapp and prajakta-gokhale and removed request for a team September 4, 2021 18:35
@wep21
Copy link
Contributor Author

wep21 commented Sep 6, 2021

@emersonknapp Could you review this?

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

While I think this is a useful feature - I think that it can be built a little more generically.

@gbiggs is currently working on #831. This ros2 bag convert feature could take the filter as an extra argument, rather than having a whole separate verb ros2 bag filter.

I would like to hold off on reviewing this PR until we have that base feature in place, so that we could build this as a simple extension to it. Because of this, I won't leave my full comments here (I would have commented on hardcoding cdr for the StorageOptions, for example. And I would say that we need tests, and control over the StorageOptions for the output bag. However, if the generic feature is built first, then this won't have to worry about most of that)

I'll try to see about the timeline on that feature. To accelerate development we may put in the singular bag->bag version first, then move on to the multi-bag implementation.

@wep21
Copy link
Contributor Author

wep21 commented Dec 8, 2021

Closed by #921.

@wep21 wep21 closed this Dec 8, 2021
@emersonknapp
Copy link
Collaborator

emersonknapp commented Dec 8, 2021

A note that #921 doesn't support arbitrary Python expression. It supports "filtering" in the sense of choosing which topic names to forward.

This PR is more related to #885, which is still open at this point.

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.

2 participants