Page MenuHomePhabricator

gendoc: group commands by category in man page and HTML help

Authored by Sietse on Apr 28 2019, 5:17 PM.



Make Mercurial's man page and HTML help group commands by category, and
present the categories in a helpful order. hg help already does this;
this patch uses the same metadata.

This patch uses the same header level for command categories and for
commands. A subsequent patch will push the command headers down one

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sietse created this revision.Apr 28 2019, 5:17 PM
martinvonz requested changes to this revision.May 1 2019, 2:51 PM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.

Looks like just want h and cmdtable from the outer scope. They are available without this trick, so you can just delete the two arguments here.


Can be written return helpcategory or help.registrar.command.CATEGORY_NONE


You could probably make it linear by creating a dict outside the loop like this:

cmdsbycategory = {category: [] for category in help.CATEGORY_ORDER}
for cmd in sorted(cmds):

That's barely longer, and it removes the need for the comment. I also think you can inline the helpcategory() function if you do that.


Doesn't look like you need to sort here since it's going to be sorted on line 216 anyway.

This revision now requires changes to proceed.May 1 2019, 2:51 PM
Sietse edited the summary of this revision. (Show Details)May 3 2019, 9:36 AM
Sietse updated this revision to Diff 14989.
Sietse marked 3 inline comments as done.May 3 2019, 9:48 AM
Sietse added inline comments.

Passing h=h, cmdtable=cmdtable as default args is to make it obvious to the reader which data the function depends on. I realize it can also silently close over the variables in the outer scope, but I think this helps legibility.

Other places in the Mercurial codebase that define an inner function, and make a captured value obvious by passing it as a default argument:

  • def commithook inside def commit in
  • def one inside def _showcompatlist in (edge case, as the default arg is actually overridden once)
  • def doit inside def debugdiscovery in `

I'd say leave this as is, but I'll let you make the call and follow that.

Sietse marked 2 inline comments as done.May 3 2019, 9:53 AM
Sietse added inline comments.

I've marked your comment about capturing/passing h and cmdtable "Done" in the sense of "replied to". I'm not sure how Phabricator's flow works -- "Done" seemed most likely to keep the flow going, but I do remain open to any reply by you (which I will then not bikeshed, because this is ultimately trivial :-P)

martinvonz added inline comments.May 3 2019, 12:06 PM

Interesting examples.

I found the commithook() quite confusing. I've sent D6336 to remove the arguments from it.

As you said, the def oneargument is overridden in one place, so I don't think that's a good comparison.

The def doit actually uses the remote=remote as a trick for creating the variable in local scope.

Even if there are a few more valid cases, the vast majority seems to be just accessing the variables from the closure.

Sietse updated this revision to Diff 14998.May 4 2019, 10:36 AM
Sietse marked 2 inline comments as done.May 4 2019, 11:29 AM
Sietse added inline comments.

Fair enough. I've removed the outer-scope variables from the function's signature, so now the function closes over them instead.

Sietse marked 2 inline comments as done.May 4 2019, 3:57 PM
martinvonz added inline comments.May 5 2019, 1:10 AM

test-check-code.t didn't like this ("gratuitous whitespace after Python keyword"), so I moved the comment to separate line above.

This revision was automatically updated to reflect the committed changes.