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

Show pull requests to source repo on fork's branch list #2914

Closed
Jackenmen opened this issue Mar 21, 2020 · 17 comments · Fixed by #3116
Closed

Show pull requests to source repo on fork's branch list #2914

Jackenmen opened this issue Mar 21, 2020 · 17 comments · Fixed by #3116

Comments

@Jackenmen
Copy link
Contributor

When some branch on the repo is a head of PR made to that repo, the PR is shown near the branch on branch list:
image

I think it would be great if branch list on forks also included the PRs that are made to their source repo so that user could figure out easily which branches can be removed.

Example URL:
https://github.com/jack1142/Red-DiscordBot/branches/all

@fregante
Copy link
Member

fregante commented Mar 28, 2020

This makes sense and indeed the existing Compare buttons do point to a page that links to the open PR already:

compare

It's just that for whatever reason GitHub doesn't show this information directly on the list.

Does this information appear in the GraphQL API? If so, it could also be useful to find PRs opened from the current repo for #2934

@fregante
Copy link
Member

fregante commented Mar 28, 2020

Here:

query { 
  repository(owner: "jack1142", name: "Red-DiscordBot") {
    refs(refPrefix: "refs/heads/", first: 100) {
      nodes {
        name
        associatedPullRequests(first: 100, states: OPEN) {
          nodes {
            number
            url
          }
        }
      }
    }
  }
}
{
  "data": {
    "repository": {
      "refs": {
        "nodes": [
          {
            "name": "V3/alias_non_existent_command",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/allow_any_valid_locale",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3676,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3676"
                }
              ]
            }
          },
          {
            "name": "V3/bring_back_role_mention_sanitizer",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3625,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3625"
                }
              ]
            }
          },
          {
            "name": "V3/changelog_pr_labeler",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/changelog_3.3.3",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3660,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3660"
                }
              ]
            }
          },
          {
            "name": "V3/develop",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/feature/audio",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/issue_2527",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/issue_3200",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3200,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3200"
                }
              ]
            }
          },
          {
            "name": "V3/issue_3605",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3679,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3679"
                }
              ]
            }
          },
          {
            "name": "V3/regional_formatting",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3677,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3677"
                }
              ]
            }
          },
          {
            "name": "V3/restore_command",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3681,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3681"
                }
              ]
            }
          },
          {
            "name": "V3/some_fun_with_async_initialize",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "develop",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "jack1142-patch-1",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3649,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3649"
                }
              ]
            }
          },
          {
            "name": "jack1142-patch-2",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "jack1142-patch-5",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "jack1142-patch-6",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3672,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3672"
                }
              ]
            }
          },
          {
            "name": "patch-3",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3215,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3215"
                }
              ]
            }
          },
          {
            "name": "pr/ghactions",
            "associatedPullRequests": {
              "nodes": []
            }
          }
        ]
      }
    }
  }
}

@Jackenmen
Copy link
Contributor Author

Looks like it does include them:

query { 
  repository(owner: "jack1142", name: "Red-DiscordBot") {
    refs(refPrefix: "refs/heads/", first: 100) {
      nodes {
        name
        associatedPullRequests(first: 100, states: OPEN) {
          nodes {
            number
            url
          }
        }
      }
    }
  }
}
{
  "data": {
    "repository": {
      "refs": {
        "nodes": [
          {
            "name": "V3/alias_non_existent_command",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/allow_any_valid_locale",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3676,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3676"
                }
              ]
            }
          },
          {
            "name": "V3/bring_back_role_mention_sanitizer",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3625,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3625"
                }
              ]
            }
          },
          {
            "name": "V3/changelog_pr_labeler",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/changelog_3.3.3",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3660,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3660"
                }
              ]
            }
          },
          {
            "name": "V3/develop",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/feature/audio",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/issue_2527",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "V3/issue_3200",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3200,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3200"
                }
              ]
            }
          },
          {
            "name": "V3/issue_3605",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3679,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3679"
                }
              ]
            }
          },
          {
            "name": "V3/regional_formatting",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3677,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3677"
                }
              ]
            }
          },
          {
            "name": "V3/restore_command",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3681,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3681"
                }
              ]
            }
          },
          {
            "name": "V3/some_fun_with_async_initialize",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "develop",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "jack1142-patch-1",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3649,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3649"
                }
              ]
            }
          },
          {
            "name": "jack1142-patch-2",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "jack1142-patch-5",
            "associatedPullRequests": {
              "nodes": []
            }
          },
          {
            "name": "jack1142-patch-6",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3672,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3672"
                }
              ]
            }
          },
          {
            "name": "patch-3",
            "associatedPullRequests": {
              "nodes": [
                {
                  "number": 3215,
                  "url": "https://github.com/Cog-Creators/Red-DiscordBot/pull/3215"
                }
              ]
            }
          },
          {
            "name": "pr/ghactions",
            "associatedPullRequests": {
              "nodes": []
            }
          }
        ]
      }
    }
  }
}

@fregante
Copy link
Member

Alternatively we can go the opposite way, like for #2934, and only query the source repo. This wouldn't support oneFork-to-manyForks PRs but that's pretty rare, I'd say.

The results are shorter and to the point though:

{
  search(
    first: 100,
    type: ISSUE,
    query: "repo:Cog-Creators/Red-DiscordBot is:pr is:open author:jack1142"
	) {
    nodes {
      ... on PullRequest {
        number
        headRefName
      }
    }
  }
}
{
  "data": {
    "search": {
      "nodes": [
        {
          "number": 3681,
          "headRefName": "V3/restore_command"
        },
        {
          "number": 3679,
          "headRefName": "V3/issue_3605"
        },
        {
          "number": 3677,
          "headRefName": "V3/regional_formatting"
        },
        {
          "number": 3676,
          "headRefName": "V3/allow_any_valid_locale"
        },
        {
          "number": 3672,
          "headRefName": "jack1142-patch-6"
        },
        {
          "number": 3660,
          "headRefName": "V3/changelog_3.3.3"
        },
        {
          "number": 3649,
          "headRefName": "jack1142-patch-1"
        },
        {
          "number": 3625,
          "headRefName": "V3/bring_back_role_mention_sanitizer"
        },
        {
          "number": 3215,
          "headRefName": "patch-3"
        },
        {
          "number": 3200,
          "headRefName": "V3/issue_3200"
        }
      ]
    }
  }
}

@Jackenmen
Copy link
Contributor Author

Actually one thing I just noticed with both, I think it would be better to do it like GH does and show all PRs, not just open ones.

@fregante
Copy link
Member

fregante commented Apr 8, 2020

Why all PRs? The point here is to highlight forks that you can/cannot delete, closed PRs don’t seem important for that.

@Jackenmen
Copy link
Contributor Author

The point here is to highlight forks that you can/cannot delete

It's definitely not the point for me, I thought that for the case you're mentioning, there's already this feature:
image

Why all PRs?

I can think of few reasons:
a) GitHub shows associated PR on branch list no matter if it's closed, merged or open so I think it would be good if this feature were consistent with original behavior.
b) not all branches are associated to a PR therefore you can't differentiate easily between branches that are part of closed or merged PR and branches that never had their PR
c) seeing associated PR can serve as a better summary of the work done in the branch - at the very least you can see the title of PR when you hover on its number in branch list and that view also shows small part of description and PR's labels
image
That info can be useful even if the PR has already been merged or closed.
d) if PR was just closed instead of getting merged, it's important to know that removing the branch can cause you losing your work. I'm sure there are plenty of cases when PR doesn't end up being merged

@fregante
Copy link
Member

fregante commented Apr 8, 2020

It's definitely not the point for me, I thought that for the case you're mentioning, there's already this feature:

D'oh, I thought we were talking about that feature. Sorry!

As long as we only show one PR per branch this should be ok, otherwise the layout breaks.

@fregante
Copy link
Member

fregante commented Apr 8, 2020

This could work:

{ 
  repository(owner: "jack1142", name: "Red-DiscordBot") {
    refs(refPrefix: "refs/heads/", last: 100) {
      nodes {
        name
        associatedPullRequests(last: 1, states: [CLOSED, OPEN]) {
          nodes {
            number
            url
          }
        }
      }
    }
  }
}

Ideally you'd query each of the visible branches instead but the query I tried didn't include closed PRs (I tried via repository.ref().associatedPullRequests() and repository().object().associatedPullRequests())

@yakov116
Copy link
Member

Ideally you'd query each of the visible branches instead but the query I tried didn't include closed PRs (I tried via repository.ref().associatedPullRequests() and repository().object().associatedPullRequests())

Can you explain what you had in mind with this comment?

Also why are you limiting it to only closed and open states?

@fregante
Copy link
Member

My last example just asks GitHub for the last 100 branches and their related PR. Ideally instead you'd ask GitHub just for the PRs that are currently on screen, like

{
	repository(...) {
		branchA: ref('branchA') {
			associatedPullRequests(...) {
				...
			}
		}
		branchB: ref('branchB') {
			associatedPullRequests(...) {
				...
			}
		}
		...
	}
}

But none of the queries I tried included closed/merged PRs; they only returned open PRs. If anyone finds a way to query branches this way AND their related PR (even if closed) it's preferable instead of my last example.

@fregante
Copy link
Member

fregante commented Apr 17, 2020

Also why are you limiting it to only closed and open states?

Mistake. I meant open/closed/merged.

@yakov116
Copy link
Member

yakov116 commented Apr 19, 2020

@fregante

{
  repository(owner: "yakov116", name: "refined-github") {
    console_errors: ref(qualifiedName: "console_errors") {
      associatedPullRequests(last: 1) {
        nodes {
          number
          state
        }
      }
    }
    clone_branch: ref(qualifiedName: "restore-clone-branch") {
      associatedPullRequests(last: 1) {
        nodes {
          number
          state
        }
      }
    }
  }
}
{
  "data": {
    "repository": {
      "console_errors": {
        "associatedPullRequests": {
          "nodes": [
            {
              "number": 2956,
              "state": "CLOSED"
            }
          ]
        }
      },
      "clone_branch": {
        "associatedPullRequests": {
          "nodes": [
            {
              "number": 3000,
              "state": "MERGED"
            }
          ]
        }
      }
    }
  }
}

Unless you had something else in mind.

I would much rather do the last 100 PR's since any search will require a new request.
i also think its safe to say that most people with a fork have less than 100 branches

Edit: Showing that it works for merged

@Jackenmen
Copy link
Contributor Author

In that case, isn't it simpler to just do it like here?
#2914 (comment)

@yakov116
Copy link
Member

yakov116 commented Apr 20, 2020

In that case, isn't it simpler to just do it like here?
#2914 (comment)

I learned the hard way; when @fregante says something, follow it. Have questions... ask later.

@fregante
Copy link
Member

In that case, isn't it simpler to just do it like here?

That works, but if the alternative option is possible the result is safer and lighter. If it’s not possible, then sure.

@fregante
Copy link
Member

I would much rather do the last 100 PR's since any search will require a new request.
i also think its safe to say that most people with a fork have less than 100 branches

Maybe you’re right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants