Skip to content

Add configure() support for issues #532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 27, 2017
Merged

Add configure() support for issues #532

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
962beba
Add configure() support for issues
bobeagan Mar 3, 2017
8380ce4
add more configure methods to address missing custom media types
bobeagan Mar 6, 2017
a45ce63
switch order of configure values for PullRequest and PullRequest/Comm…
bobeagan Mar 23, 2017
debf2b3
Merge branch 'master' into patch-2
bobeagan Mar 27, 2017
07cb9e4
update configure() to be chainable per #544
bobeagan Mar 27, 2017
f210737
revert existing bodyTypes that were defaulting to "full" instead of "…
bobeagan Mar 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/Github/Api/Gist/Comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,31 @@
namespace Github\Api\Gist;

use Github\Api\AbstractApi;
use Github\Api\AcceptHeaderTrait;

/**
* @link https://developer.github.com/v3/gists/comments/
* @author Kayla Daniels <[email protected]>
*/
class Comments extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/gists/comments/#custom-media-types
* @param string|null $bodyType
*/
public function configure($bodyType = null)
{
if (!in_array($bodyType, array('text', 'html', 'full'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s+json', $this->client->getApiVersion(), $bodyType);
}

/**
* Get all comments for a gist.
*
Expand Down
17 changes: 17 additions & 0 deletions lib/Github/Api/Gists.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,23 @@
*/
class Gists extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/gists/#custom-media-types
* @param string|null $bodyType
*/
public function configure($bodyType = null)
{
if (!in_array($bodyType, array('base64'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s', $this->client->getApiVersion(), $bodyType);
}

public function all($type = null)
{
if (!in_array($type, array('public', 'starred'))) {
Expand Down
17 changes: 17 additions & 0 deletions lib/Github/Api/Issue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@
*/
class Issue extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/issues/#custom-media-types
* @param string|null $bodyType
*/
public function configure($bodyType = null)
{
if (!in_array($bodyType, array('text', 'html', 'full'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s+json', $this->client->getApiVersion(), $bodyType);
}

/**
* List issues by username, repo and state.
*
Expand Down
4 changes: 2 additions & 2 deletions lib/Github/Api/Issue/Comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ class Comments extends AbstractApi
*/
public function configure($bodyType = null)
{
if (!in_array($bodyType, array('raw', 'text', 'html'))) {
$bodyType = 'full';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot change the default behavior. That is breaking BC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the initial PR message and also further down in the last comment I left, I was recognizing that this was a breaking change but that as it was currently implemented here (and in Repository/Comments) the option that you have defaulting currently does not align with the default behavior that is described in the GitHub API documentation.

I would suggest aligning with the documentation, noting the breaking change in the release and bumping a major version.

If you aren't interested in doing that, then I would rather go through and change all of the places where I set the default value to raw and switch them to full but then I would suggest that information gets included somewhere that the default behavior of this library does not match the default behavior of the documentation, to avoid any potential for confusion of users of the library.

Let me know your preference and I'll update accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw your comments.

I think the best solution is to accept that we made a mistake before by keep the default body type to full for the comments API. In general we should align with the docs.

Making a new major release just for this fix is not a good solution. What we should do is to create a new issue suggesting this change and add that issue in a 3.0 milestone.

So in short: Just change this back to full and I'll be happy to merge.

if (!in_array($bodyType, array('text', 'html', 'full'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s+json', $this->client->getApiVersion(), $bodyType);
Expand Down
25 changes: 25 additions & 0 deletions lib/Github/Api/PullRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,31 @@
*/
class PullRequest extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/pulls/#custom-media-types
* @param string|null $bodyType
*/
public function configure($apiVersion = null, $bodyType = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me more about the api version here. That is just to enable preview features, right? You cannot change to an older version of the API.

Also why have you chosen this order of parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this was to allow for setting the preview version of https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button

Looking now, the order doesn't make much sense as I did it, I'm guessing that I probably started on the PullRequest/Comments (since that is what I was looking to set a preview version for and then came back through later when I figured I would update all of the places that allow changing the bodyType of message responses. I'll update this to flip the order around (and also to add the missing @param that I forgot to include when copy/pasting the comment block)

{
if (!in_array($apiVersion, array('polaris-preview'))) {
$apiVersion = $this->client->getApiVersion();
}

if (!in_array($bodyType, array('text', 'html', 'full', 'diff', 'patch'))) {
$bodyType = 'raw';
}

if (!in_array($bodyType, array('diff', 'patch'))) {
$bodyType .= '+json';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s', $this->client->getApiVersion(), $bodyType);
}

/**
* Get a listing of a project's pull requests by the username, repository and (optionally) state.
*
Expand Down
22 changes: 22 additions & 0 deletions lib/Github/Api/PullRequest/Comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Github\Api\PullRequest;

use Github\Api\AbstractApi;
use Github\Api\AcceptHeaderTrait;
use Github\Exception\MissingArgumentException;

/**
Expand All @@ -11,6 +12,27 @@
*/
class Comments extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/pulls/comments/#custom-media-types
* @param string|null $bodyType
*/
public function configure($apiVersion = null, $bodyType = null)
{
if (!in_array($apiVersion, array('squirrel-girl-preview'))) {
$apiVersion = $this->client->getApiVersion();
}

if (!in_array($bodyType, array('text', 'html', 'full'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s+json', $apiVersion, $bodyType);
}

public function all($username, $repository, $pullRequest = null)
{
if (null !== $pullRequest) {
Expand Down
25 changes: 9 additions & 16 deletions lib/Github/Api/Repository/Comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,19 @@ class Comments extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/repos/comments/#custom-media-types
* @param string|null $bodyType
*/
public function configure($bodyType = null)
{
switch ($bodyType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

case 'raw':
$header = sprintf('Accept: application/vnd.github.%s.raw+json', $this->client->getApiVersion());
break;

case 'text':
$header = sprintf('Accept: application/vnd.github.%s.text+json', $this->client->getApiVersion());
break;

case 'html':
$header = sprintf('Accept: application/vnd.github.%s.html+json', $this->client->getApiVersion());
break;

default:
$header = sprintf('Accept: application/vnd.github.%s.full+json', $this->client->getApiVersion());
if (!in_array($bodyType, array('text', 'html', 'full'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = $header;
$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s+json', $this->client->getApiVersion(), $bodyType);
}

public function all($username, $repository, $sha = null)
Expand Down
18 changes: 18 additions & 0 deletions lib/Github/Api/Repository/Contents.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Github\Api\Repository;

use Github\Api\AbstractApi;
use Github\Api\AcceptHeaderTrait;
use Github\Exception\InvalidArgumentException;
use Github\Exception\ErrorException;
use Github\Exception\MissingArgumentException;
Expand All @@ -14,6 +15,23 @@
*/
class Contents extends AbstractApi
{
use AcceptHeaderTrait;

/**
* Configure the body type.
*
* @link https://developer.github.com/v3/repo/contents/#custom-media-types
* @param string|null $bodyType
*/
public function configure($bodyType = null)
{
if (!in_array($bodyType, array('html', 'object'))) {
$bodyType = 'raw';
}

$this->acceptHeaderValue = sprintf('application/vnd.github.%s.%s', $this->client->getApiVersion(), $bodyType);
}

/**
* Get content of README file in a repository.
*
Expand Down