[WIP] Enhancement: Added GitHub Repository Service #287
Conversation
|
|
||
| use EdpGithub\Client; | ||
|
|
||
| class GithubService |
There was a problem hiding this comment.
any suggestitions? i'm not very smart in naming classes :)
There was a problem hiding this comment.
GithubRepositoryService ?
There was a problem hiding this comment.
@ins0 no, I'm asking for a plain-english description of what this object represents, without programmer terminology
There was a problem hiding this comment.
RepositoryRetriever - GithubRepositoryRetriever without the prefix Github i think this can be misunderstood with the Db Repository Mapper
|
Overall a good refactoring, but some things still missing:
|
|
thanks for your review @Ocramius |
ins0
changed the title
|
@Ocramius what is your opinion - should this service moved to a seperate module folder if this code and classes grows? |
|
@ins0 it's fine as-is if the service is Application-specific. |
| { | ||
| $repositoryCollection = $this->githubClient->api('current_user')->repos($params); | ||
| if( $repositoryCollection instanceof RepositoryCollection ) | ||
| { |
There was a problem hiding this comment.
If you run
$ vendor/bin/phpcbf --standard=./phpcs.xml .
your CS issues will be resolved. See https://travis-ci.org/zendframework/modules.zendframework.com/jobs/44193562#L264.
There was a problem hiding this comment.
i know, didn't set up a hook for cs fix on commit and travel between locations (no laptop) i was forced to push :)
ins0
force-pushed
the
fix/github-service
branch
from
f00b4ac
to
ea21798
Compare
| @@ -87,6 +87,7 @@ | |||
| 'service_manager' => array( | |||
| 'factories' => array( | |||
| 'translator' => 'Zend\I18n\Translator\TranslatorServiceFactory', | |||
| 'RepositoryRetriever' => 'Application\Service\RepositoryRetrieverFactory', | |||
There was a problem hiding this comment.
Since we moved to 5.5, please use ::class syntax
There was a problem hiding this comment.
Also don't call it just RepositoryRetriever, but use the FQCN via RepositoryRetriever::class
There was a problem hiding this comment.
Not sure if we've got PHP5.5 already, do we? If so, 👍.
ins0
force-pushed
the
fix/github-service
branch
from
864c15a
to
eb5dc54
Compare
| $service = $this->prepareService(new Api\CurrentUser, json_encode($payload)); | ||
|
|
||
| $repositories = $service->getAuthenticatedUserRepositories(); | ||
| $this->assertInstanceOf('EdpGithub\Collection\RepositoryCollection', $repositories); |
There was a problem hiding this comment.
Can leverage the class keyword here, too. Helps a lot with finding usages in PhpStorm, too.
| @@ -3,7 +3,6 @@ language: php | |||
| sudo: false | |||
|
|
|||
| php: | |||
| - 5.4 | |||
There was a problem hiding this comment.
The only clue I currently have about what PHP version is currently running on the production machine is #284 (comment), so I think it's not really safe yet to assume we can rely on the availability of PHP5.5.
Can you make the changes in this PR PHP5.4 compliant?
-
classkeyword - generators?
Added GithubService and moved controller methods to service