Further improve branch getter function #4187

yakov116 · 2021-04-01T14:27:07Z

Fixes #4196
Fixes #4271

source/github-helpers/index.ts

fregante · 2021-04-01T14:50:20Z

@fregante please let me know if we should stop the next release

Good idea, disabled.

Its not in the head of the page so...

That kills the cat.

Ensure 2 things:

If the element isn't found, the feature still works, somehow
If the element isn't found, use console.warn to alert us

source/github-helpers/index.ts

yakov116 · 2021-04-01T14:52:00Z

If the element isn't found, the feature still works, somehow

This is going to be very hard its used in 15 features.

source/github-helpers/index.ts

fregante · 2021-04-01T15:08:51Z

Does this PR also fix #4178?

yakov116 · 2021-04-01T15:09:58Z

No that is isPrFiles

yakov116 · 2021-04-01T16:25:17Z

@fregante ok I figured out a way to make this DOM safe in most cases. The title has the word ' at ' in it on many pages.

But I dont know regex so I wrote it...

fregante · 2021-04-01T16:40:16Z

One thing to note is that this function only really needs the branch to support branches with slashes. In every other case we really don’t care if it exists or not, it makes no difference (if I remember correctly). So in most cases the branch parser can return undefined and the feature should still work.

yakov116 · 2021-04-01T16:42:27Z

One thing to note is that this function only really needs the branch to support branches with slashes. In every other case we really don’t care if it exists or not, it makes no difference (if I remember correctly). So in most cases the branch parser can return undefined and the feature should still work.

I think you are correct

source/github-helpers/index.ts

fregante · 2021-04-02T17:05:43Z

Probably my PR will have to be integrated with this. I visited https://github.com/eslint/eslint and there's zero mention of master in the raw HTML request, which means everything is deferred.

Generally this is not a problem, but getCurrentBranch is used by getDefaultBranch so either we await the DOM in that case or just let it do a query.

fregante · 2021-04-02T17:42:32Z

just let it do a query

That's what #4190 is doing at the moment.

This PR can be merged in the next version, if relevant

fregante · 2021-04-03T16:51:03Z

Parts of this have been added by #4193. Here I'm hoping that we can use the DOM to fill in the details in the remaining case:

slashed branches in commits pages, like follow-file-renames should handle branch names with slashes, but doesn't #4196

This is because on that page:

there's no branch selector
there's no branch mentioned in the title

yakov116 · 2021-04-05T04:50:50Z

Oh man this is not going to be a fun merge

source/github-helpers/index.ts

Co-authored-by: Federico Brigante <[email protected]>

yakov116 · 2021-04-12T13:37:44Z

🤷 I don't know

yakov116 · 2021-04-15T13:56:54Z

@fregante I tried last night again to figure this out, with no luck.

I don't care to close and let someone else attempt. Please do as you see fit.

fregante · 2021-04-17T00:49:45Z

What’s missing here other than the open reviews?

Also if it’s used by any feature (and if it doesn’t already work), on the file editing page there’s a new place to read the branch name from. https://twitter.com/natfriedman/status/1383169134096093187?s=21

yakov116 · 2021-04-18T01:10:39Z

What’s missing here other than the open reviews?

The tests, I cant figure out how to make it pass.

source/features/index.tsx

fregante · 2021-04-18T18:56:57Z

test/github-url.ts

@@ -3,6 +3,8 @@ import test from 'ava';
 import './fixtures/globals';
 import GitHubURL from '../source/github-helpers/github-url';

+document.head.insertAdjacentHTML('beforeend', '<a hidden="" data-hotkey="t" data-pjax="true" href="https://github.com/avajs/ava/find/master"></a>');


Suggested change

document.head.insertAdjacentHTML('beforeend', '<a hidden="" data-hotkey="t" data-pjax="true" href="https://github.com/avajs/ava/find/master"></a>');

document.head.insertAdjacentHTML('beforeend', '<a hidden="" data-hotkey="t" data-pjax="true" href="https://github.com/sindresorhus/refined-github/find/Slash%2Fslash"></a>');

Actually this and the .remove() call can't be here. They need to be inside the test() function that uses it

test/get-current-committish.ts

This reverts commit 4254949.

yakov116 · 2021-04-19T13:09:07Z

source/github-helpers/index.ts

@@ -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


Suggested change

// TODO [2021-04-12]: .last needed for #2799 // GHE

// TODO [2021-08-12]: .last needed for #2799 // GHE

https://github.com/sindresorhus/refined-github/runs/2376926124

Oh god. Hah I meant to write the same day, but in 2022

Can you also add unicorn/expiring-todo-comments: warn in the xo settings? I don't want it to automatically fail lint.

fregante · 2021-04-24T09:26:04Z

test/helpers.ts

@@ -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


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 data-hotkey="t" (including the code to add it before the test)

Suggested change

), 'master'); // Uses the feedlink

), 'chore/lerna-4');

document.querySelector('the feed link')!.remove();

fregante · 2021-04-24T09:35:46Z

I'm closing this PR because extremely long. Please send a new one if you have time, resolving all the review comments and squashing the commits. Or leave it for someone else because we've gone over the same code several times.

yakov116 · 2021-04-25T01:31:32Z

I'm closing this PR because extremely long. Please send a new one if you have time, resolving all the review comments and squashing the commits. Or leave it for someone else because we've gone over the same code several times.

I will leave it for someone else.

Fix GitHubURL - excluding PR Files

f6630c5

yakov116 commented Apr 1, 2021

View reviewed changes

source/github-helpers/index.ts Outdated Show resolved Hide resolved

yakov116 added 2 commits April 1, 2021 10:47

Fix tests

cb1e4f3

Please check for typo before committing

ca22fb3

fregante added the bug label Apr 1, 2021

fregante reviewed Apr 1, 2021

View reviewed changes

source/github-helpers/index.ts Show resolved Hide resolved

fregante reviewed Apr 1, 2021

View reviewed changes

source/github-helpers/index.ts Outdated Show resolved Hide resolved

yakov116 added 3 commits April 1, 2021 11:40

Fix it for now

c80fb13

Merge remote-tracking branch 'upstream/githuburl' into githuburl

eb24a4d

One BIG Mess

db42e67

yakov116 changed the title ~~Fix GitHubURL - excluding PR Files~~ Fix GitHubURL - Not DOM ready safe Apr 1, 2021

Lets use the title

83e4980

fregante self-assigned this Apr 2, 2021

fregante reviewed Apr 2, 2021

View reviewed changes

source/github-helpers/index.ts Outdated Show resolved Hide resolved

fregante mentioned this pull request Apr 2, 2021

Improve branch getter function #4190

Merged

1 task

fregante marked this pull request as draft April 2, 2021 17:43

This was referenced Apr 3, 2021

Improve branch getter function #4193

Merged

follow-file-renames should handle branch names with slashes, but doesn't #4196

Closed

fregante changed the title ~~Fix GitHubURL - Not DOM ready safe~~ Further improve branch getter function Apr 3, 2021

fregante reviewed Apr 12, 2021

View reviewed changes

source/github-helpers/index.ts Outdated Show resolved Hide resolved

yakov116 and others added 5 commits April 12, 2021 04:17

Update source/github-helpers/index.ts

c6ff873

Co-authored-by: Federico Brigante <[email protected]>

Update source/github-helpers/index.ts

bc9d068

Co-authored-by: Federico Brigante <[email protected]>

Update source/features/more-dropdown.tsx

8b3efcb

Co-authored-by: Federico Brigante <[email protected]>

Not sure what I did here

89b91a6

Add comment

2f4b63c

fregante reviewed Apr 18, 2021

View reviewed changes

source/features/index.tsx Outdated Show resolved Hide resolved

fregante added 2 commits April 19, 2021 01:38

Undo TS change

146190a

Update default slashed branch test

315f2ab

fregante reviewed Apr 18, 2021

View reviewed changes

fregante added 2 commits April 19, 2021 01:57

Use correct /find/ URL

f43511d

Waiting for element-ready@6

dbd0afc

fregante reviewed Apr 18, 2021

View reviewed changes

test/get-current-committish.ts Outdated Show resolved Hide resolved

fregante and others added 4 commits April 19, 2021 02:03

Restore index.tsx

ccaeeef

Merge remote-tracking branch 'upstream/main' into githuburl

5961155

Revert "Move to its own file"

62e24d3

This reverts commit 4254949.

Fix import

586361b

yakov116 commented Apr 19, 2021

View reviewed changes

yakov116 added 2 commits April 19, 2021 09:09

Move TODO date

17381b8

Merge branch 'main' into githuburl

c9b0af6

fregante reviewed Apr 24, 2021

View reviewed changes

fregante closed this Apr 24, 2021

yakov116 deleted the githuburl branch April 25, 2021 01:31

fregante mentioned this pull request May 1, 2021

Fix latest-tag-button on repo home page #4302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further improve branch getter function #4187

Further improve branch getter function #4187

yakov116 commented Apr 1, 2021 •

edited by fregante

fregante commented Apr 1, 2021

yakov116 commented Apr 1, 2021

fregante commented Apr 1, 2021

yakov116 commented Apr 1, 2021

yakov116 commented Apr 1, 2021

fregante commented Apr 1, 2021

yakov116 commented Apr 1, 2021

fregante commented Apr 2, 2021

fregante commented Apr 2, 2021 •

edited

fregante commented Apr 3, 2021

yakov116 commented Apr 5, 2021

yakov116 commented Apr 12, 2021

yakov116 commented Apr 15, 2021

fregante commented Apr 17, 2021

yakov116 commented Apr 18, 2021

fregante Apr 18, 2021

fregante Apr 18, 2021 •

edited

yakov116 Apr 19, 2021

fregante Apr 21, 2021

yakov116 Apr 21, 2021

fregante Apr 24, 2021

fregante Apr 24, 2021

fregante Apr 24, 2021 •

edited

fregante commented Apr 24, 2021 •

edited

yakov116 commented Apr 25, 2021

	document.head.insertAdjacentHTML('beforeend', '<a hidden="" data-hotkey="t" data-pjax="true" href="https://github.com/avajs/ava/find/master"></a>');
	document.head.insertAdjacentHTML('beforeend', '<a hidden="" data-hotkey="t" data-pjax="true" href="https://github.com/sindresorhus/refined-github/find/Slash%2Fslash"></a>');

	// TODO [2021-04-12]: .last needed for #2799 // GHE
	// TODO [2021-08-12]: .last needed for #2799 // GHE

	), 'master'); // Uses the feedlink
	), 'chore/lerna-4');
	document.querySelector('the feed link')!.remove();

Further improve branch getter function #4187

Further improve branch getter function #4187

Conversation

yakov116 commented Apr 1, 2021 • edited by fregante

fregante commented Apr 1, 2021

yakov116 commented Apr 1, 2021

fregante commented Apr 1, 2021

yakov116 commented Apr 1, 2021

yakov116 commented Apr 1, 2021

fregante commented Apr 1, 2021

yakov116 commented Apr 1, 2021

fregante commented Apr 2, 2021

fregante commented Apr 2, 2021 • edited

fregante commented Apr 3, 2021

yakov116 commented Apr 5, 2021

yakov116 commented Apr 12, 2021

yakov116 commented Apr 15, 2021

fregante commented Apr 17, 2021

yakov116 commented Apr 18, 2021

fregante Apr 18, 2021

Choose a reason for hiding this comment

fregante Apr 18, 2021 • edited

Choose a reason for hiding this comment

yakov116 Apr 19, 2021

Choose a reason for hiding this comment

fregante Apr 21, 2021

Choose a reason for hiding this comment

yakov116 Apr 21, 2021

Choose a reason for hiding this comment

fregante Apr 24, 2021

Choose a reason for hiding this comment

fregante Apr 24, 2021

Choose a reason for hiding this comment

fregante Apr 24, 2021 • edited

Choose a reason for hiding this comment

fregante commented Apr 24, 2021 • edited

yakov116 commented Apr 25, 2021

yakov116 commented Apr 1, 2021 •

edited by fregante

fregante commented Apr 2, 2021 •

edited

fregante Apr 18, 2021 •

edited

fregante Apr 24, 2021 •

edited

fregante commented Apr 24, 2021 •

edited