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

[Mads Kiilerich]

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>
# 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.py Tue Apr 16 15:33:18 2013 +0200
+++ b/mercurial/dispatch.py Wed 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.t Tue Apr 16 15:33:18 2013 +0200
+++ b/tests/test-help.t Wed 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/cddff87b/attachment.html>