-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
Rename otherwise to catch and always to finally #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
e83bdf9
to
b21fd1f
Compare
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
b21fd1f
to
a937715
Compare
@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? |
@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.) |
@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! |
@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. |
@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! |
@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. |
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