Change pull request status to NEEDS_WORK instead of resetting approval.#190
Change pull request status to NEEDS_WORK instead of resetting approval.#190tommywo wants to merge 3 commits intoAmadeusITGroup:masterfrom
Conversation
|
Hi @tommywo, thanks for this! Can you remove remove the three irrelevant commits from your PR? |
|
Hi @t-8ch. |
| } | ||
|
|
||
|
|
||
| public String getUserSlug() { |
| return credentials.getLogin(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
This PR seems to use two blank lines between functions, while the existing code only uses one.
| } | ||
| } | ||
|
|
||
|
|
|
|
||
| put(request, | ||
| json, | ||
| MessageFormat.format(PULL_REQUEST_APPROVAL_POST_ERROR_MESSAGE, pr.repository(), pr.pullRequestId())); |
There was a problem hiding this comment.
A custom error message would be better.
| pr.pullRequestId(), | ||
| credentials.getUserSlug()); | ||
| JsonObject json = new JsonObject(); | ||
| json.put("status","NEEDS_WORK"); |
|
@tommywo I still have a few minor nit-picks. |
|
@tommywo Thanks! Please squash the commits into one. As approving/un-approving and approving/"needs work" are mutually exclusive I propose to merge the setting into I admit the naming is a little bit unintuitive but we
What do you think? |
|
How about defining |
|
To be honest I would prefer not introduce a deprecation warning and force users to change their working configuration. Should we release a incompatible version 2.0 in the future we could change the setting name. |
|
Reopening to restart tests |
2 participants
No description provided.