This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6441

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.

Details

Reviewers
martinvonz
pulkit
Group Reviewers
hg-reviewers
Commits
rHGa84564b1a0b1: help: check if a subtopic exists and raise an error if it doesn't (issue6145)

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
Closed by commit rHGa84564b1a0b1: help: check if a subtopic exists and raise an error if it doesn't (issue6145) (authored by ngoldbaum). · Explain WhyMay 29 2019, 1:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/help.py (2 lines)

Diff 15293

mercurial/help.py

"and global options)\n") "and global options)\n")
% (name and " " + name or "")) % (name and " " + name or ""))
return rst return rst
def helptopic(name, subtopic=None): def helptopic(name, subtopic=None):
# Look for sub-topic entry first. # Look for sub-topic entry first.
header, doc = None, None header, doc = None, None
if subtopic and name in subtopics: if subtopic and name in subtopics:
for names, header, doc in subtopics[name]: for names, header, doc in subtopics[name]:
if subtopic in names: if subtopic in names:
break break
if not any(subtopic in s[0] for s in subtopics[name]):
raise error.UnknownCommand(name)
if not header: if not header:
av6Unsubmitted
Not Done

This made me remember that for-else statement exists.

av6: This made me remember that for-else statement exists.
ngoldbaumAuthorUnsubmitted
Not Done

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.

ngoldbaum: I left this as-is because I find for-else statements hard to read (I never remember what it…
martinvonzUnsubmitted
Not Done

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)
martinvonz: I avoid for-else for the same reason, but how about this: ``` if not any(subtopic in names for…
ngoldbaumAuthorUnsubmitted
Not Done

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.

ngoldbaum: OK, I agree that's clearer. I used `_` to match `header` and `doc` to make the line a bit…
pulkitUnsubmitted
Not Done

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

pulkit: This should work but we have `_` as a function imported from mercurial.i18n.
for topic in helptable: for topic in helptable:
names, header, doc = topic[0:3] names, header, doc = topic[0:3]
if name in names: if name in names:
break break
else: else:
raise error.UnknownCommand(name) raise error.UnknownCommand(name)
rst = [minirst.section(header)] rst = [minirst.section(header)]