Skip to content

Fix broken patch/diff links on commit PRs. Fixes #104#112

Merged
sindresorhus merged 1 commit into
refined-github:masterfrom
DrewML:issue104
Mar 31, 2016
Merged

Fix broken patch/diff links on commit PRs. Fixes #104#112
sindresorhus merged 1 commit into
refined-github:masterfrom
DrewML:issue104

Conversation

@DrewML

@DrewML DrewML commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

Note: Once #99 goes in, it would make sense to switch out the prCommit regex local to this function with the isPRCommit regex (the one that will be moving to the URL utils file shortly).

@hkdobrev hkdobrev added the bug label Mar 30, 2016
Comment thread extension/content.js Outdated
let commitUrl = location.href.replace(/\/$/, '');
const prCommit = /\/pull\/\d+\/commits/;
if (prCommit.test(commitUrl)) {
commitUrl = commitUrl.replace(prCommit, '/commit');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This check does not handle forks well. I'm not sure if it's easy to support forks in this scenario though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I figured it wouldn't be this easy :) Do you have an example link to a page where it breaks down?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, actually when checking it out, it seems GitHub copies the commits in the base repo so it's working properly even with PRs from forks.

@hkdobrev

Copy link
Copy Markdown
Contributor

LGTM 💫

@sindresorhus

Copy link
Copy Markdown
Member

it would make sense to switch out the prCommit regex local to this function with the isPRCommit regex

#99 landed. Can you do this?

@DrewML

DrewML commented Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

No problem - will push an update after work tonight.

@DrewML

DrewML commented Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

On second thought, I'm going to sit on this until #123 goes in (it's pretty small), and then I'll make the update to consume the new page-detection module.

@hkdobrev

Copy link
Copy Markdown
Contributor

On second thought, I'm going to sit on this until #123 goes in (it's pretty small), and then I'll make the update to consume the new page-detection module.

Hehe, I thought I'll update this code in #123 after this is merged as this is a bugfix and #123 is changing more things and needs to be more thoroughly tested.

@DrewML

DrewML commented Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

@hkdobrev Cool, that works for me. Wasn't going to throw the work over the fence to you, but now I will :)

@DrewML

DrewML commented Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

Update pushed.

@hkdobrev

Copy link
Copy Markdown
Contributor

LGTM :shipit:

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

Labels

Development

Successfully merging this pull request may close these issues.

3 participants