Add client builder #480
Add client builder #480
Conversation
|
I like the idea! 👍 |
|
@Nyholm you should also update the docs for customizing the api-client as |
|
Thank you. I've updated the PR |
|
A few more places which use the old constructor |
| } | ||
|
|
||
|
|
||
|
|
|
|
||
| /** | ||
| * A builder that builds the API client. This will allow you to fluently add and remove | ||
| * plugins. |
There was a problem hiding this comment.
you can place each sentence on new line, right now it feels a bit strange :D
| */ | ||
| public function __construct(HttpClient $httpClient = null, $apiVersion = null, $enterpriseUrl = null) | ||
| public function __construct(Builder $httpClientBuilder = null, $apiVersion = null, $enterpriseUrl = null) |
There was a problem hiding this comment.
IMO it's better to leave old constructor and create new builder inline or support both.
There was a problem hiding this comment.
Maybe keep the old constructor indeed but deprecated it (throw deprecation warning) and use the new constructor in 3.0. Or you could say use the new constructor from now on as 2.0 stable is not yet released
There was a problem hiding this comment.
My idea is that client should always be constructed without extra manipulations, i.e.
$github = new Client();
$github->doAnything();
// or
$github = new Client($guzzle);
maybe static factory can be a possible solution.
There was a problem hiding this comment.
You are right about that, it should be a simple as possible to instantiate and use the client
There was a problem hiding this comment.
I did not like the new constructor myself to be honest. Should I add another constructor argument with the builder?
|
I made an update. Are we happy with this constructor? |
| $httpBuilder = new Github\HttpClient\Builder(new Http\Adapter\Guzzle6\Client()); | ||
| $httpBuilder->addPlugin(new CustomUserAgentPlugin()); | ||
|
|
||
| $client = new Github\Client(null, $httpBuilder); |
There was a problem hiding this comment.
Check out this example.
Are we happy with the constructor like this?
There was a problem hiding this comment.
Hmm, I don't know. It looks a bit weird to me that you must start with null. I will think about a different approach
There was a problem hiding this comment.
I could keep Github\Client($httpBuilder, $version, $enterprice);
and then have a factory like:
Github\Client::create($guzzle);There was a problem hiding this comment.
That would maybe be a good solution. What do you think @cursedcoder?
There was a problem hiding this comment.
Seems nice.
Just Client::fromAdapter() or fromClient, put some more sense in your method name.
There was a problem hiding this comment.
Great! I'll update the PR after lunch
There was a problem hiding this comment.
Client::withHttpClient($guzzle)?
Client::usingHttpClient($guzzle)?
Client::createWithHttpClient($guzzle)?
There was a problem hiding this comment.
Client::createFromHttpClient($guzzle)? Otherwise I would go for createWith..
|
There, this PR is updated. I'm happy to change the name of My opinion is that you would not create a Github client from a http client. The Github client is using the http client somehow. |
|
YEs maybe you are right about the name of the method! |
I have no strong opinions at all. =) @cursedcoder can you make a decision and I can consider this PR ready for final review. |
Got you. So it makes sense to rename |
|
Thank you. I've updated the PR |
| @@ -5,6 +5,20 @@ The change log describes what is "Added", "Removed", "Changed" or "Fixed" betwee | |||
|
|
|||
| ## 2.0.0-rc5 (UNRELEASED) | |||
|
|
|||
| ### Changed | |||
There was a problem hiding this comment.
The changed part is already added for RC5 right under added (line 32 in your changed file)
There was a problem hiding this comment.
That's correct. Thank you.
|
Thank you for merging |
This will fix #461
The client builder makes sure that all logic related to creating the client is in a separate class.
This is a first draft and feedback is welcome.
TODO