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

Factor sync submodules into VS command #1642

Merged
merged 5 commits into from May 4, 2018

Conversation

Projects
None yet
2 participants
@jcansdale
Contributor

jcansdale commented May 2, 2018

What this PR does

  • Extract the sync submodules action into a VS command called
    GitHub.SyncSubmodules

Reviewers

I've extended the command interface to include a method that returns results from the command. Is this a reasonable thing to do if results are sometimes required from a command? It can also be used standalone without returning any results.
https://github.com/github/VisualStudio/blob/36e680e5a0cd936cafbd27af658335727d28841c/src/GitHub.Exports/Commands/ISyncSubmodulesCommand.cs#L16

Related to #1638

Factor sync submodules into VS command
Extract the sync submodules action into a VS command called
GitHub.SyncSubmodules.

@jcansdale jcansdale requested a review from grokys May 2, 2018

@grokys

Looks good, and seems to work well. This could be very handy. Just one question.

if (!complete)
{
statusBarNotificationService.ShowMessage("Failed to sync submodules." + Environment.NewLine + writer);

This comment has been minimized.

@grokys

grokys May 3, 2018

Contributor

Is it normal for VS commands to write directly to the status bar? Is there no concept of "output" for commands, which e.g. shows in the command pane?

This comment has been minimized.

@jcansdale

jcansdale May 3, 2018

Contributor

I think the status bar is what commands tend to use as their ambient UI. There isn't really any concept of a native UI or output for a command.

One possibility would be to pass in a progress object that the command could report back to. This could also sidestep the issue of returning results.

I wanted to start off with a simple refactoring and and avoid any extra implementation detail. We can revisit next time this command is touched (if we decide to surface it somewhere else).

@grokys

grokys approved these changes May 3, 2018

@jcansdale jcansdale merged commit a3aca21 into master May 4, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jcansdale jcansdale deleted the refactor/sync-submodules-vs-command branch May 4, 2018

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