Integrations are now Apps! #592
Conversation
m1guelpf
changed the title
lib/Github/Api/Apps.php
Outdated
| * | ||
| * @return self | ||
| */ | ||
| public function configure() |
There was a problem hiding this comment.
I think we should keep BC here and just remove the $this->acceptHeaderValue = 'application/vnd.github.machine-man-preview+json'; but keep the method as users of the lib are possibly calling this method
lib/Github/Api/Apps.php
Outdated
| * @author Nils Adermann <naderman@naderman.de> | ||
| */ | ||
| class Integrations extends AbstractApi | ||
| class Apps extends AbstractApi |
There was a problem hiding this comment.
Also to keep BC the class can not be renamed (in my opinion). But the class can be deprecated and a new Apps class can be created (where this class can extend from)
There was a problem hiding this comment.
As I also modified the Client to point to the new class, there's no reason to keep using this...
There was a problem hiding this comment.
I know, I wanted to do a same sort of change but @Nyholm came up with a good point.
lib/Github/Api/Apps.php
Outdated
| class Apps extends AbstractApi | ||
| { | ||
| /** | ||
| * @deprecated |
There was a problem hiding this comment.
Oh I see. Move this method back to the Integrations class. It is weird to add a class with deprecated functions.
lib/Github/Api/Integrations.php
Outdated
| /** | ||
| * @link https://developer.github.com/v3/integrations/ | ||
| * @deprecated |
There was a problem hiding this comment.
Add a comment saying what the new thing is.
There was a problem hiding this comment.
Maybe we can do it "symfony" style with a @trigger_error when using the class. Like this https://github.com/symfony/symfony/blob/3.3/src/Symfony/Component/ClassLoader/ApcClassLoader.php#L14
But also keep the annotation so ide's will show it. What do you think @Nyholm?
| @@ -29,8 +29,9 @@ | |||
| * @method Api\Gists gist() | |||
| * @method Api\Gists gists() | |||
| * @method Api\Miscellaneous\Gitignore gitignore() | |||
| * @method Api\Integrations integration() | |||
There was a problem hiding this comment.
You could leave these comments.
There was a problem hiding this comment.
yes indeed keep the old methods (to the deprecated class) so a user will know they have to change their code to the new methods
lib/Github/Client.php
Outdated
| @@ -200,7 +201,8 @@ public function api($name) | |||
|
|
|||
| case 'integration': | |||
| case 'integrations': | |||
| $api = new Api\Integrations($this); | |||
There was a problem hiding this comment.
You should keep this. Calling integrations should give you the deprecated Integrations class, right?
There was a problem hiding this comment.
That's right, this way the user knows he has to update some code
lib/Github/Client.php
Outdated
| @@ -201,6 +201,8 @@ public function api($name) | |||
|
|
|||
| case 'integration': | |||
| case 'integrations': | |||
| $api = new Api\Integrations($this); | |||
| break | |||
|
Yeah it looks like packagist is also having problems https://twitter.com/packagist/status/866952618349920257 |
|
@m1guelpf I've restarted the build! |
|
The build is green again! Thanks @m1guelpf |
See #591