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 Distinct operation #111

Merged
merged 2 commits into from
Jun 23, 2021
Merged

Update Distinct operation #111

merged 2 commits into from
Jun 23, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jun 16, 2021

This PR

  • Update the Distinct operation
  • Has an updated documentation
  • Is backward compatible
  • Has updated tests
  • Has static tests

Fixes #110

src/Collection.php Outdated Show resolved Hide resolved
@AlexandruGG
Copy link
Collaborator

here's how to fix the phpstan issue: https://phpstan.org/r/a5fbb74a-9e50-401a-b11e-05d96cc477db

@drupol drupol force-pushed the 110-update-distinct-operation branch 2 times, most recently from 8a1f24e to d615b61 Compare June 20, 2021 09:03
@drupol
Copy link
Collaborator Author

drupol commented Jun 20, 2021

Do you have an idea on how I could fix the annotations here @AlexandruGG ?

@drupol drupol force-pushed the 110-update-distinct-operation branch from d615b61 to 8fe5e15 Compare June 21, 2021 20:02
@AlexandruGG
Copy link
Collaborator

Do you have an idea on how I could fix the annotations here @AlexandruGG ?

Hey sorry I only saw this now. If everything else is done leave this to me I can have a look and try to get it ready tomorrow

@drupol
Copy link
Collaborator Author

drupol commented Jun 21, 2021

Sure, no hurry!

AlexandruGG
AlexandruGG previously approved these changes Jun 22, 2021
@AlexandruGG
Copy link
Collaborator

AlexandruGG commented Jun 22, 2021

@drupol this is sorted now, have a look at my last commit c0fca86 to see the changes done.

Maybe we can have a chat sometime so I can explain how I approach the static analysis checks to ensure that we get the types right and that we protect against incorrect usage (otherwise failures will only be detected by tests / at runtime!). It can be tricky with all the closures sometimes though 😅

@drupol
Copy link
Collaborator Author

drupol commented Jun 22, 2021

This, is my current state of mind:

Image

Going to read this carefully and learn! :)

@AlexandruGG
Copy link
Collaborator

This, is my current state of mind:

Image

Going to read this carefully and learn! :)

😄 that was legitimately my feeling when I first found your library and looked into the code! I had never worked with so many closures and currying in PHP!

@drupol
Copy link
Collaborator Author

drupol commented Jun 23, 2021

smile that was legitimately my feeling when I first found your library and looked into the code! I had never worked with so many closures and currying in PHP!

Haha thanks mate! Glad you like it and that you learned something! That's the spirit :)

@drupol drupol force-pushed the 110-update-distinct-operation branch from c0fca86 to 77201ba Compare June 23, 2021 06:38
@drupol drupol marked this pull request as ready for review June 23, 2021 06:38
@drupol drupol merged commit 4b0f81b into master Jun 23, 2021
@drupol drupol deleted the 110-update-distinct-operation branch June 23, 2021 06:41
@drupol
Copy link
Collaborator Author

drupol commented Jun 23, 2021

Maybe we can have a chat sometime so I can explain how I approach the static analysis checks to ensure that we get the types right and that we protect against incorrect usage (otherwise failures will only be detected by tests / at runtime!). It can be tricky with all the closures sometimes though sweat_smile

Great idea, anytime !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let users customize how to access a value in the Distinct operation.
2 participants