search

reactphp / promise

Promises/A implementation for PHP.
https://reactphp.org/promise/
MIT License
2.38k stars 146 forks source link

Rename otherwise to catch and always to finally #206

Closed WyriHaximus closed 2 years ago

WyriHaximus commented 2 years ago

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

clue commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@WyriHaximus My bad, agree :100: 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 commented 2 years ago

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