Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@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
# for free to join this conversation on GitHub. Already have an account? # 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