-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: refactor to use Guzzle #2
base: main
Are you sure you want to change the base?
Conversation
29833f8
to
33a6b11
Compare
|
||
namespace RetroAchievements\Api\Exceptions; | ||
|
||
interface RetroAchievementsException |
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.
All exceptions implement this, which allows for catching any RA client exception. 👍🏻
return $this; | ||
} | ||
|
||
public function getTopTenUsers(): mixed |
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.
Not sure if these should be split into action traits based on the subtypes in the API docs. It would increase readability and discovery.
For example, getTopTenUsers()
could be under a ManagesFeeds
trait, and getGameInfo()
and getGameInfoExtended()
could be under ManagesGames
🙂
33a6b11
to
16957ed
Compare
16957ed
to
440a35f
Compare
protected function getApiUrl(string $endpoint, array $parameters = []): mixed | ||
{ | ||
return $this->get( | ||
sprintf('/API/%s?%s', $endpoint, http_build_query([ |
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.
I've changed this to /API/
rather than lowercase, as otherwise it hits a 404 for all the requests. 🤷🏻
It's kind of a shame, but I assume this will be resolved when migrating to proper Laravel routes and controllers for the API. 👍🏻 Speaking of, are there any plans / a roadmap for this (I'd be interested in helping)?
Another possibility would be to create classes representing each resource, and then we would be able to map the values into these. This would be similar to how the JS package uses it's |
Hi @owenvoke! I believe @luchaos may be otherwise engaged at the moment. My perception is all of our API packages are intended to be reference wrappers around RAWeb's endpoints, similar to |
Hi @wescopeland, thanks for the details. I modelled this off of the API (and looked into the JS package previously). The changes in this PR should cover the expectations. 👍🏻 I've got no issues with waiting for feedback, I was more adding that comment as a note for myself. 👍🏻 |
Obviously this doesn't have to be used, but I thought I'd create a quick example of what this package could look like using Guzzle. 👍🏻
And here's a test script you can use:
I'd also be happy to implement PHPStan and Pint for static analysis and code style formatting (or some tests). 👍🏻