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 \Throwable exclusively in rejection function docs #142

Merged
merged 2 commits into from
Oct 7, 2019

Conversation

jsor
Copy link
Member

@jsor jsor commented May 9, 2019

Follow-up for #138.

@WyriHaximus WyriHaximus added this to the v3.0.0 milestone May 9, 2019
@WyriHaximus WyriHaximus requested review from WyriHaximus and clue May 9, 2019 18:12
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

The changeset looks reasonable to me, but just one question: Is is common knowledge that Throwable is the base of Exception or should we perhaps make this more explicit? It's my understanding most common use cases would likely reject a promise with a subclass of Exception such as RuntimeException (and family) and it might make sense to keep this as a search keyword in the documentation?

@jsor
Copy link
Member Author

jsor commented May 9, 2019

Good point, i will try to rework the docs 👍 . But we must make sure to not recommend \Exception as type-declarations in callbacks. Used eg. in otherwise() callbacks, this might lead to uncaught \Error's.

@clue
Copy link
Member

clue commented May 9, 2019

I concur, adding Throwable as the type hint and adding explicit docs for Exception seems to be the best solution here 👍

@WyriHaximus
Copy link
Member

@clue added a short paragraph about Throwable vs Exception.

@WyriHaximus WyriHaximus force-pushed the throwable-rejection-readme-fixes branch from c1a75d8 to a747ad4 Compare October 6, 2019 19:42
@WyriHaximus WyriHaximus force-pushed the throwable-rejection-readme-fixes branch from a747ad4 to 6a45c24 Compare October 6, 2019 20:07
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Thanks for the update, changes LGTM! 💯

@WyriHaximus
Copy link
Member

🎉 !

@jsor jsor merged commit e49defd into reactphp:master Oct 7, 2019
@jsor jsor deleted the throwable-rejection-readme-fixes branch October 8, 2019 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants