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

Support filtering the messages on a receiver #303

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

shsms
Copy link
Contributor

@shsms shsms commented Jun 27, 2024

A new filter method is added to the Receiver interface, which allows the application of a filter function on the messages on a receiver.

Example:

async for message in receiver.filter(lambda num: num % 2):
    print(f"An even number: {message}")

@shsms shsms requested a review from a team as a code owner June 27, 2024 13:12
@shsms shsms requested a review from llucax June 27, 2024 13:12
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) labels Jun 27, 2024
@llucax
Copy link
Contributor

llucax commented Jun 28, 2024

What about adding an async_filter to the new core repo instead? This would be useful with any async iterator. I would also remove map() and add an async_map() to core.

@llucax
Copy link
Contributor

llucax commented Jun 28, 2024

There is also https://snyk.io/advisor/python/asyncstdlib. Maybe we should start using that? It is strange that this is not part of Python stdlib already.

@llucax
Copy link
Contributor

llucax commented Jun 28, 2024

@llucax
Copy link
Contributor

llucax commented Jun 28, 2024

If we agree it is a good idea we could add it to repo-config so it is used by default, like we do with typing_extensions.

@shsms
Copy link
Contributor Author

shsms commented Jun 28, 2024

What about adding an async_filter to the new core repo instead? This would be useful with any async iterator. I would also remove map() and add an async_map() to core.

This gives out Receivers, we can do all the things we can do with receivers with them. Maybe we can have an additional async_filter or use asyncstd, but having it here is still valuable.

@shsms
Copy link
Contributor Author

shsms commented Jun 28, 2024

If we agree it is a good idea we could add it to repo-config so it is used by default, like we do with typing_extensions.

I do agree, it would be very useful, but I don't think it can replace this PR, because the filter in this PR returns receivers, which we can put in select or any of the many possibilities.

@llucax
Copy link
Contributor

llucax commented Jun 28, 2024

Right. OK, and I guess it is useful to get a receiver because it can be used in select(). It came to my mind a couple of times that maybe we should have a utility receiver that converts any async iterator into a receiver. So we could potentially do:

filtered_receiver = as_receiver(a.filter(lambda num: num % 2), original_receiver)
async for selected in select(filtered_receiver, ...):
    print(f"An even number: {message}")

But if we want to move in that direction, we'll eventually have to remove map() too, and that will be a breaking change, so I'm happy to leave that for 2.0 and add filter now if it is useful.

Will start the review now then...

@llucax
Copy link
Contributor

llucax commented Jun 28, 2024

What about adding an async_filter to the new core repo instead? This would be useful with any async iterator. I would also remove map() and add an async_map() to core.

This gives out Receivers, we can do all the things we can do with receivers with them. Maybe we can have an additional async_filter or use asyncstd, but having it here is still valuable.

src/frequenz/channels/_receiver.py Show resolved Hide resolved
src/frequenz/channels/_receiver.py Outdated Show resolved Hide resolved
@shsms shsms force-pushed the filter branch 2 times, most recently from 3becd73 to e72d19c Compare July 2, 2024 09:48
@shsms
Copy link
Contributor Author

shsms commented Jul 2, 2024

Based on #301

@shsms shsms marked this pull request as draft July 2, 2024 09:49
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

LGTM. Not approving until it's not a draft anymore.

@llucax
Copy link
Contributor

llucax commented Jul 3, 2024

Also tests are failing:

src/frequenz/channels/_receiver.py:451:101: E501 line too long (138 > 100 characters)

shsms added 3 commits July 3, 2024 14:45
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms shsms marked this pull request as ready for review July 3, 2024 12:47
@shsms
Copy link
Contributor Author

shsms commented Jul 3, 2024

Fixed the long line

@shsms shsms requested a review from llucax July 3, 2024 12:47
@shsms shsms added this pull request to the merge queue Jul 3, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 779e807 Jul 3, 2024
14 checks passed
@shsms shsms deleted the filter branch July 3, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants