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.

Sign up for GitHub

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

Jump to bottom

Add Reduce operation #139

Merged
merged 6 commits into from
Jul 19, 2021
Merged

Add Reduce operation #139

merged 6 commits into from
Jul 19, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jul 11, 2021 •
edited
Loading

This PR:

  • Provide a new operation: Reduce
  • Has unit tests (phpspec)
  • Has static analysis tests (psalm, phpstan)
  • Has documentation
  • Have proper static typing

@drupol drupol requested a review from AlexandruGG July 11, 2021 21:29
@drupol drupol self-assigned this Jul 11, 2021
@drupol drupol mentioned this pull request Jul 11, 2021
Merged
2 tasks
@drupol drupol marked this pull request as ready for review July 12, 2021 07:23
@drupol drupol changed the title Add reduce operation Add Reduce operation Jul 12, 2021
@drupol drupol force-pushed the feature/add-reduce-operation branch from ca6c295 to 173f6ce Compare July 12, 2021 12:14
@drupol drupol removed their assignment Jul 12, 2021
@drupol drupol force-pushed the feature/add-reduce-operation branch 3 times, most recently from b3ce099 to 0993829 Compare July 12, 2021 19:38
@drupol drupol force-pushed the feature/add-reduce-operation branch 7 times, most recently from f5e5706 to afed0de Compare July 17, 2021 15:45
@drupol
Copy link
Collaborator Author

drupol commented Jul 17, 2021

@AlexandruGG Do you think the static analysis stuff is good enough?

@drupol drupol force-pushed the feature/add-reduce-operation branch 3 times, most recently from bc3e1fc to a7cbb83 Compare July 17, 2021 16:37
@drupol drupol marked this pull request as draft July 17, 2021 16:38
@drupol drupol force-pushed the feature/add-reduce-operation branch 3 times, most recently from 7b673a6 to 56ffc34 Compare July 17, 2021 16:51
@drupol drupol force-pushed the feature/add-reduce-operation branch from e7c651e to 09119ff Compare July 18, 2021 17:11
@drupol
Copy link
Collaborator Author

drupol commented Jul 18, 2021

I'm going to create separate PR's, it's too messy in here :) this PR was only supposed for adding the Reduce operation.

@drupol drupol force-pushed the feature/add-reduce-operation branch 2 times, most recently from 652d7d2 to dfd764d Compare July 18, 2021 17:54
@AlexandruGG
Copy link
Collaborator

I'm going to create separate PR's, it's too messy in here :) this PR was only supposed for adding the Reduce operation.

Yes please 🙌

@drupol
Copy link
Collaborator Author

drupol commented Jul 18, 2021

I'm going to create separate PR's, it's too messy in here :) this PR was only supposed for adding the Reduce operation.

Yes please 🙌

Everything's done already !

@drupol drupol force-pushed the feature/add-reduce-operation branch from dfd764d to e744778 Compare July 19, 2021 07:28
@AlexandruGG AlexandruGG force-pushed the feature/add-reduce-operation branch from d7142f0 to 2d111f7 Compare July 19, 2021 17:39
@drupol
Copy link
Collaborator Author

drupol commented Jul 19, 2021 •
edited
Loading

Oops, looks like you added back all the commits I removed yesterday :S (see #153)

@AlexandruGG AlexandruGG force-pushed the feature/add-reduce-operation branch from 2d111f7 to 670f52f Compare July 19, 2021 17:41
@AlexandruGG
Copy link
Collaborator

Oops, looks like you added back all the commits I removed yesterday :S (see #153)

Yeah I just noticed, I had your branch already locally 😓

@AlexandruGG
Copy link
Collaborator

Oops, looks like you added back all the commits I removed yesterday :S (see #153)

Yeah I just noticed, I had your branch already locally 😓

Sorry, can you fix it? There's only one commit I added (other than the master merge)

@drupol
Copy link
Collaborator Author

drupol commented Jul 19, 2021

Ok going to fix it.

@drupol drupol force-pushed the feature/add-reduce-operation branch from 670f52f to 847f611 Compare July 19, 2021 17:50
@drupol drupol force-pushed the feature/add-reduce-operation branch from 847f611 to 34fa160 Compare July 19, 2021 17:50
@drupol
Copy link
Collaborator Author

drupol commented Jul 19, 2021

Job done.

To get it locally:

git fetch --all
git checkout feature/add-reduce-operation
git reset origin/feature/add-reduce-operation --hard

AlexandruGG
AlexandruGG approved these changes Jul 19, 2021
View reviewed changes
Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great 🎉

For the static analysis tests I always try to add some "failing" to ensure that the constraints we set work and make sense.

In this case the main constraint is that if the user doesn't provide an initial value then the default null will be used, which means the callback needs to be able to accept null as the carry 😁 .

Also when the initial value is provided then the collection will not contain null if the callback doesn't return null

@drupol drupol merged commit 18a68c4 into master Jul 19, 2021
@drupol drupol deleted the feature/add-reduce-operation branch July 19, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexandruGG AlexandruGG AlexandruGG approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@drupol @AlexandruGG