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

Rename OnlyIfPrevious to WithPrevious #357

Merged
merged 6 commits into from
Dec 4, 2024

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Dec 4, 2024

The new name makes is more clear that this predicate is adding an extra condition to the message filtering based on the previous message.

Fixes #355.

@llucax llucax requested a review from a team as a code owner December 4, 2024 12:00
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:experimental Affects the experimental package labels Dec 4, 2024
@llucax llucax self-assigned this Dec 4, 2024
@llucax llucax requested a review from shsms December 4, 2024 12:01
@llucax llucax added the type:enhancement New feature or enhancement visitble to users label Dec 4, 2024
@llucax llucax enabled auto-merge December 4, 2024 12:01
@llucax llucax disabled auto-merge December 4, 2024 12:02
…evious one"

With the upcoming name change of `OnlyIfPrevious` to `WithPrevious`, it
doesn't look like `filter(WithPrevious(lambda prev, new:
prev != new))` is so ugly and verbose to justify having `ChangedOnly` as
a special case. Specially since the name is a bit confusing, and not
clear enough about what it does. The verbose alternative make it more
clear that it's comparing to the previous message.

This reverts commit 2b9541c.

Signed-off-by: Leandro Lucarella <[email protected]>
The new name makes is more clear that this predicate is adding an
extra condition to the message filtering based on the previous
message.

Signed-off-by: Leandro Lucarella <[email protected]>
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

Just a cosmetic about documentation, LGTM

Make it more clear that `WithPrevious` is a composable predicate, to
build new predicates.

Signed-off-by: Leandro Lucarella <[email protected]>
Now that `ChangedOnly` was removed it makes more sense to use that as an
example instead of a more obscure example using `is` for comparison.

Signed-off-by: Leandro Lucarella <[email protected]>
This matches the `filter()` method.

Signed-off-by: Leandro Lucarella <[email protected]>
We don't plan to add more predicates for now, so we rename
`_predicates.py` to `_with_previous.py` and
`test_predicates_only_if_previous.py` to `test_with_previous.py`.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax added this pull request to the merge queue Dec 4, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 56ae688 Dec 4, 2024
14 checks passed
@llucax llucax deleted the with-previous branch December 4, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:docs Affects the documentation part:experimental Affects the experimental package part:tests Affects the unit, integration and performance (benchmarks) tests type:enhancement New feature or enhancement visitble to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename OnlyIfPrevious as WithPrevious
2 participants