mcomella / apt_github_improvements

A browser extension to add features to GitHub for the Android Product Team (APT) at Mozilla
Mozilla Public License 2.0
3 stars 0 forks source link

Link Issues <-> PRs #4

Closed mcomella closed 6 years ago

mcomella commented 6 years ago

Prior art: https://github.com/mcomella/github-issue-hoister/ Current efforts: https://github.com/mcomella/github-issue-pr-linker

Refined GitHub does something like this too: https://github.com/sindresorhus/refined-github/blob/master/source/features/highlight-closing-prs-in-open-issues.js

mcomella commented 6 years ago

To link PR -> issue, the PR must indicate which issue(s) it affects. We can identify this by searching in the:

mcomella commented 6 years ago

To link issue -> PR, we need to leverage the links that PRs have created. In an issue page, GitHub provides us:

Guaranteed issues -> PRs/commits

It's unclear what happens if both the PR comments and commits reference an issue.

Non-guaranteed: PRs reference issue but may not address issue


To reverse this from a user perspective, references in:

Commit links aren't very useful because we have to crawl the commits of all open PRs in order to find which PR it belongs to (afaict). However, the preview API allows us to search issues by commit hash.

mcomella commented 6 years ago

To find guaranteed PR -> issues in all cases (not just "Closes"), we'll have to cache our own PR -> issue data:

Things we may want to report to the user:

Things to watch out for:

mcomella commented 6 years ago

There is also fix and stuff: https://help.github.com/articles/closing-issues-using-keywords/

mcomella commented 6 years ago

There are two types of use cases:

My suggestion algorithm is:

Notes:

Concerns:


My suggested schema:

v: <dbVersion int>,

/* Map issue -> PR; PR -> issue is just scraped */
issue-o/r/<num>: [{ /* PR num */
  num: <int>,
  closes: <boolean>
}]

/* last updates times for rate limiting; we can collapse these if no other keys */
issue-list-o/r: {
  lastUpdated: <int>
}

pr-o/r/<num>: {
  lastUpdated: <int>
}

Have to watch out for:

We're not handling:


FYI: rate limits are:

mcomella commented 6 years ago

I landed the basic implementation. Before release, we additionally need:

We may need:

Things we won't address intentionally, for implementation simplicity/time:

mcomella commented 6 years ago

We also need to consider paging on the github api.

mcomella commented 6 years ago

The MVP is implemented and I filed follow-ups for additional features. In particular, we should consider doing #21 before we ship.