Skip to content

/ cli

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

Jump to bottom

Add dotnet help <verb> support #5922

Merged
merged 7 commits into from Mar 16, 2017
Merged

Add dotnet help <verb> support #5922

merged 7 commits into from Mar 16, 2017

Conversation

@blackdwarf
Copy link

@blackdwarf blackdwarf commented Mar 5, 2017

This commit adds supports for getting more detailed help by using the dotnet help <verb> syntax (e.g. dotnet help build). This change opens up the URL that is specified for each verb in the default browser on the user's machine, so internet access is required.

The PR removes the s_builtIns from dotnet/Program.cs into a separate class and adds a class that contains the links. I went this way to minimize code changes overall as well as to create a situation where we have a single place to add a command to the built-ins dictionary so that things do not go out of sync. Another approach is that this could be a property on the CommandLineApplication class that each command can fill in.

/cc @richlander
blackdwarf
Add support for dotnet help <verb>
Loading status checks…
4aacb22 
This commit adds supports for getting more detailed help by using the
`dotnet help <verb>` syntax (e.g. `dotnet help build`). This change
opens up the URL that is specified for each verb in the default browser
on the user's machine, so internet access is required.
@blackdwarf blackdwarf requested review from livarcocc and jonsequitur Mar 5, 2017
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 5, 2017

@dotnet-bot test RHEL7.2 x64 Release Build please
@eerhardt
eerhardt reviewed Mar 6, 2017
View changes
src/dotnet/Program.cs Outdated
@@ -169,10 +127,12 @@ internal static int ProcessArgs(string[] args, ITelemetry telemetryClient = null
telemetryClient.TrackEvent(command, null, null);

int exitCode;
Func<string[], int> builtIn;
if (s_builtIns.TryGetValue(command, out builtIn))
// Func<string[], int> builtIn;

This comment has been minimized.

Sign in to view
@eerhardt

eerhardt Mar 6, 2017
Member

Can be removed

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 6, 2017
Author

Fixed.

src/dotnet/BuiltInCommandsCatalog.cs
{
public static Dictionary<string, BuiltInCommandMetadata> Commands = new Dictionary<string, BuiltInCommandMetadata>
{
["add"] = new BuiltInCommandMetadata

This comment has been minimized.

Sign in to view
@eerhardt

eerhardt Mar 6, 2017
Member

Does this handle sub commands? Ex. 'dotnet help add package'

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 6, 2017
Author

@eerhardt nope, it does not. I guess I could make it work like that, come to thing of it...

This comment has been minimized.

Sign in to view
@jonsequitur

jonsequitur Mar 6, 2017
Collaborator

We should probably add this as a feature into the new parser. @blackdwarf, it'll give you a chance to kick the tires there.

This comment has been minimized.

Sign in to view
@jonsequitur

jonsequitur Mar 6, 2017
Collaborator

The new parser may make this a bit more natural. We could just add DocLink as a property on Command. The advantage there will be that the structure of the commands and options is compiler-enforced and tab completion will light up for free for the help-able commands.

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 11, 2017
Author

@jonsequitur this sounds like a good idea, but see below on the need to possibly keep this information in a separate file. Not saying that this would be impossible in the new parser, of course. :)

@richlander
Copy link
Member

@richlander richlander commented Mar 6, 2017

I'm a big fan of this user experience. It is very similar to "git help pull".

I'm wondering if the aka.ms links are a good choice. There are a problems with them that I see:

  • No one in OSS land knows where they go.
  • There a PITA to change, even if you do have access.

I'd rather see these in a simple text format (in a separate file) and encourage the dotnet/docs repo folks to be partly responsible for the experience. That's really tough with opaque links.

/cc @mairaw @BillWagner
Zlatko Knezevic
Responding to PR feedback
Loading status checks…
4da96e1
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 6, 2017

@richlander the aka.ms links are a so-so choice in my view. You are right that they are opaque and the final URL is not visible unless they are resolved. However, given the moves that will happen with the docs in the next week, this was the only sane way of testing the feature and making sure it is at least somewhat future proof.

There are several options on changing this:

  1. Use a different, more open URL shortener service. Not sure if this helps though.
  2. Keep metadata elsewhere (e.g. in a file as you mention), which incurs the cost of parsing this data on invocation - not sure how good that is.
  3. Simply use full URLs so that it is clear where they resolve, and since our docs are fed from dotnet/docs it is quite OSS-friendly.

Regardless, the approach taken here can support all three above. 😄
@richlander
Copy link
Member

@richlander richlander commented Mar 6, 2017

I wouldn't block your PR on this issue. I'd suggest taking the PR and opening an issue on this topic.

I think the cost of opening a file to read contents should be pretty minimal. It should be what 50ms (on a 5400 RPM disk) to do that work?
@eerhardt
Copy link
Member

@eerhardt eerhardt commented Mar 6, 2017

I think the cost of opening a file to read contents should be pretty minimal.

Agreed. Especially since:

  1. This is only occurred on "help" commands.
  2. You are about to open a web browser and download information from a remote machine, which will be a lot more costly.
@jonsequitur
Copy link
Collaborator

@jonsequitur jonsequitur commented Mar 6, 2017

We discussed a few trade-offs concerning a link versus more thorough on-disk help. The main advantages to the links were that the documentation can be updated more easily and the up-front download of the CLI will be smaller.
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 11, 2017

I've been thinking about this for a while yesterday and I've come to the conclusion that the aka.ms links are not bad. We would want to establish a model where we can have as stable URLs as possible so that any changes to the docs URLs do not necessitate re-shipping of the CLI. aka.ms is just one means to the end with that regard. Changing them I haven't found to be that hard, honestly.

As for the separate file, I plan on making that change to this PR, but with the above I am afraid that I don't see a pressing need. Any change to the CLI, including a simple text file that contains the mappings of commands to their URLs, would have to go through the same PR process as a code change, that is to say, moving the mapping elsewhere does not impact docs' team potential ownership of these links.
@richlander
Copy link
Member

@richlander richlander commented Mar 12, 2017

Ah, that's a good point.

Here's a middle ground ... in the code, you could add a comment that includes the target of the aka.ms link. That would satisfy my concern.
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 12, 2017

@richlander sure, that makes sense. Will do.
Zlatko Knezevic
Responding to PR feedback
Loading status checks…
fa98674
@richlander
Copy link
Member

@richlander richlander commented Mar 14, 2017

Cool. Can you remove "/en-us" from the links? I always remove that. Once we have loc'd docs, then the non-locale specific links make the right choice whereas the locale ones are specific. English FTW, right? Thanks.
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 14, 2017

Done.
Zlatko Knezevic
Removing locale from the commented links
Loading status checks…
36e3d5d
@richlander
Copy link
Member

@richlander richlander commented Mar 14, 2017

LGTM
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 15, 2017

@livarcocc you ok with this change?
@livarcocc
livarcocc reviewed Mar 15, 2017
View changes
test/dotnet-help.Tests/GivenThatIWantToShowHelpForDotnetCommand.cs Outdated
@@ -65,5 +66,39 @@ public void WhenHelpOptionIsPassedToDotnetItPrintsUsage(string helpArg)
cmd.Should().Pass();
cmd.StdOut.Should().ContainVisuallySameFragment(HelpText);
}

[Fact]
public void WhenInvalidCommandIsPassedToDOtnetHelpItPrintsError()

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

NIT: Typo here at ToDOtnet

@livarcocc
livarcocc reviewed Mar 15, 2017
View changes
test/dotnet-help.Tests/GivenThatIWantToShowHelpForDotnetCommand.cs Outdated
.ExecuteWithCapturedOutput("help invalid");

cmd.Should().Fail();
cmd.StdErr.Should().ContainVisuallySameFragment($"Specified command invalid is not a valid CLI command. Please specify a valid CLI commands. For more information, run dotnet help.");

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

Should the command name be in single quotes: Specified command 'invalid' is ...?

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

Also, "...please specify a valid CLI commands" seems wrong.

Should we just go ahead and print the commands here?

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 15, 2017 •
edited
Author

We could, but I wold like to keep the error as well, mostly because it will tell users what happened.

@livarcocc
livarcocc reviewed Mar 15, 2017
View changes
src/dotnet/commands/dotnet-help/HelpCommand.cs Outdated
Cli.BuiltInCommandMetadata builtIn;
if (Cli.BuiltInCommandsCatalog.Commands.TryGetValue(commandNameArgument.Value, out builtIn))
{
// var p = Process.Start(GetProcessStartInfo(builtIn));

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

Remove this comment.

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 15, 2017
Author

Fixed.

@livarcocc
livarcocc reviewed Mar 15, 2017
View changes
src/dotnet/commands/dotnet-help/HelpCommand.cs Outdated

app.OnExecute(() =>
{
Cli.BuiltInCommandMetadata builtIn;

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

Does the type here really need the namespace?

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 15, 2017
Author

Fixed.

@livarcocc
livarcocc reviewed Mar 15, 2017
View changes
src/dotnet/commands/dotnet-help/HelpCommand.cs Outdated
if (Cli.BuiltInCommandsCatalog.Commands.TryGetValue(commandNameArgument.Value, out builtIn))
{
// var p = Process.Start(GetProcessStartInfo(builtIn));
var process = ConfigureProcess(builtIn.DocLink.ToString());

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

Should we make the DocLink a string straight up instead of doing this conversion?

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 15, 2017
Author

Sure.

@livarcocc
livarcocc reviewed Mar 15, 2017
View changes
test/dotnet-help.Tests/GivenThatIWantToShowHelpForDotnetCommand.cs
@@ -8,6 +8,7 @@
using Microsoft.DotNet.Tools.Test.Utilities;

This comment has been minimized.

Sign in to view
@livarcocc

livarcocc Mar 15, 2017

Do we need to test the mapping between commands and URLs?

This comment has been minimized.

Sign in to view
@blackdwarf

blackdwarf Mar 15, 2017
Author

It doesn't have to be a fixed thing. We could, I guess, but they can change.

Zlatko Knezevic added 3 commits Mar 15, 2017
Zlatko Knezevic
Responding to PR feedback
d251734
blackdwarf
Responding to PR feedback
Loading status checks…
30a872b
blackdwarf
Fixing failing test
Loading status checks…
f6ba5c4
@blackdwarf
Copy link
Author

@blackdwarf blackdwarf commented Mar 16, 2017

@livarcocc do say if you're ok with the change to get merged now.
@livarcocc
livarcocc approved these changes Mar 16, 2017
View changes
@livarcocc livarcocc merged commit d347e27 into dotnet:master Mar 16, 2017
9 checks passed
9 checks passed
CentOS7.1 x64 Debug Build Build finished.
Details
Debian8.2 x64 Debug Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
OSX x64 Release Build Build finished.
Details
RHEL7.2 x64 Release Build Build finished.
Details
Ubuntu x64 Release Build Build finished.
Details
Ubuntu16.04 x64 Debug Build Build finished.
Details
Windows_NT x64 Release Build Build finished.
Details
Windows_NT x86 Debug Build Build finished.
Details
@richlander
Copy link
Member

@richlander richlander commented Mar 16, 2017

Woohoo! Thanks @blackdwarf!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerhardt eerhardt

@livarcocc livarcocc

@jonsequitur jonsequitur

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@blackdwarf @richlander @eerhardt @jonsequitur @livarcocc @dnfclas
You can’t perform that action at this time.