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

Add `list-prs-for-file` feature #2197

Merged
merged 24 commits into from Jul 14, 2019

Conversation

Projects
None yet
3 participants
@jerone
Copy link
Contributor

commented Jun 30, 2019

Description

Highlight affected PRs in file.

Closes

Closes #1323

Test

TODO

  • Caching.
  • Improve name. Suggestions...
  • Consider getting PR's from original when in own fork?

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@jerone jerone force-pushed the jerone:highlight-file-in-prs branch from 9a35ebf to b6e17f8 Jul 1, 2019

@jerone jerone force-pushed the jerone:highlight-file-in-prs branch from b6e17f8 to 3becb96 Jul 1, 2019

Fix
@@ -107,6 +107,7 @@ import './features/highest-rated-comment';
import './features/clean-issue-filters';
import './features/minimize-upload-bar';
import './features/cycle-lists-with-keyboard-shortcuts';
import './features/highlight-file-in-prs';

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 3, 2019

Collaborator

Maybe:

  • open-prs-for-file (but it sounds like a verb to open a PR)
  • list-prs-for-file
  • list-prs-with-file
  • list-prs-editing-current-file

This comment has been minimized.

Copy link
@jerone

jerone Jul 3, 2019

Author Contributor

I included the word highlight because it's design is somewhat related to highlight-closing-prs-in-open-issues.

What about:

  • highlight-file-altered-in-pr
  • file-altered-in-pr
  • altered-in-pr
  • file-used-in-pr

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 4, 2019

Collaborator

In that case highlight kinda works because it brings the same information from the bottom of the thread to the top. In this case this information is fetched from elsewhere.

This feature is about showing a list of PRs related to a file, so wouldn’t it make sense to focus the name on that? Technically it’s show-prs-containing-the-current-file, not the other way around. We’re not “highlighting files”, we’re “adding links towards PRs”

This comment has been minimized.

Copy link
@jerone

jerone Jul 4, 2019

Author Contributor

I agree with what you are saying. It is the otherway around.

What about highlight-affected-prs-in-file... The more I think about this one, the more it has my preference.

This comment has been minimized.

Copy link
@jerone

jerone Jul 5, 2019

Author Contributor

I went with highlight-affected-prs-in-file. The name is long, but it captures exactly what it does.

Show resolved Hide resolved source/features/highlight-file-in-prs.tsx Outdated
Show resolved Hide resolved source/features/highlight-file-in-prs.tsx Outdated
Show resolved Hide resolved source/features/highlight-file-in-prs.tsx Outdated
Show resolved Hide resolved source/features/highlight-file-in-prs.tsx Outdated

jerone and others added some commits Jul 3, 2019

Grammar
Co-Authored-By: Federico Brigante <github@bfred.it>
self-closing
Co-Authored-By: Federico Brigante <github@bfred.it>

@jerone jerone force-pushed the jerone:highlight-file-in-prs branch from b4badaf to 1ba5498 Jul 5, 2019

@jerone jerone changed the title Add `highlight-file-in-prs` feature Add `highlight-affected-prs-in-file` feature Jul 5, 2019

@jerone jerone marked this pull request as ready for review Jul 5, 2019

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@bfred-it & @sindresorhus This is ready for testing and review 🐱‍👤

@bfred-it

This comment was marked as resolved.

Copy link
Collaborator

commented Jul 8, 2019

Please run this to delete the tags from your fork and locally, we keep pulling them from old forks and pushing some troublesome ones back to this repo

git tag -l | xargs git tag -d
git ls-remote --tags --refs origin | cut -f2 | xargs git push origin --delete
@jerone

This comment was marked as resolved.

Copy link
Contributor Author

commented Jul 8, 2019

@bfred-it Am getting error when running this in bash:

$ git ls-remote --tags --refs origin | cut -f2 | xargs git push origin --delete
fatal: --delete doesn't make sense without any refs

Or in cmd and PowerShell:

$ git ls-remote --tags --refs origin | cut -f2 | xargs git push origin --delete
'cut' is not recognized as an internal or external command,
operable program or batch file.
@bfred-it

This comment was marked as resolved.

Copy link
Collaborator

commented Jul 8, 2019

Does this work:

git fetch
git push origin --delete $(git tag -l)
git tag -d $(git tag -l)

From: https://gist.github.com/okunishinishi/9424779

@jerone

This comment was marked as resolved.

Copy link
Contributor Author

commented Jul 8, 2019

@bfred-it commented on Jul 8, 2019, 11:37 AM GMT+2:

Does this work:

git fetch
git push origin --delete $(git tag -l)
git tag -d $(git tag -l)

From: gist.github.com/okunishinishi/9424779

Same issue:

$ git push origin --delete $(git tag -l)
fatal: --delete doesn't make sense without any refs

Whatever I do, at every fetch I get all tags back. :(

Edit
It looks like they are coming from here: https://github.com/sindresorhus/refined-github/tags
Mine are empty: https://github.com/jerone/refined-github/tags

Edit 2:
I removed git remote to this repo for now. Will wait a few days for the tags to be cleaned before adding it back.

@bfred-it

This comment was marked as resolved.

Copy link
Collaborator

commented Jul 8, 2019

Mine are empty: jerone/refined-github/tags

That's probably why you get that error: fatal: --delete doesn't make sense without any refs

All good now, I think I saw the tags on your fork earlier but when I checked again they were gone, the code probably worked the first time you ran it.

No worries about the tags now, you can add the remote and not worry about them.

Merge branch 'master' into highlight-file-in-prs
# Conflicts:
#	source/content.ts
Merge branch 'master' into highlight-file-in-prs
# Conflicts:
#	source/content.ts

@bfred-it bfred-it changed the title Add `highlight-affected-prs-in-file` feature Add `list-prs-for-file` feature Jul 14, 2019

Rename
PRs are not affected, *files* are affected

PRs are not highlighted (since they don't appear anywhere in the page, we're *adding* them, not *highlighting* them)

@bfred-it bfred-it self-assigned this Jul 14, 2019

bfred-it added some commits Jul 14, 2019

Limit PRs in original cached object
... rather than while showing them
Make `fetch` function name self-explanatory
And avoid confusion with native `window.fetch`
Move list above commit box on Edit pages
And reduce CSS also for `branch-buttons`
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

I made them more prominent on Edit pages:

@bfred-it bfred-it merged commit 4eab1fb into sindresorhus:master Jul 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jerone jerone deleted the jerone:highlight-file-in-prs branch Jul 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.