Add [Github] pull request state badge #1207
Conversation
paulmelnikow
changed the title
| @@ -9,5 +9,6 @@ | |||
| "grey": { "colorB": "#555" }, | |||
| "gray": { "colorB": "#555" }, | |||
| "lightgrey": { "colorB": "#9f9f9f" }, | |||
| "lightgray": { "colorB": "#9f9f9f" } | |||
| "lightgray": { "colorB": "#9f9f9f" }, | |||
| "purple": { "colorB": "#6f42c1" } | |||
There was a problem hiding this comment.
Is this github's merged purple? If we do add a named "purple" it probably should not be github's. You can hard-code it in the badge code instead.
| // pull request state | ||
|
|
||
| t.create('Pull request state, regular case') | ||
| .get('/pr-state/badges/shields/578.json') |
There was a problem hiding this comment.
Let's make this /pulls/state for consistency with e.g. https://img.shields.io/github/pulls/detail/comments/badges/shields/1112.svg.
| .expectedJSONTypes(Joi.object().keys({ | ||
| name: 'pull request', | ||
| value: Joi.string().regex(/^\w+/) | ||
| })); |
There was a problem hiding this comment.
Could you get these tests passing? Otherwise they look good!
| <td> | ||
| <code>https://img.shields.io/github/pr-state/badges/shields/6.svg</code></td> | ||
| </tr> | ||
| <tr> |
There was a problem hiding this comment.
After #1163 try.html is generated, so could you please make these changes in lib/all-badge-examples.js instead, and then run npm run build?
try.html will no longer be tracked after #1194.
| badgeData.logo = getLogo('github', data); | ||
| } | ||
|
|
||
| githubAuth.request(request, apiUrl, {}, function (err, res, buffer) { |
There was a problem hiding this comment.
You can use an arrow function here: (err, res, buffer) =>
| try { | ||
| const pr = JSON.parse(buffer); | ||
| let colorscheme = ''; | ||
| let status = ''; |
There was a problem hiding this comment.
It would be clearer what's happening with these declarations let colorscheme if you moved them just before their switch statements. Also you can just write let colorscheme;. It doesn't help to assign an empty string.
| let colorscheme = ''; | ||
| let status = ''; | ||
|
|
||
| const responseIsIssueLike = pr.hasOwnProperty('state') && |
There was a problem hiding this comment.
How about calling it responseIsValid to indicate this code is validating the response?
| .get('/pr-state/badges/shields/578.json') | ||
| .expectJSONTypes(Joi.object().keys({ | ||
| name: Joi.equal('pull request state'), | ||
| value: Joi.equal('closed', 'merged', 'mergeable', 'has conflicts', 'need rebase') |
There was a problem hiding this comment.
This will always be 'merged'.
| .get('/pr-state/badges/shields/-1.json') | ||
| .expectJSONTypes(Joi.object().keys({ | ||
| name: Joi.equal('pull request state'), | ||
| value: Joi.equal('inaccessible') |
There was a problem hiding this comment.
Since these are both literals you can simplify this to .expectJSON({ ... })
| .get('/pr-state/badges/shields/6.json') | ||
| .expectedJSONTypes(Joi.object().keys({ | ||
| name: 'pull request', | ||
| value: Joi.string().regex(/^\w+/) |
There was a problem hiding this comment.
I'm not sure I understand the purpose of this test. Could you clarify? ^^
|
@paulmelnikow Thanks for the great review. Will address all the notes ASAP. |
|
Hey, just thought I'd follow up. Could you address these comments when you have a chance? Also, please merge master into your branch to resolve the conflicts. |
|
Yes. I was traveling a lot lately. Will do it in the next few days. |
paulmelnikow
added
the
on-hold
|
I'd love to get this in. Feel free to reopen it when you're ready! Anyone else reading this who want this implemented, you can grab the commits from the pull request branch using |
URL pattern:
/github/pr-state/<user>/<repo>/<issueId>.<format>The issue (which ID is provided in the URL) must be a pull-request.
The following states are handled:
inaccessible(lightgrey) if the request failed or the response was not a json-formatted issueclosed(red) if the PR is closed and not mergedmerged(Github purple-ish) if the PR is closed and mergedmergeable(green) if the PR is open and clean (build OK, no conflicts detected, rebased on latest version of the base branch)unstable(yellow) if the PR is open but unstabehas conflicts(red) if the PR is open but not mergeable (conflicts with base branch detected)unknown(lightgrey) if the PR is open but the mergeability is not computed yet (more details here)Related PR: GH-806