This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

Summary

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
level.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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.
doc/gendoc.py
188–214

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.

195–197

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

203–209

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):
    cmdsbycategory[helpcategory(cmd)].append(cmd)

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.

207–209

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.
doc/gendoc.py
188–214

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 localrepo.py
  • def one inside def _showcompatlist in templateutil.py (edge case, as the default arg is actually overridden once)
  • def doit inside def debugdiscovery in `debugcommands.py

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.
doc/gendoc.py
188–214

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
doc/gendoc.py
188–214

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.
doc/gendoc.py
188–214

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
doc/gendoc.py
207

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.