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

Shorten no-referrer GitHub URLs too #33

Merged
merged 9 commits into from
Nov 16, 2021
Merged

Shorten no-referrer GitHub URLs too #33

merged 9 commits into from
Nov 16, 2021

Conversation

cheap-glitch
Copy link
Contributor

@cheap-glitch cheap-glitch commented Nov 9, 2021

test.js Show resolved Hide resolved
Co-authored-by: Federico Brigante <[email protected]>
@fregante
Copy link
Member

fregante commented Nov 9, 2021

Yeah it doesn't because those links are usually shortened natively. To properly add support for refined-github/refined-github#4981 the regular issues should also work. The example URL in that issue isn't shortened

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably good 😃

@cheap-glitch

This comment has been minimized.

@fregante

This comment has been minimized.

test.js Outdated Show resolved Hide resolved
@cheap-glitch cheap-glitch marked this pull request as draft November 9, 2021 20:39
@cheap-glitch cheap-glitch marked this pull request as ready for review November 15, 2021 12:01
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this should support issues and PRs even without isRedirection, to both work as a general-purpose shortener (rather than extension-specific) and to (I think) support comment links like

#28 (comment)
#33 (review)

Currently isRedirection does not display that (comment)/(review) part


However non-isRedirection support isn't required for this PR.

index.js Outdated Show resolved Hide resolved
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

@fregante fregante changed the title Shorten GitHub redirect domains Shorten no-referral GitHub URLs too Nov 16, 2021
@fregante fregante merged commit 2c4fadf into main Nov 16, 2021
@fregante fregante deleted the shorten-redirects branch November 16, 2021 09:19
@fregante fregante changed the title Shorten no-referral GitHub URLs too Shorten no-referrer GitHub URLs too Nov 16, 2021
@@ -4,7 +4,9 @@ const patchDiffRegex = /[.](patch|diff)$/;
const releaseRegex = /^releases[/]tag[/]([^/]+)/;
const labelRegex = /^labels[/]([^/]+)/;
const compareRegex = /^compare[/]([^/]+)/;
const pullRegex = /^pull[/](\d+)[/]([^/]+)(?:[/]([\da-f]{40})[.][.]([\da-f]{40}))?$/;
const pullRegex = /^pull[/](\d+)(?:[/]([^/]+))?(?:[/]([\da-f]{40})[.][.]([\da-f]{40}))?$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to use named groups here to describe what each group is capturing, even if unused. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes 👍

@fregante
Copy link
Member

You can open a PR with the same title on refined GitHub and install just this dependency

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

Successfully merging this pull request may close these issues.

Support GitHub redirect domains in shorten-links
2 participants