Conversation
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.
This is everything for now. Will do a new review tomorrow but it might be best that you already do some of the changes before I do so. Thanks!
src/Clients/Login.php
Outdated
@@ -0,0 +1,69 @@ | |||
<?php | |||
|
|||
namespace GitHub\Sponsors\Clients; |
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.
Move this back to the GitHub\Sponsors
namespace.
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.
With the coming things in mind I think that having them in a dedicated namespace than all in package root is a better idea to keep classes structured and organized.
use GitHub\Sponsors\Clients\Viewer; | ||
use Illuminate\Http\Client\Factory; | ||
|
||
final class Client |
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.
Merge the logic of this class with GraphqlClient
and name it Client
.
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.
The idea was that the Client
is more a Factory
now - possibly we should rename it!?
And the GraphqlClient
only cares about sending GraphQL queries to GitHub.
Merging them would also mean that I would have to pass an instance of Client
to the Login/Viewer
classes which would mean that they could also call themselves or the other client.
Add strict types Co-authored-by: Dries Vints <dries@vints.io>
✅ |
resolves #13
This PR switches the current single method calls to a fluent builder style for easier readability and this also allows us to use different classes per context - as the GraphQL queries for
viewer
anduser/organization
are different and also allow different things. For example only a viewer can retrieve the tiers for a sponsorship.I renamed and changed some more class names in that context as I think they match better now. But I'm open for discussion here and change things back.