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

Use new keywords #231

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Use new keywords #231

merged 1 commit into from
Jan 31, 2022

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jan 6, 2022

Motivation

There are multiple goals behind this PR.

First, is to get rid of AbstractOperation. Its sole purpose is to provide the static method of, which is just syntactic sugar.
Syntactic sugar is cool, but it's not needed in Collection. We could gain some performance by completely removing them.

Then, by removing it and instantiating operation using new could also be a good thing for static analysis and types inference.

Note

When dealing with methods having the @pure annotation, using the keyword new to create operation will trigger an ImpureFunctionCall in PSalm.

However, when having nested functions in those methods, using the new keyword, will not trigger an ImpureFunctionCall in PSalm.

This PR:

  • Use the new keyword instead of using the static method of().
  • PSalm score is now 97% 🎉

@drupol drupol self-assigned this Jan 6, 2022
@drupol drupol force-pushed the feat/use-new-keyword branch 3 times, most recently from 329cbd2 to 323b4c8 Compare January 6, 2022 21:24
@drupol drupol marked this pull request as ready for review January 7, 2022 10:32
@drupol drupol removed their assignment Jan 7, 2022
@drupol drupol added the enhancement New feature or request label Jan 7, 2022
@github-actions github-actions bot added the stale label Jan 12, 2022
@loophp loophp deleted a comment from github-actions bot Jan 12, 2022
@drupol drupol removed the stale label Jan 12, 2022
@drupol drupol requested a review from AlexandruGG January 14, 2022 08:21
AlexandruGG
AlexandruGG previously approved these changes Jan 16, 2022
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.

Nice 👍 . I like the idea of this.

All looks good except for the Unpack changes. Perhaps you could leave those for a separate PR and address the comment?

@drupol
Copy link
Collaborator Author

drupol commented Jan 17, 2022

Nice 👍 . I like the idea of this.

All looks good except for the Unpack changes. Perhaps you could leave those for a separate PR and address the comment?

Done in this PR.

@github-actions
Copy link

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Jan 23, 2022
@AlexandruGG AlexandruGG removed the stale label Jan 23, 2022
@drupol drupol force-pushed the feat/use-new-keyword branch 8 times, most recently from cb12ad6 to 6f82a91 Compare January 30, 2022 21:20
@drupol drupol force-pushed the feat/use-new-keyword branch from 6f82a91 to cad1141 Compare January 30, 2022 21:29
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.

💯

@drupol drupol merged commit 82fcea1 into master Jan 31, 2022
@drupol drupol deleted the feat/use-new-keyword branch January 31, 2022 11:38
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