Add `Reject` operation. #125

drupol · 2021-07-06T15:37:40Z

This PR

Add a new operation Reject.
Has tests
Has documentation
Has static analysis tests

Depends on #126
Related to #123

AlexandruGG · 2021-07-06T21:10:39Z

@drupol if you solve the conflicts here I can give this one a good look as well

drupol · 2021-07-06T21:14:01Z

Rebased and ready to review!

drupol · 2021-07-06T21:34:05Z

tests/static-analysis/reject.php

-reject_checkMap(Collection::fromIterable(['foo' => 'bar', 'baz' => ''])->filter($stringValueCallback, $stringValueCallback2));
-reject_checkMap(Collection::fromIterable(['foo' => 'bar', 'baz' => ''])->filter($stringKeyValueCallback1));
-reject_checkMap(Collection::fromIterable(['foo' => 'bar', 'baz' => ''])->filter($stringKeyValueCallback1, $stringKeyValueCallback2));
+reject_checkList(Collection::fromIterable([1, 2, 3])->reject());


#facepalm ... I should more careful sometimes !

src/Operation/Reject.php

AlexandruGG · 2021-07-06T21:38:46Z

docs/pages/code/operations/reject.php

+$collection = Collection::fromIterable(range(1, 10))
+    ->reject($divisibleBy2, $divisibleBy3); // [1, 5, 7]
+
+$collection = Collection::fromIterable(range(1, 10))
+    ->reject($divisibleBy2)
+    ->reject($divisibleBy3); // [1, 5, 7]


is it just me or because of the nature of reject, calling it with variadic callbacks is the same as calling it multiple times? Acts as logical AND

Yes exactly !

Not "A or B" is "Not A and Not B"

AlexandruGG

This is missing tests 😉

drupol · 2021-07-06T21:47:02Z

This is missing tests wink

Going to do this tomorrow, I'm out for today.

AlexandruGG · 2021-07-06T21:56:46Z

This is missing tests 😉

On this, I was surprised nothing in the CI workflows alerted us about it. But now I see there's nothing in the workflows to cause it to fail if a certain code coverage % is not reached. Would you want to have something like that?

Another two things I noticed looking into the jobs 😄

this is broken

we are running the jobs twice, it would be sufficient to either trigger them on pull_request OR on push

I can create a separate PR to fix number 2, but not too sure about number 1

drupol · 2021-07-07T07:05:25Z

This is missing tests wink

On this, I was surprised nothing in the CI workflows alerted us about it. But now I see there's nothing in the workflows to cause it to fail if a certain code coverage % is not reached. Would you want to have something like that?

Yes good idea and it's quite easy to do, it's already done here: https://github.com/ecphp/cas-bundle/blob/master/grumphp.yml.dist#L9

Another two things I noticed looking into the jobs smile
1. this is broken

Yes this is broken on Windows, and it's "normal". This step is only supposed to be triggered on Linux. This is also why continue-on-error: true is there.

1. we are running the jobs twice, it would be sufficient to either trigger them on `pull_request` OR on `push`

Yeah I think that could be fixed in a separate PR indeed.

AlexandruGG · 2021-07-07T07:38:13Z

On this, I was surprised nothing in the CI workflows alerted us about it. But now I see there's nothing in the workflows to cause it to fail if a certain code coverage % is not reached. Would you want to have something like that?

Yes good idea and it's quite easy to do, it's already done here: https://github.com/ecphp/cas-bundle/blob/master/grumphp.yml.dist#L9

on this, does it mean that the coverage check is already enforced but the level (80%) is too low so that's why it didn't fail?

drupol · 2021-07-07T07:47:14Z

On this, I was surprised nothing in the CI workflows alerted us about it. But now I see there's nothing in the workflows to cause it to fail if a certain code coverage % is not reached. Would you want to have something like that?

Yes good idea and it's quite easy to do, it's already done here: https://github.com/ecphp/cas-bundle/blob/master/grumphp.yml.dist#L9

on this, does it mean that the coverage check is already enforced but the level (80%) is too low so that's why it didn't fail?

That's for ecphp/cas-bundle project 😄 !

AlexandruGG · 2021-07-07T07:49:06Z

On this, I was surprised nothing in the CI workflows alerted us about it. But now I see there's nothing in the workflows to cause it to fail if a certain code coverage % is not reached. Would you want to have something like that?

Yes good idea and it's quite easy to do, it's already done here: https://github.com/ecphp/cas-bundle/blob/master/grumphp.yml.dist#L9

on this, does it mean that the coverage check is already enforced but the level (80%) is too low so that's why it didn't fail?

That's for ecphp/cas-bundle project 😄 !

Oops misread it, I thought it was on the php-conventions project 😄

drupol added the enhancement New feature or request label Jul 6, 2021

This was referenced Jul 6, 2021

Refactor ::partition() and ::span() #124

Merged

Update Filter operation. #126

Merged

drupol force-pushed the feature/add-reject-operation branch from bfa1f7f to cb8ef1e Compare July 6, 2021 17:02

drupol changed the base branch from master to refactor-update-filter-operation July 6, 2021 17:02

drupol force-pushed the feature/add-reject-operation branch from cb8ef1e to c796e59 Compare July 6, 2021 17:06

drupol changed the title ~~Add Reject operation.~~ Add Reject operation. Jul 6, 2021

drupol requested a review from AlexandruGG July 6, 2021 19:54

drupol marked this pull request as ready for review July 6, 2021 19:57

Base automatically changed from refactor-update-filter-operation to master July 6, 2021 21:09

drupol force-pushed the feature/add-reject-operation branch from c796e59 to 2b8a228 Compare July 6, 2021 21:13

drupol commented Jul 6, 2021

View reviewed changes

src/Operation/Reject.php Outdated Show resolved Hide resolved

AlexandruGG force-pushed the feature/add-reject-operation branch from 376f2b8 to 71d0732 Compare July 6, 2021 21:36

AlexandruGG reviewed Jul 6, 2021

View reviewed changes

drupol and others added 7 commits July 7, 2021 08:56

Add Reject operation.

bf09baf

Update documentation.

d9302ce

Add static analysis tests - basically a ripoff of filter.

5ba7ed9

Tweaks

cc03cfe

Fix docs include statements

2f22366

Fix weird spaces

e92d12c

Add forgotten tests.

99c12d3

drupol force-pushed the feature/add-reject-operation branch from 71d0732 to 99c12d3 Compare July 7, 2021 06:56

Extra info in docs

dc32641

AlexandruGG approved these changes Jul 7, 2021

View reviewed changes

drupol merged commit a065b1c into master Jul 7, 2021

drupol deleted the feature/add-reject-operation branch July 7, 2021 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Reject` operation. #125

Add `Reject` operation. #125

drupol commented Jul 6, 2021 •

edited by AlexandruGG

Loading

AlexandruGG commented Jul 6, 2021

drupol commented Jul 6, 2021

drupol Jul 6, 2021

AlexandruGG Jul 6, 2021

drupol Jul 6, 2021

AlexandruGG left a comment

drupol commented Jul 6, 2021

AlexandruGG commented Jul 6, 2021

drupol commented Jul 7, 2021

AlexandruGG commented Jul 7, 2021

drupol commented Jul 7, 2021

AlexandruGG commented Jul 7, 2021

Add Reject operation. #125

Add Reject operation. #125

Conversation

drupol commented Jul 6, 2021 • edited by AlexandruGG Loading

AlexandruGG commented Jul 6, 2021

drupol commented Jul 6, 2021

drupol Jul 6, 2021

Choose a reason for hiding this comment

AlexandruGG Jul 6, 2021

Choose a reason for hiding this comment

drupol Jul 6, 2021

Choose a reason for hiding this comment

AlexandruGG left a comment

Choose a reason for hiding this comment

drupol commented Jul 6, 2021

AlexandruGG commented Jul 6, 2021

drupol commented Jul 7, 2021

AlexandruGG commented Jul 7, 2021

drupol commented Jul 7, 2021

AlexandruGG commented Jul 7, 2021

Add `Reject` operation. #125

Add `Reject` operation. #125

drupol commented Jul 6, 2021 •

edited by AlexandruGG

Loading