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.

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

Badge request: GitHub merged pull requests #6164

Open
AraHaan opened this issue Feb 12, 2021 · 14 comments
Open

Badge request: GitHub merged pull requests #6164

AraHaan opened this issue Feb 12, 2021 · 14 comments
Labels
service-badge Accepted and actionable changes, features, and bugs

Comments

@AraHaan
Copy link

AraHaan commented Feb 12, 2021

📋 Description

I would like a badge that counts all of the pull requests that was merged only, the closed pr badge could be changed I think to filter out merged prs.

  • The badge is for GitHub.
  • What sort of information should this badge show?
    The badge should contain the count of merged pull requests, also the count of merged pull requests could also be used to subtract from the closed pull request badge to (or well make a version of it that excludes merged prs).

🔗 example

Where can we get the data from?
list the pull requests first using GET https://api.github.com/repos/{owner}/{repo}/pulls then request:
GET https://api.github.com/repos/{owner}/{repo}/pulls/{pull_number}/merge

🎤 Motivation

I noticed that we could show up opened, closed issues and pull requests among other things however lacked the ability to get a badge for merged pull requests.

  • What is the specific use case?
    People like me want to show that we merge pull requests a ton and it would support those who feel like contributing to projects too.
@AraHaan AraHaan added the service-badge Accepted and actionable changes, features, and bugs label Feb 12, 2021
@AraHaan
Copy link
Author

AraHaan commented Feb 12, 2021

It seems there is another way as well to -> https://github.com/Elskom/Sdk/pulls?q=is%3Apr+is%3Aclosed+is%3Aunmerged
returns only unmerged pr's while -> https://github.com/Elskom/Sdk/pulls?q=is%3Apr+is%3Aclosed+is%3Amerged
returns only merged.

@calebcartwright
Copy link
Member

Thanks! I think that the API information is not correct for what you're asking for though, or at least not a viable option.

We actually already support distinguishing between merged vs. closed for individual PRs

#6159
https://img.shields.io/github/pulls/detail/state/badges/shields/6159

#6161
https://img.shields.io/github/pulls/detail/state/badges/shields/6161

The difference here is the desire to filter against the total PR count for merged vs. closed, and this is actually much simpler to do via the v4/GraphQL API. Fortunately, our existing badge that reports total for both issues and PRs already does this!

query(
$user: String!
$repo: String!
$states: [PullRequestState!]
$labels: [String!]
) {
repository(owner: $user, name: $repo) {
pullRequests(states: $states, labels: $labels) {
totalCount
}
}
}
`,
variables: {
...commonVariables,
states: isClosed ? ['MERGED', 'CLOSED'] : ['OPEN'],
},
schema: pullRequestCountSchema,

Think all that would be needed to make this happen is an additional route variant (issues-pr-merged, for consistency)

static route = {
base: 'github',
pattern:
':variant(issues|issues-closed|issues-pr|issues-pr-closed):raw(-raw)?/:user/:repo/:label*',
}

and then tweaking the respective signatures to support passing through the merged-only option, and using the state filters accordingly:

variables: {
...commonVariables,
states: isClosed ? ['MERGED', 'CLOSED'] : ['OPEN'],
},

closed = merged+closed states
merged = merged state
otherwise open state

@AraHaan
Copy link
Author

AraHaan commented Feb 12, 2021

@calebcartwright can we also have an issues-pr-closed-unmerged too eventually?

@calebcartwright
Copy link
Member

issues-pr-closed-unmerged too eventually?

would probably need to bikeshed on the name a bit, but i don't see why not!

@calebcartwright
Copy link
Member

Good point Chris. Do you think we should go ahead and close this (folks can always use the color query param if they want it to have the same purple color) or would it be worthwhile to make those options available like the other PR aggregate status badges?

@AraHaan
Copy link
Author

AraHaan commented Feb 13, 2021

I would like the options available like the other pr aggregate badges.

Also @chris48s it does not say merged or unmerged at the end either.

@AraHaan
Copy link
Author

AraHaan commented Feb 13, 2021

https://github.com/badges/shields/blob/master/services/github/github-issues-search.service.js#L26-L28

I see from here it does not let you set text at the end, perhaps have an optional query param for it that defaults to nothing?.

@calebcartwright
Copy link
Member

I see from here it does not let you set text at the end, perhaps have an optional query param for it that defaults to nothing?.

No, this is by design. We do not allow open ended badge message customization #6135. For the issues status badges, we have two kinds, one that puts the status in the label (raw) and another that puts it as a suffix in the message:

https://img.shields.io/github/issues-pr-raw/badges/shields

https://img.shields.io/github/issues-pr/badges/shields

the former is already possible via the issue search badge and query params as noted above, the latter would not be.

i'm not personally opposed to to including the additional PR status variants, but it does seem like quite a bit of overhead just to get the non-raw versions (status on the message)

@paulmelnikow paulmelnikow changed the title Badge for merged pull requests. Badge request: GitHub merged pull requests Jul 9, 2021
@jaas666
Copy link

jaas666 commented May 12, 2022

This issue seems like it was solved.
Can I suggest closing it?

@calebcartwright
Copy link
Member

This issue seems like it was solved. Can I suggest closing it?

Thanks for the question! However, no, I don't see this as resolved.

It's often the case that there's a viable alternative/workaround available via existing features such as our dynamic badge features (or custom endpoint), but that doesn't inherently negate/resolve the request for a native badge.

This issue was requesting a native badge that doesn't yet exist (though which has some alternative as noted earlier in the thread). From the maintainer side we've not explicitly said "no we won't do this", and the author hasn't changed their mind yet either and closed the issue themselves.

I don't anticipate the issue being closed unless and until one of those two things happen, or unless someone implements the requested badge.

@jaas666
Copy link

jaas666 commented May 12, 2022 via email

@PaulaBarszcz
Copy link
Collaborator

I would like to work on this one :)

@Hronom
Copy link

Hronom commented May 19, 2023

Please someone work on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants