Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Open PR link to the selected (not active) repo on GitHub #1214
Conversation
We know SelectedRepository and IVisualStudioBrowser shouldn't be null.
|
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; } | |||
grokys
Sep 1, 2017
Contributor
If we're moving things about here, might it be worth making the naming more consistent, i.e. "OpenPullRequestOnGitHub`?
If we're moving things about here, might it be worth making the naming more consistent, i.e. "OpenPullRequestOnGitHub`?
|
Looks like I also need to fix this link. It seems to be a similar symptom but a different cause. I'll split this into a separate issue/PR. |
|
@grokys now with tests and consistent naming. |
|
Better! Just one small thing. |
| @@ -334,5 +343,12 @@ void DoCreatePullRequest() | |||
| var d = new ViewWithData(UIControllerFlow.PullRequestCreation); | |||
| navigate.OnNext(d); | |||
| } | |||
|
|
|||
| void DoOpenPROnGitHub(int pullRequest) | |||
grokys
Sep 5, 2017
Contributor
This should probably be DoOpenPullRequestOnGitHub now :)
This should probably be DoOpenPullRequestOnGitHub now :)
|
Looks good now! |
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/VsixUtilnotjaredpar/VsixUtil: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