[PATCH] Bug 3749 --help does not show non-command help topics

[Mads Kiilerich]

[Resending with proper formatting ... mehope]

Hi

Nice patch.

Anyway, here are some comments that perhaps can make it even better.

On 04/17/2013 11:47 AM, Ankur Ankan wrote:
> # HG changeset patch
> # User Ankur Ankan <ankurankan at gmail.com <mailto:ankurankan at gmail.com>>
> # Date 1366191853 -19800
> #      Wed Apr 17 15:14:13 2013 +0530
> # Node ID 3c08507d8eff9c67d376e7283e096e64e794197b
> # Parent  a59e575c6ff87b517a8bb167c509a93003bfef53
> help: show topic help with --help (issue3749)
>

I would like to see a bit more description here. ( 
http://mercurial.selenic.com/wiki/ContributingChanges#Patch_descriptions )

First of all I would like to see an example of what command line it is 
that will be possible. It is not obvious from the summary line.

I assume that one thing you learned while creating this patch is that it 
is a bit tricky because help topics not are commands and options like 
--help can't take optional parameters. A reviewer might be wondering why 
this can't be implemented the same way as ordinary --help is. Briefly 
explaining the tricky part makes it easier to review ... and to 
understand the change later on.

Have you considered the alternative approach of adding help topics to 
the command list? It with perhaps be interesting with a brief note 
telling why you decided not do that.

> diff -r a59e575c6ff8 -r 3c08507d8eff mercurial/dispatch.py
> --- a/mercurial/dispatch.pyTue Apr 16 15:33:18 2013 +0200
> +++ b/mercurial/dispatch.pyWed Apr 17 15:14:13 2013 +0530
> @@ -635,7 +635,18 @@
>          encoding.fallbackencoding = fallback
>      fullargs = args
> -    cmd, func, args, options, cmdoptions = _parse(lui, args)
> +    try:
> +        cmd, func, args, options, cmdoptions = _parse(lui, args)
> +    except error.UnknownCommand:
> +        if '--help' in fullargs[:-1]:
> +            commands.help_(ui, fullargs[-1])

These two lines seems to assume that the options are given exactly as 
'--help topicname'.

Shouldn't 'hg topicname --help' be supported too? The tests indicate 
that it intentionally shouldn't. I would expect a brief explanation of 
that in the description.

And how about 'hg -v --help urls --debug' ?

> +            return
> +        else:

else is not necessary after an unconditional return. It could be argued 
that being explicit is good ... but usually we like briefness and no 
redundancy even more.

> +            for args in fullargs:
> +                if args[0] != '-':
> +                    cmd = args
> +                    break
> +            raise error.UnknownCommand(cmd)

This is all just to raise the exception again if there was no --help? 
Then just use 'raise'.

>      if options["config"]:
>          raise util.Abort(_("option --config may not be abbreviated!"))
> diff -r a59e575c6ff8 -r 3c08507d8eff tests/test-help.t
> --- a/tests/test-help.tTue Apr 16 15:33:18 2013 +0200
> +++ b/tests/test-help.tWed Apr 17 15:14:13 2013 +0530
> @@ -741,6 +741,419 @@
>        The reserved name "." indicates the working directory parent. If no
>        working directory is checked out, it is equivalent to null. If an
>        uncommitted merge is in progress, "." is the revision of the 
> first parent.
> +Test for topic help with --help

I suggest adding an empty line before the description of the new test - 
that makes the test file more readable.

> +
> +  $ hg --help revset

(revset is one of the bigger help topics. Using a short one like 
'multirevs' makes the test less verbose and more readable.)

> +Test for Unknown Command when hg revset --help is used
> +
> +  $ hg revset --help
> +  hg: unknown command 'revset'
> +  Mercurial Distributed SCM

It might also be an idea to test some other 'evil' command lines such as 
the example I gave above - no matter if you intentionally do support 
them or not.

'hg --help foo' might also be interesting to test ... unless we already 
have test coverage for that.

/Mads
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20130417/a8aa4d2b/attachment.html>