-
Notifications
You must be signed in to change notification settings - Fork 12
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
refactor: configurable client #111
Conversation
…m/php-client into refactor/configurable-client
src/API/AbstractAPI.php
Outdated
private function buildUrl(string $method, string $path): string | ||
{ | ||
if ($method === 'post') { | ||
$baseUri = Arr::get($this->client->getHosts(), 'transactions', $this->client->getHosts()['api']); | ||
} else { | ||
$baseUri = $this->client->getHosts()['api']; | ||
} |
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.
was more thinking along the lines of this @alfonsobries
private function buildUrl(string $method, string $path): string | |
{ | |
if ($method === 'post') { | |
$baseUri = Arr::get($this->client->getHosts(), 'transactions', $this->client->getHosts()['api']); | |
} else { | |
$baseUri = $this->client->getHosts()['api']; | |
} | |
private function buildUrl(string $method, string $path, string $api = 'api'): string | |
{ | |
$baseUri = $this->client->getHosts()[$api]; |
that way it will default to api
, but some requests (like the transaction one) can pass their respective api type to use to construct the URL. That way it remains extensible (not limited to an internal check that needs updating) and will prepare it for the evm options as well.
Probably best to not pass it as a string but create an enum for it so the options are limited, and have a failsafe where it defaults to the "api"
option if the entry doesn't exist in the config?
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.
@ItsANameToo my bad at the moment though your comment was on the constructor
What do you think about this alternative, think is a good idea to pass the api on the same method that makes the api call so its clear that it uses a different api
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.
@alfonsobries looks good 👍
Summary
https://app.clickup.com/t/86du8dubn
you can add
evm
endpoint but currently not used anywhere, also addedsrc/ArkClientBuilder.php
to build a client with a more intuitive api but maybe is not really necessary, up to discussionRefer to testing script for details on how to use
Checklist