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

Update Filter operation. #126

Merged
merged 4 commits into from
Jul 6, 2021
Merged

Update Filter operation. #126

merged 4 commits into from
Jul 6, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jul 6, 2021

All the operations having variadic arguments evaluates as logical OR, the filter operation should also follow that rule.

BREAKING CHANGE: yes

This PR

  • Update the Filter operation
  • Has updated documentation
  • Has updated tests

Related to #123 and #125

BREAKING CHANGE: yes
@drupol drupol force-pushed the refactor-update-filter-operation branch from 330bb4f to 2be47c8 Compare July 6, 2021 16:10
@drupol drupol requested a review from AlexandruGG July 6, 2021 16:10
@drupol drupol marked this pull request as ready for review July 6, 2021 16:11
@drupol drupol mentioned this pull request Jul 6, 2021
4 tasks
@drupol drupol added the enhancement New feature or request label Jul 6, 2021
@drupol drupol changed the title Update Filter operation. Update Filter operation. Jul 6, 2021
*/
static function (Iterator $iterator) use ($callbacks): Iterator {
static function (Iterator $iterator) use ($callbacks): Generator {
// TODO: Find a way to avoid repeating this everywhere.
Copy link
Collaborator

@AlexandruGG AlexandruGG Jul 6, 2021

Choose a reason for hiding this comment

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

I just noticed this pattern in the other operations now that you commented this 😄.

This can't be an operation by itself so where could we put it 🤔. Maybe in AbstractOperation or on a trait?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dunno yet, but it's going to be a follow up for sure.

Copy link
Collaborator

@AlexandruGG AlexandruGG Jul 6, 2021

Choose a reason for hiding this comment

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

I was going to bring up the fact that we should aim to look at solving some of the TODOs in the codebase 😁

Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment

Choose a reason for hiding this comment

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

Given the BC break is this going out in 4.2?

@drupol drupol merged commit 7e2e39b into master Jul 6, 2021
@drupol drupol deleted the refactor-update-filter-operation branch July 6, 2021 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants