-
Notifications
You must be signed in to change notification settings - Fork 729
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.
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
pullRequestReview.review.htmlUrl #946
pullRequestReview.review.htmlUrl #946
Conversation
@@ -46,6 +46,7 @@ | |||
private String commit_id; | |||
private GHPullRequestReviewState state; | |||
private String submitted_at; | |||
private String html_url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ObjectMapper configuration explicitly declares snake_case
strategy, but there is no single standard of field naming - I see both standard java (htmlUrl
) and snake case (html_url
) over the project (only fields, not getters for sure). This is confusing, IMHO it should be better to standardize it (personal preference - to java).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seregamorph "standardize to java naming" - agreed. It just hasn't been a priority. Especially since we don't have
sufficient code coverage to have high confidence in tests ensuring it is done right. PR welcome.
@@ -102,7 +103,7 @@ public GHPullRequestReviewState getState() { | |||
|
|||
@Override | |||
public URL getHtmlUrl() { | |||
return null; | |||
return GitHubClient.parseURL(html_url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back compatibility saved. If field is not present, the return value is still null
Please note, that https://docs.github.com/en/rest/reference/pulls#reviews does not explicitly declare this parameter. I assume it only present in the webhook. |
Description
Webhook event
GHPullRequestReviewEvent: pull_request_review
has review object in it which declareshtml_url
field:Old implementation ignores it (always null).
Proof: see the test, event existing test payload
pull_request_review.json
has it (ignored in test).Before submitting a PR:
We love getting PRs, but we hate asking people for the same basic changes every time.
master
. Create your PR from that branch.mvn clean compile
locally. This may reformat your code, commit those changes.mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.When creating a PR: