Support for merge_commit_sha #231
Conversation
| * FYI: https://developer.github.com/changes/2013-04-25-deprecating-merge-commit-sha | ||
| */ | ||
| @Deprecated | ||
| public String getMergeCommitSha() throws IOException { |
There was a problem hiding this comment.
What happens if the field disappears? Does the method return null in such case?
There was a problem hiding this comment.
When this happens, GitHub API v4 will be released and we should review the current status (compatibility).
There was a problem hiding this comment.
@oleg-nenashev Any recommendation here to improve it?
There was a problem hiding this comment.
@oleg-nenashev I've checked that if your model has an attribute and is not mapped in the JSON, its get() method returns null. I think that is ok.
There was a problem hiding this comment.
Right. This line is preventing to fail on missing properties.
There was a problem hiding this comment.
@oleg-nenashev @CheckForNull?
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| @@ -142,9 +145,9 @@ public PullRequest getPullRequest() { | |||
| } | |||
|
|
|||
| // | |||
| // details that are only available via get with ID | |||
| // | |||
| // | |||
There was a problem hiding this comment.
Unrelated changes but...
There was a problem hiding this comment.
Makes sense to convert the comment to Javadoc then :)
There was a problem hiding this comment.
Really, it is not a method comment, only a note in the middle of the file.
|
why you want to use deprecated field? |
|
@lanwen because it can be valid for some unknown time or on some ancient enterprise installations? |
| @@ -186,6 +189,15 @@ public int getChangedFiles() throws IOException { | |||
| } | |||
|
|
|||
| /** | |||
| * FYI: https://developer.github.com/changes/2013-04-25-deprecating-merge-commit-sha | |||
|
🐝 |
|
@reviewbybees done |
|
👍 |
|
Does not make sense to introduce a deprecated API, especially as this will probably still working in v4. |
|
probably makes sense call variable in camel case according to java convention and use Json annotation. |
|
@jglick if you don't need to use it, simply ignore it. GH API V4 is not planned, there is not a date. While this happens this attribute is the only way to resolve some problems. |
|
@jglick make sense to include not documented behaviors? For example, retrieve |
|
@KostyaSha I agree but I tried to follow the current conventions in this project. |
|
@jglick I apologize. I understood wrong your comment. I should read twice before to reply. Specially, when I read english text. |
|
@jglick I'm going to remove the tag |
|
@jglick |
|
LGTM |
|
@reviewbybees done |
|
Belated 🐝. I do not think we really want to be using this API. But I see no harm in exposing it via the library. It is up to the caller whether to rely on it or not. I presume that if and when the field is deleted from the JSON by GH, |
|
This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging. |
|
@jglick If a field is removed from GitHub API, its getter asociated will return |
GHPullRequestobject now includesmerge_commit_sha@reviewbybees specially, @oleg-nenashev