Skip to content
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

Fix the help command to work with intrinsic commands. #1231

Merged
merged 1 commit into from Feb 16, 2016

Conversation

@AustinWise
Copy link

@AustinWise AustinWise commented Feb 4, 2016

The old "dotnet help" code assumed every command is defined by a "dotnet-command" executable. However this is no longer true for intrinsic commands.

I tried this change out with both intrinsic and extrinsic commands and they now print help as expected.

I also added some tests to make sure the "dotnet help" command executes successfully.

@dnfclas
Copy link

@dnfclas dnfclas commented Feb 4, 2016

Hi @AustinWise, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@AustinWise
Copy link
Author

@AustinWise AustinWise commented Feb 4, 2016

The OS X build failed for an odd reason (build log said git was not installed). I rebased onto the tip of 1.0.0 and pushed to get it to try building again.

@@ -113,9 +113,25 @@ private static int ProcessArgs(string[] args)

var appArgs = (lastArg + 1) >= args.Length ? Enumerable.Empty<string>() : args.Skip(lastArg + 1).ToArray();

if (string.IsNullOrEmpty(command) || command.Equals("help", StringComparison.OrdinalIgnoreCase))
if (string.IsNullOrEmpty(command))

This comment has been minimized.

@livarcocc

livarcocc Feb 5, 2016

Can you move this help handling part into its own method?

This comment has been minimized.

@AustinWise

AustinWise Feb 5, 2016
Author

I pushed a new commit that has this all moved to a separate method. Let me know if it looks better.

{
return RunHelpCommand(appArgs);
PrintHelp();

This comment has been minimized.

@livarcocc

livarcocc Feb 5, 2016

I would move the printhelp in there as well.

@@ -153,21 +154,26 @@ private static int ProcessArgs(string[] args)
.ExitCode;
}

private static int RunHelpCommand(IEnumerable<string> appArgs)
/// <returns>true if the generic help message should be printed</returns>
private static bool ProcessHelpCommand(ref string command, ref IEnumerable<string> appArgs)

This comment has been minimized.

@livarcocc

livarcocc Feb 5, 2016

Any way we can get rid of the refs? That would be great.

@AustinWise
Copy link
Author

@AustinWise AustinWise commented Feb 5, 2016

I push a new commit wherein I moved made 'help' be a command like any other. That seems cleaner than making Cli.Program pretend a help command exists.

I assume the plan is eventually make the help command more like Git's, i.e. show a manpage on Unix and open the documentation in a browser on Windows. So while the HelpCommand class is very small now, it should be big enough to justify its existence in the future.

{
if (args.Length == 0)
{
Cli.Program.PrintHelp();

This comment has been minimized.

@livarcocc

livarcocc Feb 5, 2016

Should you simply move that method in here? Is it used anywhere else? Seems to me like it shouldn't. Does it currently depend on too much that is in the Cli.Program right now? If that is the case, this is fine by me, for now.

@AustinWise
Copy link
Author

@AustinWise AustinWise commented Feb 5, 2016

I tried moving moving more of the help functions into the HelpCommand. This way the control flow is more consistently from Program to HelpCommand, which seems to make more sense than have HelpCommand call back to Program.

@AustinWise
Copy link
Author

@AustinWise AustinWise commented Feb 5, 2016

@dotnet-bot test this please
It seems like package restore failed.

@davidfowl
Copy link

@davidfowl davidfowl commented Feb 16, 2016

This needs another rebase 😢

Also fix the help command to work with intrinsic commands.
@AustinWise
Copy link
Author

@AustinWise AustinWise commented Feb 16, 2016

@davidfowl I rebased and squashed the commits.

@davidfowl
Copy link

@davidfowl davidfowl commented Feb 16, 2016

So the command is dotnet help {command} which translates into dotnet-{command} --help should help be listed as a top level command?

davidfowl added a commit that referenced this pull request Feb 16, 2016
Fix the help command to work with intrinsic commands.
@davidfowl davidfowl merged commit 1e18150 into dotnet:rel/1.0.0 Feb 16, 2016
8 checks passed
8 checks passed
CentOS7.1 Debug Build Build finished. 136 tests run, 1 skipped, 0 failed.
Details
CentOS7.1 Release Build Build finished. 136 tests run, 1 skipped, 0 failed.
Details
OSX Debug Build Build finished. 136 tests run, 1 skipped, 0 failed.
Details
OSX Release Build Build finished. 136 tests run, 1 skipped, 0 failed.
Details
Ubuntu Debug Build Build finished. 136 tests run, 1 skipped, 0 failed.
Details
Ubuntu Release Build Build finished. 136 tests run, 1 skipped, 0 failed.
Details
Windows_NT Debug Build Build finished. 136 tests run, 0 skipped, 0 failed.
Details
Windows_NT Release Build Build finished. 136 tests run, 0 skipped, 0 failed.
Details
wli3 pushed a commit to wli3/cli that referenced this pull request Jul 14, 2017
Create profiling symbols by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.