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

fix(psalm): Fix all @throws annotations and add missing ones #3270

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

No description provided.

Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin added the 2. developing Items that are currently under development label Sep 21, 2024
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I kind of hate that Exception is sometimes \Exception and sometimes \OCP\DB\Exception. I tend to never use Exception because of that to make it clear when we throw the root one.

Also @throws blocks should contain an explanation when the exceptions is thrown, except for self-explanatory ones like NoUserException and InvalidArgumentException.

And for all methods implementing an OCP interface they should not throw unless it’s permitted by the interface. But that does mean for some of them we should modify the interface instead.
For controllers I do not know the policy, are they allowed to throw, and which kind of exceptions/throwable is handled by Nextcloud core?

Comment on lines +233 to +234
* @throws ContainerExceptionInterface
* @throws Throwable
Copy link
Contributor

Choose a reason for hiding this comment

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

When does that throw Throwable? For me that should never be documented, Throwable which are not Exception are bugs to be fixed, not documented. Anything can throw a Throwable if you use it wrong.
Would also be good if those @throws tag would include a description of when to expect a throw.

Comment on lines +26 to +29
/**
* @throws Exception
* @throws RequestBuilderException
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be caught inside, this method is not documented as throwing in ICapability and should not throw.

use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class Create extends Base {
/**
* @throws LogicException
Copy link
Contributor

Choose a reason for hiding this comment

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

when? Add a description please

Comment on lines +21 to 26
/**
* @throws LogicException
*/
public function __construct() {
parent::__construct();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @throws LogicException
*/
public function __construct() {
parent::__construct();
}

dead code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Items that are currently under development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants