This repository has been archived by the owner on Apr 4, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adopt fluent syntax for client calls #15
Adopt fluent syntax for client calls #15
Changes from 3 commits
f5adc31
d7db4cb
5d09238
52f9f9e
f85c781
3a27651
ef09da1
96ce003
032f2a5
71cee46
be5ff48
f41a5d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 itClient
.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 aFactory
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 theLogin/Viewer
classes which would mean that they could also call themselves or the other 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.
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.
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.