Shields: pr/issue status badge don't reflect "merged" status

[tooomm]

For example:
This is a merged PR, however, it shows as closed.

Also, why do they lack the leading # which is typical for naming issue or pr numbers at github?

[RedSparr0w]

Unfortunately it doesn't look like GitHub provides if the issue was close by merge or other

API response for PR #1114:

{
  "url": "https://api.github.com/repos/badges/shields/issues/1114",
  "repository_url": "https://api.github.com/repos/badges/shields",
  "labels_url": "https://api.github.com/repos/badges/shields/issues/1114/labels{/name}",
  "comments_url": "https://api.github.com/repos/badges/shields/issues/1114/comments",
  "events_url": "https://api.github.com/repos/badges/shields/issues/1114/events",
  "html_url": "https://github.com/badges/shields/pull/1114",
  "id": 261744651,
  "node_id": "MDExOlB1bGxSZXF1ZXN0MTQzOTYyNjQy",
  "number": 1114,
  "title": "[GitHub] Issue and pull request detail and check state",
  "user": {
    "login": "paulmelnikow",
    "id": 1487036,
    "node_id": "MDQ6VXNlcjE0ODcwMzY=",
    "avatar_url": "https://avatars2.githubusercontent.com/u/1487036?v=4",
    "gravatar_id": "",
    "url": "https://api.github.com/users/paulmelnikow",
    "html_url": "https://github.com/paulmelnikow",
    "followers_url": "https://api.github.com/users/paulmelnikow/followers",
    "following_url": "https://api.github.com/users/paulmelnikow/following{/other_user}",
    "gists_url": "https://api.github.com/users/paulmelnikow/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/paulmelnikow/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/paulmelnikow/subscriptions",
    "organizations_url": "https://api.github.com/users/paulmelnikow/orgs",
    "repos_url": "https://api.github.com/users/paulmelnikow/repos",
    "events_url": "https://api.github.com/users/paulmelnikow/events{/privacy}",
    "received_events_url": "https://api.github.com/users/paulmelnikow/received_events",
    "type": "User",
    "site_admin": false
  },
  "labels": [
    {
      "id": 317278803,
      "node_id": "MDU6TGFiZWwzMTcyNzg4MDM=",
      "url": "https://api.github.com/repos/badges/shields/labels/developer-experience",
      "name": "developer-experience",
      "color": "5319e7",
      "default": false
    },
    {
      "id": 316395868,
      "node_id": "MDU6TGFiZWwzMTYzOTU4Njg=",
      "url": "https://api.github.com/repos/badges/shields/labels/service-badge",
      "name": "service-badge",
      "color": "fbca04",
      "default": false
    }
  ],
  "state": "closed",
  "locked": false,
  "assignee": null,
  "assignees": [

  ],
  "milestone": null,
  "comments": 1,
  "created_at": "2017-09-29T19:28:48Z",
  "updated_at": "2017-10-02T17:26:46Z",
  "closed_at": "2017-10-02T17:26:43Z",
  "author_association": "MEMBER",
  "pull_request": {
    "url": "https://api.github.com/repos/badges/shields/pulls/1114",
    "html_url": "https://github.com/badges/shields/pull/1114",
    "diff_url": "https://github.com/badges/shields/pull/1114.diff",
    "patch_url": "https://github.com/badges/shields/pull/1114.patch"
  },
  "body": "This adds badges for Github issues and pull requests. You can display the state, title, username, number of comments, age, time since last update, and state of checks.\r\n\r\nBlocked on #1112 (see FIXMEs).\r\n\r\nProvides an endpoint the Shields CI can use to fetch PR titles for #979 and resolves #1011.",
  "closed_by": {
    "login": "paulmelnikow",
    "id": 1487036,
    "node_id": "MDQ6VXNlcjE0ODcwMzY=",
    "avatar_url": "https://avatars2.githubusercontent.com/u/1487036?v=4",
    "gravatar_id": "",
    "url": "https://api.github.com/users/paulmelnikow",
    "html_url": "https://github.com/paulmelnikow",
    "followers_url": "https://api.github.com/users/paulmelnikow/followers",
    "following_url": "https://api.github.com/users/paulmelnikow/following{/other_user}",
    "gists_url": "https://api.github.com/users/paulmelnikow/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/paulmelnikow/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/paulmelnikow/subscriptions",
    "organizations_url": "https://api.github.com/users/paulmelnikow/orgs",
    "repos_url": "https://api.github.com/users/paulmelnikow/repos",
    "events_url": "https://api.github.com/users/paulmelnikow/events{/privacy}",
    "received_events_url": "https://api.github.com/users/paulmelnikow/received_events",
    "type": "User",
    "site_admin": false
  }
}

[platan]

Issue API does not provide this information, but pull request API does.

curl https://api.github.com/repos/badges/shields/pulls/1114 -s | jq . | grep merge
  "merged_at": "2017-10-02T17:26:42Z",
  "merge_commit_sha": "bb66a99a66af3838c8d5bf3ea2f6af09d2450004",
      "merges_url": "https://api.github.com/repos/paulmelnikow/shields/merges",
      "merges_url": "https://api.github.com/repos/badges/shields/merges",
  "merged": true,
  "mergeable": null,
  "mergeable_state": "unknown",
  "merged_by": {

Maybe we can switch to this API in this case (I didn't check the code)?

[RedSparr0w]

Nice find, could probably just do something like:
server.js#L3741-L3747

      switch (which) {
        case 's': {
          let state = parsedData.state;
          uri = `${githubApiUrl}/repos/${owner}/${repo}/pulls/${number}`; // uri would need to be `let` not `const`
          if (isPR && state === 'closed'){
            githubAuth.request(request, uri, {}, (err, res, buffer) => {
              //...errorHandling
              const PR_data = JSON.parse(buffer);
              if (PR_data.merged)
                state = 'merged';
            });
          }
          badgeData.text[1] = state;
          badgeData.colorscheme = null;
          badgeData.colorB = makeColorB(githubStateColor(state), queryParams);
          break;
        }

[platan]

I would like not to make another call to Github REST API. Currently Github issue detail integration is able handle issues and pull requests: https://github.com/badges/shields/blob/f4ea029e52af0ab6e974212a5a85703c40c7f704/server.js#L3722
Maybe for this badge (state) we can split responsibilities and handle issues only with https://img.shields.io/github/issues/detail/s/... and pull requests only with https://img.shields.io/github/pulls/detail/s/... ? Github issues API returns data for a pull request, but pull request API returns 404 for issues.

[RedSparr0w]

That's probably the best course of action, might pay to differ merging #1720 and split the front end examples up instead?

[platan]

@RedSparr0w I'm not sure if I understand you correctly, but we can marge #1720 and split "GitHub issue/pull request state" in a PR adding support for "merge" status of a PR (preferred) or alternatively we can split all "GitHub issue/pull request" badge examples (will produce a lot of examples).

[PyvesB]

I would also be inclined to merge #1720 and then iterate on splitting the implementation into two different badges.

[paulmelnikow]

Oops, sorry, I didn't see this before merging #1720, or I would have left it open.

In addition to the status, perhaps checks is another place where we'd want a separate implementation for pull requests.

[ice1000]

I am very looking forward to this. Any chance of having a "merged" status?

I see https://github.com/badges/shields/issues/1719#issuecomment-395527138 and https://github.com/badges/shields/issues/1719#issuecomment-395224356 and https://github.com/badges/shields/issues/1719#issuecomment-394793131 which makes this issue pretty much an implemented issue (while not yet put the work to the master branch).

[calebcartwright]

@ice1000 - just want to clarify this is still a change that would have to first be implemented; it is not already completed and simply pending a merge.

If anyone is interested in implementing it, we'd be happy to review!

[ice1000]

I want to implement that as a separate "pull request" badge without changing current labels (and document the difference somewhere else). Does that sounds like a plan?

[calebcartwright]

I'm not sure what the best approach is at this point. There's been a lot of changes to the code in question since this issue was originally opened and discussed, especially after the refactoring to the new service model.

[ice1000]

Should be closed in favor of #3295

[calebcartwright]

Closed in #3295