Page MenuHomePhabricator

help: check if a subtopic exists and raise an error if it doesn't (issue6145)
ClosedPublic

Authored by ngoldbaum on May 23 2019, 11:36 AM.

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

ngoldbaum created this revision.May 23 2019, 11:36 AM
av6 added a subscriber: av6.May 24 2019, 9:08 AM
av6 added inline comments.
mercurial/help.py
689–695

This made me remember that for-else statement exists.

ngoldbaum added inline comments.May 24 2019, 9:29 AM
mercurial/help.py
689–695

I left this as-is because I find for-else statements hard to read (I never remember what it means!) and find this to be clearer even if there's a bit more boilerplate.

martinvonz requested changes to this revision.May 25 2019, 12:46 AM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.
mercurial/help.py
689–695

I avoid for-else for the same reason, but how about this:

if not any(subtopic in names for names, header, doc in subtopics[name]):
    raise error.UnknownCommand(name)
This revision now requires changes to proceed.May 25 2019, 12:46 AM
ngoldbaum updated this revision to Diff 15258.May 25 2019, 7:54 AM
ngoldbaum added inline comments.May 25 2019, 7:56 AM
mercurial/help.py
689–695

OK, I agree that's clearer. I used _ to match header and doc to make the line a bit shorter and to make it a bit clearer for me to read since those aren't being used.

pulkit added a subscriber: pulkit.May 25 2019, 1:31 PM
pulkit added inline comments.
mercurial/help.py
689–695

This should work but we have _ as a function imported from mercurial.i18n.

ngoldbaum updated this revision to Diff 15268.May 26 2019, 5:06 PM
pulkit accepted this revision.May 29 2019, 1:24 PM
This revision was automatically updated to reflect the committed changes.