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
Lint Skipped
Unit
Unit Tests Skipped

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 15268

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)]