Adds support for fixing missing throws doc block #7994
Conversation
orklah
added
the
release:feature
|
Cool! That looks nice! If you target 4.x, you must keep compatibility with PHP 7.1 though. Can you check this and the other CI failure? Could you also add a test case with two different exceptions, to check the docblock rendering? |
| '<?php | ||
| /** | ||
| * @throws InvalidArgumentException|DomainException | ||
| * @throws Exception |
There was a problem hiding this comment.
i preserve the existing throws annotation and add a new one based on inference
There was a problem hiding this comment.
So the fix will always add a new throws annotation which can have Exception1|Exception2 syntax, but not modify existing one
| @@ -719,6 +725,8 @@ public function analyze( | |||
| } | |||
|
|
|||
| if (!$is_expected) { | |||
| assert(class_exists($possibly_thrown_exception)); | |||
There was a problem hiding this comment.
should i keep this assert or remove it, it makes sense for throws keyword to only contain classes which exist
There was a problem hiding this comment.
ideally this check would not be needed, but that means modifying the return type for the function getUncaughtThrows which would be a bigger change
There was a problem hiding this comment.
Please drop it. It's risky because classes could be stubbed or inserted through a Plugin so you can't guarantee they'll exists for Psalm to autoload
| ['MissingThrowsDocblock'], | ||
| true, | ||
| ], | ||
| 'doesNotAddDuplicateThrows' => [ |
There was a problem hiding this comment.
This validates that the fix only adds exceptions which are not listed in the current throws doc block
|
Seems fine to me, if you mark this as ready, I'm up to merge it :) |
|
@orklah Can this be merged or is it something to do still |
|
Sorry :( I didn't get a notification on the status change! Thanks! |
Fixes #7918