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

feat: refactor to use Guzzle #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

owenvoke
Copy link

@owenvoke owenvoke commented Apr 24, 2024

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:

<?php

declare(strict_types=1);

use RetroAchievements\Api\RetroAchievements;

require __DIR__ . '/vendor/autoload.php';

$username = getenv('RA_API_USERNAME');
$token = getenv('RA_API_TOKEN');

$client = new RetroAchievements($username, $token);

dump($client->getGameInfo(23796));

I'd also be happy to implement PHPStan and Pint for static analysis and code style formatting (or some tests). 👍🏻

@owenvoke owenvoke force-pushed the feature/guzzle branch 2 times, most recently from 29833f8 to 33a6b11 Compare April 24, 2024 09:20

namespace RetroAchievements\Api\Exceptions;

interface RetroAchievementsException
Copy link
Author

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
Copy link
Author

@owenvoke owenvoke Apr 24, 2024

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 🙂

protected function getApiUrl(string $endpoint, array $parameters = []): mixed
{
return $this->get(
sprintf('/API/%s?%s', $endpoint, http_build_query([
Copy link
Author

@owenvoke owenvoke Apr 24, 2024

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)?

@owenvoke
Copy link
Author

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 model interfaces.

@wescopeland
Copy link
Member

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 api-js. I'm sure you've already looked through api-js's source code, but if not it can be found here: https://github.com/RetroAchievements/api-js

@owenvoke
Copy link
Author

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. 👍🏻

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.

2 participants