Skip to content

Rename otherwise to catch and always to finally - #206

Closed
WyriHaximus wants to merge 1 commit into
reactphp:masterfrom
WyriHaximus-secret-labs:rename-otherwise-to-catch-and-always-to-finally
Closed

Rename otherwise to catch and always to finally#206
WyriHaximus wants to merge 1 commit into
reactphp:masterfrom
WyriHaximus-secret-labs:rename-otherwise-to-catch-and-always-to-finally

Conversation

@WyriHaximus

@WyriHaximusWyriHaximus commented Jan 14, 2022

Copy link
Copy Markdown
Member

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

@WyriHaximusWyriHaximus added this to the v3.0.0 milestone Jan 14, 2022

@jsorjsor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎉

@WyriHaximus
WyriHaximusforce-pushed the rename-otherwise-to-catch-and-always-to-finally branch from e83bdf9 to b21fd1fCompareJanuary 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
WyriHaximusforce-pushed the rename-otherwise-to-catch-and-always-to-finally branch from b21fd1f to a937715CompareJanuary 17, 2022 11:22
@clue

clue commented Jan 23, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
MemberAuthor

@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

clue commented Jan 23, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
MemberAuthor

@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

clue commented Jan 23, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
MemberAuthor

@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.

@clueclue removed this from the v3.0.0 milestone Aug 31, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@WyriHaximus@clue@jsor