Skip to content
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.

Sign up for GitHub

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

Add [Github] pull request state badge #1207

Closed
wants to merge 1 commit into from
Closed

Add [Github] pull request state badge #1207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/colorscheme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@
"grey": { "colorB": "#555" },
"gray": { "colorB": "#555" },
"lightgrey": { "colorB": "#9f9f9f" },
"lightgray": { "colorB": "#9f9f9f" }
"lightgray": { "colorB": "#9f9f9f" },
"purple": { "colorB": "#6f42c1" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
98 changes: 97 additions & 1 deletion lib/github-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,104 @@ function mapGithubReleaseDate(camp, githubApiUrl, githubAuth) {
}));
}

function mapGithubPRState(camp, githubApiUrl, githubAuth){
camp.route(/^\/github\/pr-state\/([^/]+)\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/,
cache((data, match, sendBadge, request) => {
const [, owner, repo, number, format] = match;
const apiUrl = `${githubApiUrl}/repos/${owner}/${repo}/pulls/${number}`;
const badgeData = getBadgeData('pull request', data);

if (badgeData.template === 'social') {
badgeData.logo = getLogo('github', data);
}

githubAuth.request(request, apiUrl, {}, function (err, res, buffer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use an arrow function here: (err, res, buffer) =>

if (err != null) {
badgeData.text[1] = 'inaccessible';
sendBadge(format, badgeData);
return;
}

//github return 404 if repo not found or no pr
if(res.statusCode === 404) {
badgeData.text[1] = 'no pr or repo not found';
sendBadge(format, badgeData);
return;
}

try {
const pr = JSON.parse(buffer);
let colorscheme = '';
let status = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const responseIsIssueLike = pr.hasOwnProperty('state') &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling it responseIsValid to indicate this code is validating the response?

pr.hasOwnProperty('merged') &&
pr.hasOwnProperty('mergeable') &&
pr.hasOwnProperty('mergeable_state');

if(!responseIsIssueLike) {
badgeData.text[1] = 'inaccessible';
sendBadge(format, badgeData);
return;
}

if (pr.state === 'closed') {
badgeData.text[1] = pr.merged ? 'merged' : 'closed';
badgeData.colorscheme = pr.merged ? 'purple' : 'closed';
sendBadge(format, badgeData);
return;
}

if(pr.state !== 'open') {
badgeData.text[1] = 'inaccessible';
sendBadge(format, badgeData);
return;
}

switch (pr.mergeable_state) {
case 'clean':
status = 'mergeable';
break;
case 'dirty':
status = 'has conflicts';
break;
case 'behind':
status = 'needs rebase';
break;
default:
status = pr.mergeable_state;
}

switch (pr.mergeable) {
case null:
colorscheme = 'lightgrey';
break;
case 'unstable':
case 'behind':
colorscheme = 'yellow';
break;
case true:
colorscheme = 'green';
break;
default:
colorscheme = 'red';
}

badgeData.text[1] = status;
badgeData.colorscheme = colorscheme;
sendBadge(format, badgeData);
} catch (e) {
console.log(e);
badgeData.text[1] = 'invalid';
sendBadge(format, badgeData);
}
});
}));
}


module.exports = {
mapGithubCommitsSince,
mapGithubReleaseDate
mapGithubReleaseDate,
mapGithubPRState
};
8 changes: 6 additions & 2 deletions server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ const {

const {
mapGithubCommitsSince,
mapGithubReleaseDate
mapGithubReleaseDate,
mapGithubPRState
} = require("./lib/github-provider");

var semver = require('semver');
Expand Down Expand Up @@ -3433,7 +3434,10 @@ cache(function(data, match, sendBadge, request) {
mapGithubReleaseDate(camp, githubApiUrl, githubAuth);

// GitHub commits since integration.
mapGithubCommitsSince(camp, githubApiUrl ,githubAuth);
mapGithubCommitsSince(camp, githubApiUrl, githubAuth);

// GitHub pull request state
mapGithubPRState(camp, githubApiUrl, githubAuth);

// GitHub release-download-count and pre-release-download-count integration.
camp.route(/^\/github\/(downloads|downloads-pre)\/([^/]+)\/([^/]+)(\/.+)?\/([^/]+)\.(svg|png|gif|jpg|json)$/,
Expand Down
170 changes: 170 additions & 0 deletions service-tests/github.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,176 @@ t.create('GitHub closed issues raw')
value: Joi.string().regex(/^\w+\+?$/)
}));

// pull request state

t.create('Pull request state, regular case')
.get('/pr-state/badges/shields/578.json')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this /pulls/state for consistency with e.g. https://img.shields.io/github/pulls/detail/comments/badges/shields/1112.svg.

.expectJSONTypes(Joi.object().keys({
name: Joi.equal('pull request state'),
value: Joi.equal('closed', 'merged', 'mergeable', 'has conflicts', 'need rebase')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always be 'merged'.

}));

t.create('Pull request state, closed pull request')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: null,
mergeable_state: 'unknown',
merged: false,
state: 'closed'
})
)
.expectJSON({
name: 'pull request state',
value: 'closed'
});

t.create('Pull request state, merged pull request')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: null,
mergeable_state: 'unknown',
merged: true,
state: 'closed'
})
)
.expectJSON({
name: 'pull request state',
value: 'merged'
});

t.create('Pull request state, pull request with unknown state')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: null,
mergeable_state: 'unknown',
merged: false,
state: 'open'
})
)
.expectJSON({
name: 'pull request state',
value: 'unknown'
});

t.create('Pull request state, mergeable pull request')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: true,
mergeable_state: 'clean',
merged: false,
state: 'open'
})
)
.expectJSON({
name: 'pull request state',
value: 'mergeable'
});

t.create('Pull request state, pull request with conflicts')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: false,
mergeable_state: 'dirty',
merged: false,
state: 'open'
})
)
.expectJSON({
name: 'pull request state',
value: 'has conflicts'
});

t.create('Pull request state, pull request that needs rebase')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: false,
mergeable_state: 'behind',
merged: false,
state: 'open'
})
)
.expectJSON({
name: 'pull request state',
value: 'need rebase'
});

t.create('Pull request state, mergeable pull request but build unstable')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, {
mergeable: true,
mergeable_state: 'unstable',
merged: false,
state: 'open'
})
)
.expectJSON({
name: 'pull request state',
value: 'unstable'
});

t.create('Pull request state, valid response but not a Github issue')
.get('/pr-state/badges/shields/-1.json')
.expectJSONTypes(Joi.object().keys({
name: Joi.equal('pull request state'),
value: Joi.equal('inaccessible')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are both literals you can simplify this to .expectJSON({ ... })

}));

t.create('Pull request state, invalid response')
.get('/pr-state/badges/shields/578.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/badges/shields/pulls/578')
.query(true)
.reply(200, 'This should be JSON')
)
.expectJSON({
name: 'pull request state',
value: 'inaccessible'
});

t.create('Pull request state, request error')
.get('/pr-state/badges/shields/578.json')
.networkOff()
.expectJSON({
name: 'pull request state',
value: 'inaccessible'
});

t.create('GitHub pull request state')
.get('/pr-state/badges/shields/6.json')
.expectedJSONTypes(Joi.object().keys({
name: 'pull request',
value: Joi.string().regex(/^\w+/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the purpose of this test. Could you clarify? ^^

}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you get these tests passing? Otherwise they look good!


t.create('GitHub open issues')
.get('/issues/badges/shields.json')
.expectJSONTypes(Joi.object().keys({
Expand Down
7 changes: 7 additions & 0 deletions try.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1577,6 +1577,13 @@ <h3 id="miscellaneous">Miscellaneous</h3>
</td>
<td><code>https://img.shields.io/github/issues-pr-closed/cdnjs/cdnjs.svg</code></td>
</tr>
<tr>
<th data-keywords='GitHub pullrequest pr state merge mergeability' data-doc='githubDoc'> GitHub pull request state: </th>
<td>
<img src='/github/pr-state/badges/shields/6.svg' alt=''/></td>
<td>
<code>https://img.shields.io/github/pr-state/badges/shields/6.svg</code></td>
</tr>
<tr>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<th data-doc="githubDoc" data-keywords="GitHub issue label">GitHub issues by-label:</th>
<td>
Expand Down