This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6327

gendoc: guarantee that all commands were processed
ClosedPublic

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

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG037a97d62625: gendoc: guarantee that all commands were processed
Summary

The new logic renders the commands belonging to each category in turn.
Commands with an unregistered category are at risk of getting skipped
because their category is not in the list. By comparing the list of all
commands to a log of processed commands, we can detect commands with
unregistered categories and fail with an error message.

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
Sietse retitled this revision from gendoc: guarantee that all commands were processed. to gendoc: guarantee that all commands were processed.Apr 29 2019, 7:08 AM
martinvonz requested changes to this revision.May 1 2019, 3:01 PM
martinvonz added a subscriber: martinvonz.

I think this patch will be simpler if you create the dict I suggested on D6326. You could then put the check for debug commands in that initial loop. You could also put the check just after that loop (any categories found that are not in help.CATEGORY_ORDER would be unregistered).

This revision now requires changes to proceed.May 1 2019, 3:01 PM
Sietse retitled this revision from gendoc: guarantee that all commands were processed to gendoc: guarantee that all commands were processed..May 3 2019, 9:36 AM
Sietse retitled this revision from gendoc: guarantee that all commands were processed. to gendoc: guarantee that all commands were processed.
Sietse updated this revision to Diff 14990.
martinvonz added inline comments.May 3 2019, 12:06 PM
doc/gendoc.py
198–207

No need to use exceptions here (KeyError could potentially be raised by something we didn't expect and then we'd misdiagnose it here). I think it's clearer like this:

category = helpcategory(cmd)
if category in cmdsbycategory:
    raise ...
cmdsbycategory[category].append(cmd)
200

I think a previous version of this patch ignored uncategorized debug commands. Will this version error out?

Sietse updated this revision to Diff 14999.May 4 2019, 10:36 AM
Sietse marked 2 inline comments as done.May 4 2019, 12:02 PM
Sietse added inline comments.
doc/gendoc.py
200

It doesn't error out, I checked before uploading. Debug commands have CATEGORY_NONE; see also below.

The purpose of this patch is to give confidence that no commands got 'lost' as we moved from 'process every command' (which guarantees every commands gets processed) to 'process every registered category' (where commands can get skipped if their category isn't registered).

The previous version of this patch compared an expected list of commands to a log of processed commands. Because the main command-rendering loop skipped debug commands, I manually removed them from the expected list, too. That's why it ignored debug commands.

The current version of this patch checks at command-sorting time that every command is part of a registered category. Commands with no category (such as debug commands) get sorted into CATEGORY_NONE, which is a known category displayed as "Uncategorized commands". We've guaranteed that the command-rendering loop will see every command, and what it skips or keeps is up to the rendering logic.

martinvonz added inline comments.May 4 2019, 12:31 PM
doc/gendoc.py
200

The previous version of this patch compared an expected list of commands to a log of processed commands. Because the main command-rendering loop skipped debug commands, I manually removed them from the expected list, too. That's why it ignored debug commands.

Ah, that makes sense.

Sietse marked 3 inline comments as done.May 4 2019, 3:58 PM
Closed by commit rHG037a97d62625: gendoc: guarantee that all commands were processed (authored by Sietse). · Explain WhyMay 5 2019, 1:12 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mdoc/gendoc.py (15 lines)

Diff 15013

doc/gendoc.py

"""Given a canonical command name from `cmds` (above), retrieve its """Given a canonical command name from `cmds` (above), retrieve its
help category. If helpcategory is None, default to CATEGORY_NONE. help category. If helpcategory is None, default to CATEGORY_NONE.
""" """
fullname = h[cmd] fullname = h[cmd]
details = cmdtable[fullname] details = cmdtable[fullname]
helpcategory = details[0].helpcategory helpcategory = details[0].helpcategory
return helpcategory or help.registrar.command.CATEGORY_NONE return helpcategory or help.registrar.command.CATEGORY_NONE
# Print the help for each command. We present the commands grouped by
# 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} cmdsbycategory = {category: [] for category in help.CATEGORY_ORDER}
for cmd in cmds: for cmd in cmds:
# If a command category wasn't registered, the command won't get
# rendered below, so we raise an AssertionError.
martinvonzUnsubmitted
Done

I think a previous version of this patch ignored uncategorized debug commands. Will this version error out?

martinvonz: I think a previous version of this patch ignored uncategorized debug commands. Will this…
SietseAuthorUnsubmitted
Done

It doesn't error out, I checked before uploading. Debug commands have CATEGORY_NONE; see also below.

The purpose of this patch is to give confidence that no commands got 'lost' as we moved from 'process every command' (which guarantees every commands gets processed) to 'process every registered category' (where commands can get skipped if their category isn't registered).

The previous version of this patch compared an expected list of commands to a log of processed commands. Because the main command-rendering loop skipped debug commands, I manually removed them from the expected list, too. That's why it ignored debug commands.

The current version of this patch checks at command-sorting time that every command is part of a registered category. Commands with no category (such as debug commands) get sorted into CATEGORY_NONE, which is a known category displayed as "Uncategorized commands". We've guaranteed that the command-rendering loop will see every command, and what it skips or keeps is up to the rendering logic.

Sietse: It doesn't error out, I checked before uploading. Debug commands have CATEGORY_NONE; see also…
martinvonzUnsubmitted
Done

The previous version of this patch compared an expected list of commands to a log of processed commands. Because the main command-rendering loop skipped debug commands, I manually removed them from the expected list, too. That's why it ignored debug commands.

Ah, that makes sense.

martinvonz: > The previous version of this patch compared an expected list of commands to a log of…
if helpcategory(cmd) not in cmdsbycategory:
raise AssertionError(
"The following command did not register its (category) in "
"help.CATEGORY_ORDER: %s (%s)" % (cmd, helpcategory(cmd)))
cmdsbycategory[helpcategory(cmd)].append(cmd) cmdsbycategory[helpcategory(cmd)].append(cmd)
# Print the help for each command. We present the commands grouped by
martinvonzUnsubmitted
Done

No need to use exceptions here (KeyError could potentially be raised by something we didn't expect and then we'd misdiagnose it here). I think it's clearer like this:

category = helpcategory(cmd)
if category in cmdsbycategory:
    raise ...
cmdsbycategory[category].append(cmd)
martinvonz: No need to use exceptions here (`KeyError` could potentially be raised by something we didn't…
# category, and we use help.CATEGORY_ORDER as a guide for a helpful order
# in which to present the categories.
for category in help.CATEGORY_ORDER: for category in help.CATEGORY_ORDER:
categorycmds = cmdsbycategory[category] categorycmds = cmdsbycategory[category]
if not categorycmds: if not categorycmds:
# Skip empty categories # Skip empty categories
continue continue
# Print a section header for the category. # Print a section header for the category.
# For now, the category header is at the same level as the headers for # For now, the category header is at the same level as the headers for
# the commands in the category; this is fixed in the next commit. # the commands in the category; this is fixed in the next commit.
if multioccur: if multioccur:
ui.write(_(b"\n[+] marked option can be specified" ui.write(_(b"\n[+] marked option can be specified"
b" multiple times\n")) b" multiple times\n"))
ui.write(b"\n") ui.write(b"\n")
# aliases # aliases
if d[b'aliases']: if d[b'aliases']:
ui.write(_(b" aliases: %s\n\n") % b" ".join(d[b'aliases'])) ui.write(_(b" aliases: %s\n\n") % b" ".join(d[b'aliases']))
def allextensionnames(): def allextensionnames():
return set(extensions.enabled().keys()) | set(extensions.disabled().keys()) return set(extensions.enabled().keys()) | set(extensions.disabled().keys())
if __name__ == "__main__": if __name__ == "__main__":
doc = b'hg.1.gendoc' doc = b'hg.1.gendoc'
if len(sys.argv) > 1: if len(sys.argv) > 1:
doc = encoding.strtolocal(sys.argv[1]) doc = encoding.strtolocal(sys.argv[1])
ui = uimod.ui.load() ui = uimod.ui.load()
if doc == b'hg.1.gendoc': if doc == b'hg.1.gendoc':
showdoc(ui) showdoc(ui)
else: else:
showtopic(ui, encoding.strtolocal(sys.argv[1])) showtopic(ui, encoding.strtolocal(sys.argv[1]))