Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix @throws DocBlock about LogicException#29699
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is a follow-up ofsymfony#28536
ogizanagi commentedDec 27, 2018
Well, logic exceptions aren't even supposed to be documented actually. It makes no sense as it represents a broken code path (either a misconfiguration or developer mistake) and should never be caught. |
| /** | ||
| * Create a new Transport Serializer. | ||
| * | ||
| * @return self |
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.
We tend to avoid adding docs that do not add any additional information (here everything is in the method signature). The same goes for the description above.
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.
Make sense, done.
chalasr commentedDec 29, 2018
I agree with@ogizanagi, such exceptions are thrown to provide a meaningful error instead of a fatal one (class not found). Documenting them would not bring any value. 👎 for me |
fabpot commentedJan 1, 2019
Closing as 2 member of the core voted against this PR. |
gido commentedJan 1, 2019
I understand the point about non documenting LogicException in this case. But the current DocBlock is still wrong (mentions of RuntimeException). Would you accept a PR that remove them? |
fabpot commentedJan 1, 2019
Yes, removing docblocks is always great! |
This is a follow-up of#28536.
\RuntimeExceptionwere replaced by\LogicExceptionbut the DocBlock documentation wasn't updated accordingly.When working on it, I found some inconsistency: In some component the exception raised is from the SPL (
\LogicException) and in other the exception is from the component itself (when theLogicExceptionexist). What is the policy in this case ?