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

Introduce MapN | Deprecate Map With Multiple Callbacks | Static Analysis Checks #112

Merged
merged 7 commits into from
Jun 20, 2021

Conversation

AlexandruGG
Copy link
Collaborator

@AlexandruGG AlexandruGG commented Jun 19, 2021

This PR introduces MapN based on the discussions in #77, and deprecates Map usage with multiple callbacks.

  • MapN code (same as Map but with more mixed type hints)
  • Deprecate Map with multiple callbacks
  • Map stays backwards compatible
  • Tests
  • Static analysis checks
  • Documentation

@AlexandruGG
Copy link
Collaborator Author

@drupol hey, let me know if this looks good to you and I will update the documentation as well

Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi Alex,

Thanks for this PR !
I was about to give the approval but then I had an idea.

Instead of throwing the exception in the Operation class Map, why not throwing it from Collection::map() ?
Wouldn't it be better? WDYT?

@AlexandruGG
Copy link
Collaborator Author

Hi Alex,

Thanks for this PR !
I was about to give the approval but then I had an idea.

Instead of throwing the exception in the Operation class Map, why not throwing it from Collection::map() ?
Wouldn't it be better? WDYT?

Hey, thanks for reviewing!

I considered that, however it's not sufficient because the library provides the Operations to be used separately. So someone that will use the operation directly like this also needs to know about the deprecation if used with multiple callbacks. That's why I also had to update this usage to MapN

@drupol
Copy link
Collaborator

drupol commented Jun 19, 2021

Hi Alex,
Thanks for this PR !
I was about to give the approval but then I had an idea.
Instead of throwing the exception in the Operation class Map, why not throwing it from Collection::map() ?
Wouldn't it be better? WDYT?

Hey, thanks for reviewing!

I considered that, however it's not sufficient because the library provides the Operations to be used separately. So someone that will use the operation directly like this also needs to know about the deprecation if used with multiple callbacks. That's why I also had to update this usage to MapN

You're perfectly right, that was a bad idea :)

Can't wait to be in version 5 to remove the ugly count() in map!

@AlexandruGG AlexandruGG marked this pull request as ready for review June 20, 2021 08:13
@AlexandruGG
Copy link
Collaborator Author

@drupol hey, added documentation now and addressed comments, this should be ready 😁

Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Nothing to add here, all good :)

@drupol drupol merged commit 43806dd into loophp:master Jun 20, 2021
@AlexandruGG AlexandruGG deleted the feature/introduce-mapn branch June 20, 2021 08:17
@AlexandruGG AlexandruGG mentioned this pull request Jun 20, 2021
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.

None yet

2 participants