-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Further improve branch getter function #4187
Changes from 40 commits
f6630c5
cb1e4f3
ca22fb3
c80fb13
eb24a4d
db42e67
83e4980
2a58301
4401857
69ef080
659e62e
1a04cdd
1ca241a
04b7396
9e70957
44e1a11
296b022
4254949
7496845
068b21b
38bb092
a673cb3
6318630
f4f8601
179db54
e0e4364
a636300
c6ff873
bc9d068
8b3efcb
89b91a6
2f4b63c
146190a
315f2ab
f43511d
dbd0afc
ccaeeef
5961155
62e24d3
586361b
17381b8
c9b0af6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -34,9 +34,35 @@ export const getCurrentCommittish = (pathname = location.pathname, title = docum | |||||
return parsedTitle.groups!.branch; | ||||||
} | ||||||
|
||||||
// TODO [2021-04-12]: .last needed for #2799 // GHE | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh god. Hah I meant to write the same day, but in 2022 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add |
||||||
const feedLink = select.last('link[type="application/atom+xml"]'); | ||||||
fregante marked this conversation as resolved.
Show resolved
Hide resolved
yakov116 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (feedLink) { | ||||||
return new URL(feedLink.href) | ||||||
.pathname | ||||||
.split('/') | ||||||
.slice(4) // Drops the initial /user/repo/route/ part | ||||||
.join('/') | ||||||
.replace(/\.atom$/, ''); | ||||||
} | ||||||
|
||||||
return unslashedCommittish; | ||||||
}; | ||||||
|
||||||
// Used on `isRepoHome` where can also determine the branch from the feedLink | ||||||
export const getCurrentCommittishAnywhere = async (): Promise<string | undefined> => { | ||||||
fregante marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const committish = getCurrentCommittish(); | ||||||
if (committish || !pageDetect.isRepoHome()) { | ||||||
return committish; | ||||||
} | ||||||
fregante marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
const feedLink = await elementReady('a[data-hotkey="t"]')!; | ||||||
if (!feedLink) { | ||||||
return; | ||||||
} | ||||||
|
||||||
return decodeURIComponent(feedLink.pathname.split('/').pop()!); | ||||||
}; | ||||||
|
||||||
export const isFirefox = navigator.userAgent.includes('Firefox/'); | ||||||
|
||||||
// The type requires at least one parameter https://stackoverflow.com/a/49910890 | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -221,15 +221,17 @@ test('getCurrentCommittish', t => { | |||||||
'github.com' | ||||||||
)); | ||||||||
|
||||||||
document.head.insertAdjacentHTML('beforeend', '<a data-hotkey="t" href="https://github.com/typescript-eslint/typescript-eslint/find/cool%2Fdefault%2Fbranch"></a>'); | ||||||||
// Root | ||||||||
t.is(getCurrentCommittish( | ||||||||
'/typescript-eslint/typescript-eslint', | ||||||||
'typescript-eslint/typescript-eslint: Monorepo for all the tooling which enables ESLint to support TypeScript' | ||||||||
), undefined); | ||||||||
), 'cool/default/branch'); | ||||||||
t.is(getCurrentCommittish( | ||||||||
'/typescript-eslint/typescript-eslint/tree/chore/lerna-4', | ||||||||
'typescript-eslint/typescript-eslint at chore/lerna-4' | ||||||||
), 'chore/lerna-4'); | ||||||||
document.querySelector('[data-hotkey="t"]')!.remove(); | ||||||||
|
||||||||
// Sub folder | ||||||||
t.is(getCurrentCommittish( | ||||||||
|
@@ -289,7 +291,7 @@ test('getCurrentCommittish', t => { | |||||||
t.is(getCurrentCommittish( | ||||||||
'/typescript-eslint/typescript-eslint/commits/chore/lerna-4/docs/getting-started/README.md', | ||||||||
'History for docs/getting-started/README.md - typescript-eslint/typescript-eslint' | ||||||||
), 'chore'); // Wrong, but | ||||||||
), 'master'); // Uses the feedlink | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only shows that the test is wrong, and thus useless. For this to be tested correctly, it should have the correct feedLink, added and removed just for this test like we do for
Suggested change
|
||||||||
|
||||||||
// Single commit | ||||||||
t.is(getCurrentCommittish( | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this link have a file path but not a branch? Steps to reproduce please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/yakov116/refined-github/commits/f23b687b3b89aa95a76193722cdfeff740646670?after=f23b687b3b89aa95a76193722cdfeff740646670+34&path%5B%5D=source&path%5B%5D=features&path%5B%5D=release-download-count.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steps to find find that link on GitHub.com. That's just a URL that anyone can write manually but maybe doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the url we were testing until now. The branch in the path is a new thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That URL does not have a
branch
URL parameter (branchFromSearch
). Where did you see that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I wanted:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this confirms that there's no way that
filePathFromSearch
is truey whilebranchFromSearch
isn't, so it should probably beThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GHE. The branch part is a new thing (at least it was not there when we made GitHubURL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the
pathname
look exactly like now too? Because we can easily extract the slashed branch from there instead, and not use thebranch
parameter at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it did.