This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6326

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

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

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG3816e361e3d8: gendoc: group commands by category in man page and HTML help
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
Lint Skipped
Unit
Unit Tests Skipped

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–213

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–213

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–213

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–213

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–213

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.

Closed by commit rHG3816e361e3d8: gendoc: group commands by category in man page and HTML help (authored by Sietse). · Explain WhyMay 5 2019, 1:11 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mdoc/gendoc.py (28 lines)

Diff 14989

doc/gendoc.py

def commandprinter(ui, cmdtable, sectionfunc): def commandprinter(ui, cmdtable, sectionfunc):
h = {} h = {}
for c, attr in cmdtable.items(): for c, attr in cmdtable.items():
f = c.split(b"|")[0] f = c.split(b"|")[0]
f = f.lstrip(b"^") f = f.lstrip(b"^")
h[f] = c h[f] = c
cmds = h.keys() cmds = h.keys()
if True: def helpcategory(cmd, h=h, cmdtable=cmdtable):
for f in sorted(cmds): """Given a canonical command name from `cmds` (above), retrieve its
help category. If helpcategory is None, default to CATEGORY_NONE
"""
fullname = h[cmd]
details = cmdtable[fullname]
helpcategory = details[0].helpcategory
return helpcategory or help.registrar.command.CATEGORY_NONE
# Print the help for each command. We present the commands grouped by
martinvonzUnsubmitted
Done

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

martinvonz: Can be written `return helpcategory or help.registrar.command.CATEGORY_NONE`
# category, and we use help.CATEGORY_ORDER as a guide for a helpful order
# in which to present the categories.
cmdsbycategory = {category: [] for category in help.CATEGORY_ORDER}
for cmd in cmds:
cmdsbycategory[helpcategory(cmd)].append(cmd)
for category in help.CATEGORY_ORDER:
categorycmds = cmdsbycategory[category]
if not categorycmds:
continue # Skip empty categories
martinvonzUnsubmitted
Not Done

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

martinvonz: `test-check-code.t` didn't like this ("gratuitous whitespace after Python keyword"), so I moved…
# Print a section header for the category.
# For now, the category header is at the same level as the headers for
martinvonzUnsubmitted
Done

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

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

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.

martinvonz: You could probably make it linear by creating a dict outside the loop like this: ```…
# the commands in the category; this is fixed in the next commit.
ui.write(sectionfunc(help.CATEGORY_NAMES[category]))
# Print each command in the category
for f in sorted(categorycmds):
martinvonzUnsubmitted
Done

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.

martinvonz: Looks like just want `h` and `cmdtable` from the outer scope. They are available without this…
SietseAuthorUnsubmitted
Done

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: Passing `h=h, cmdtable=cmdtable` as default args is to make it obvious to the reader which data…
martinvonzUnsubmitted
Done

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.

martinvonz: Interesting examples. I found the `commithook()` quite confusing. I've sent D6336 to remove…
SietseAuthorUnsubmitted
Done

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

Sietse: Fair enough. I've removed the outer-scope variables from the function's signature, so now the…
SietseAuthorUnsubmitted
Done

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)

Sietse: I've marked your comment about capturing/passing `h` and `cmdtable` "Done" in the sense of…
if f.startswith(b"debug"): if f.startswith(b"debug"):
continue continue
d = get_cmd(h[f], cmdtable) d = get_cmd(h[f], cmdtable)
ui.write(sectionfunc(d[b'cmd'])) ui.write(sectionfunc(d[b'cmd']))
# short description # short description
ui.write(d[b'desc'][0]) ui.write(d[b'desc'][0])
# synopsis # synopsis
ui.write(b"::\n\n") ui.write(b"::\n\n")