Add configure() support for issues #532
Conversation
Borrowed from issue comments, but changed the default bodyType to "raw" since the documentation indicates that is the default when no specific media type is passed: https://developer.github.com/v3/media/#comment-body-properties
|
There are others that should be updated too, this was just the first one that was causing me issue. If I get a chance on Monday, I can try to add the others to this PR (or if you merge before then, I'll make a separate). |
- also fixes the Issue/Comments default configure value - also adds support for polaris-preview in PullRequest - also adds support for squirrel-girl-preview in PullRequest/Comments - replaces switch case with in_array in Repository/Comments to standardize
|
I updated more places that used custom media types in the Accept header. I noticed what is likely a bug in Repository/Comments where I also fixed a few places where the default value was not correct to what the API docs specified, which might be considered a breaking change if you version according to semver. |
| public function configure($bodyType = null) | ||
| { | ||
| switch ($bodyType) { |
lib/Github/Api/PullRequest.php
Outdated
| * @link https://developer.github.com/v3/pulls/#custom-media-types | ||
| * @param string|null $bodyType | ||
| */ | ||
| public function configure($apiVersion = null, $bodyType = null) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
lib/Github/Api/Issue/Comments.php
Outdated
| @@ -23,8 +23,8 @@ class Comments extends AbstractApi | |||
| */ | |||
| public function configure($bodyType = null) | |||
| { | |||
| if (!in_array($bodyType, array('raw', 'text', 'html'))) { | |||
| $bodyType = 'full'; | |||
There was a problem hiding this comment.
You cannot change the default behavior. That is breaking BC
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…ents Users would more commonly be interested in changing the bodyType rather than apiVersion Also, added missing @param values
# Conflicts: # lib/Github/Api/Repository/Comments.php
…raw" Per @Nyholm "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."
|
@Nyholm I reverted the two breaking changes and will submit a different PR for to change the default behavior of those instances to align with the GH docs. I also updated the newly added configure() methods to be chainable per a recently merged PR that introduced that functionality. |
Borrowed from issue comments, but changed the default bodyType to "raw" since the documentation indicates that is the default when no specific media type is passed: https://developer.github.com/v3/media/#comment-body-properties