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

Rename otherwise to catch and always to finally #206

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Jan 14, 2022

Due to limitations in the PHP language these two methods couldn't use keywords as names. With PHP 7+ this is possible, and it makes it a lot clearer what the methods do.

Refs: #19

@WyriHaximus WyriHaximus added this to the v3.0.0 milestone Jan 14, 2022
@WyriHaximus WyriHaximus requested review from jsor, cboden and clue January 14, 2022 16:45
Copy link
Member

@jsor jsor left a comment

Choose a reason for hiding this comment

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

🎉

@WyriHaximus WyriHaximus force-pushed the rename-otherwise-to-catch-and-always-to-finally branch from e83bdf9 to b21fd1f Compare January 17, 2022 11:21
Due to limitations in the PHP language these two methods couldn't use keywords as names. With PHP 7+ this is possible, and it makes it a lot clearer what the methods do.

Refs: reactphp#19
@WyriHaximus WyriHaximus force-pushed the rename-otherwise-to-catch-and-always-to-finally branch from b21fd1f to a937715 Compare January 17, 2022 11:22
@clue
Copy link
Member

clue commented Jan 23, 2022

@WyriHaximus Thanks for looking into this, the changes makes perfect sense to me! This PR doesn't allow edits, so I've just filed #208 that builds on top of this but adds additional documentation and tests to keep 100% code coverage of the affected code paths. WDYT?

@WyriHaximus
Copy link
Member Author

@clue I'd rather have you ping me next time so I can cherry pick your commit into this PR. But this route also works for this time. Will make sure I have the allow edits by maintainers checkbox enabled. (Which is should be interestingly enough.)

@clue
Copy link
Member

clue commented Jan 23, 2022

@WyriHaximus Fair enough, will do next time, sorry for the confusion! The diff between both PRs also turned out to be rather big (much bigger than I originally expected) due to the duplicate tests, so I figured it could make sense to take a look at both and see which approach makes most sense. In either case, thanks for kicking this off!

@WyriHaximus
Copy link
Member Author

@clue Another option would have been to take my commit and append it with your changes. But in a broader sense, it's about having everyone that worked on a feature/fix/maintenance on the changelog/release, not just the person filing the PR that got in.

@clue
Copy link
Member

clue commented Jan 23, 2022

@WyriHaximus My bad, agree 💯 Credit where credit is due, my PR should have been two commits at the very least. I don't think there's a reasonable way to revert the merge, but happy to include you as part of the release notes for this PR!

@WyriHaximus
Copy link
Member Author

@clue I don't expect a revert, I approved and merged it myself. If I expected a revert I wouldn't have done that 😉 . But I do think we can try and see if doing this with more PR's/contributions makes sense.

@clue clue removed this from the v3.0.0 milestone Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants