Page MenuHomePhabricator

gendoc: guarantee that all commands were processed
ClosedPublic

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

Details

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
This revision was automatically updated to reflect the committed changes.