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

Add @throws annotation #287

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VincentLanglet
Copy link

What:

  • Bug Fix
  • New Feature (?)

Description:

HI @gehrisandro @nunomaduro, I recently used this library and got an API exception in production. Currently there is no @throws phpdoc above method to help the developer to be aware exception are thrown and which can be caught.
@throws annotation allow tools like PHPStorm or PHPStan to report not caught exception. I tried to improve the documentation then.

  • Since there are multiple exception in this library I introduced a OpenAIThrowable interface which allow to catch all of them.
  • Then, I added @throws OpenAIThrowable to every method throwing an exception
  • To be sure the documentation is up to date, I updated the phpstan config to check @throws tag: https://phpstan.org/blog/bring-your-exceptions-under-control
  • Since $response->getBody()->getContents(); throw a RuntimeException, I introduced a UnreadableResponse exception to encapsulate the default exception (and implements OpenAIThrowable).

Thanks.

Related:

@pkly
Copy link

pkly commented Jan 11, 2024

I just added this library to our project and noticed exception throws were missing on higher level interfaces but were added on request interfaces which were used.

This would be a nice addition, but I would instead like to request specific exceptions that are thrown by default implementations to be specified on the interfaces instead of the interface.
Though obviously the interface to be shared across the exceptions is a nice idea.

@VincentLanglet
Copy link
Author

but I would instead like to request specific exceptions that are thrown by default implementations to be specified on the interfaces instead of the interface.

The interface should be about contract and not about "default implementations".
This allow different implementations.

Rather than saying exactly the exception thrown in the default implementation (which is difficult because it can change often) it's generally better to throw an Interface, saying "All theses exceptions could be thrown or not" and if someone else implements the interface he can decide which exceptions from the interface he throws.

@VincentLanglet
Copy link
Author

@gehrisandro @nunomaduro I rebased the PR if you have time to take a look :)

Thanks a lot.

@pkly
Copy link

pkly commented Jan 11, 2024

In that case all of the exceptions should be behind interfaces, though that is rather... annoying.

@VincentLanglet
Copy link
Author

In that case all of the exceptions should be behind interfaces, though that is rather... annoying.

You can catch Interfaces, that the same.
You can also write

catch (ExceptionA) {
  // dothings
} catch (ExceptionInterface) {
  // dothings
}

And that way more useful, than a PHPdoc ExceptionA|ExceptionB|ExceptionC|....

Most of the biggest PHP works with interfaces.

@pkly
Copy link

pkly commented Jan 11, 2024

My point being biggest frameworks use separate interfaces for specific error types, not a generic one.

@VincentLanglet
Copy link
Author

Friendly ping @gehrisandro @nunomaduro do you have time to review ? thanks ! :)

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.

None yet

2 participants