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

Open PR link to the selected (not active) repo on GitHub #1214

Merged
merged 12 commits into from Sep 5, 2017

Conversation

Projects
None yet
2 participants
@jcansdale
Contributor

jcansdale commented Sep 1, 2017

Previously the # link in the PR list wasn't taking account of the selected repo (i.e. fork or target repo). This meant that if you tried to open a PR that you'd submitted from a fork, you'd get a 404 rather than the PR you'd submitted opening. Alternatively you might get the wrong PR, if a PR/issue with the same number exists in your local repo.

For example, the link highlighted below attempts to open jcansdale/VsixUtil not jaredpar/VsixUtil:

image

This PR changes it to open the selected repo. It also moves the OpenPROnGitHub command from the View to ViewModel (with the other commands).

I'm also planning to refactor the command to use Rx for consistency with the other commands in the ViewModel.

Fixes #1208

@jcansdale jcansdale requested a review from shana Sep 1, 2017

@grokys

Looks good in general, though to me opening a browser is more of a view level than a view model level concern, but we could debate this forever and never reach a conclusion.

The argument for these commands being in the view model would be to facilitate unit testing, so I'm going to request a unit test ;)

@@ -269,6 +276,8 @@ public IAccount EmptyUser
public ReactiveCommand<object> OpenPullRequest { get; }
public ReactiveCommand<object> CreatePullRequest { get; }
public ReactiveCommand<object> OpenPROnGitHub { get; }

This comment has been minimized.

@grokys

grokys Sep 1, 2017

Contributor

If we're moving things about here, might it be worth making the naming more consistent, i.e. "OpenPullRequestOnGitHub`?

@jcansdale

Looks like I also need to fix this link.
image

It seems to be a similar symptom but a different cause. I'll split this into a separate issue/PR.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Sep 5, 2017

@grokys now with tests and consistent naming. 😄

@grokys

Better! Just one small thing.

@@ -334,5 +343,12 @@ void DoCreatePullRequest()
var d = new ViewWithData(UIControllerFlow.PullRequestCreation);
navigate.OnNext(d);
}
void DoOpenPROnGitHub(int pullRequest)

This comment has been minimized.

@grokys

grokys Sep 5, 2017

Contributor

This should probably be DoOpenPullRequestOnGitHub now :)

@grokys

grokys approved these changes Sep 5, 2017

Looks good now!

@jcansdale jcansdale merged commit ad67d7f into master Sep 5, 2017

5 checks passed

GitHub CLA @jcansdale has accepted the GitHub Contributor License Agreement.
Details
VisualStudio Build #7869059 succeeded in 94s
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins/build_log Jenkins Build Log
Details

@jcansdale jcansdale deleted the fixes/1208-dont-link-to-fork branch Sep 5, 2017

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