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

login
register
mail settings
Submitter phabricator
Date May 23, 2019, 3:36 p.m.
Message ID <differential-rev-PHID-DREV-mlf3d4ipaq7bocq7b2hd-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40210/
State Superseded
Headers show

Comments

phabricator - May 23, 2019, 3:36 p.m.
ngoldbaum created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6441

AFFECTED FILES
  mercurial/help.py

CHANGE DETAILS




To: ngoldbaum, #hg-reviewers
Cc: mercurial-devel
phabricator - May 24, 2019, 1:08 p.m.
av6 added inline comments.

INLINE COMMENTS

> help.py:689-695
> +            exists = False
>              for names, header, doc in subtopics[name]:
>                  if subtopic in names:
> +                    exists = True
>                      break
> +            if not exists:
> +                raise error.UnknownCommand(name)

This made me remember that for-else statement exists.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6441

To: ngoldbaum, #hg-reviewers
Cc: av6, mercurial-devel
phabricator - May 24, 2019, 1:29 p.m.
ngoldbaum added inline comments.

INLINE COMMENTS

> av6 wrote in help.py:689-695
> This made me remember that for-else statement exists.

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6441

To: ngoldbaum, #hg-reviewers
Cc: av6, mercurial-devel
phabricator - May 25, 2019, 4:46 a.m.
martinvonz requested changes to this revision.
martinvonz added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ngoldbaum wrote in 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.

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)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6441

To: ngoldbaum, #hg-reviewers, martinvonz
Cc: martinvonz, av6, mercurial-devel
phabricator - May 25, 2019, 11:56 a.m.
ngoldbaum added inline comments.

INLINE COMMENTS

> martinvonz wrote in 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)

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6441

To: ngoldbaum, #hg-reviewers, martinvonz
Cc: martinvonz, av6, mercurial-devel
phabricator - May 25, 2019, 5:31 p.m.
pulkit added inline comments.

INLINE COMMENTS

> ngoldbaum wrote in 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.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6441

To: ngoldbaum, #hg-reviewers, martinvonz
Cc: pulkit, martinvonz, av6, mercurial-devel

Patch

diff --git a/mercurial/help.py b/mercurial/help.py
--- a/mercurial/help.py
+++ b/mercurial/help.py
@@ -686,9 +686,13 @@ 
         # Look for sub-topic entry first.
         header, doc = None, None
         if subtopic and name in subtopics:
+            exists = False
             for names, header, doc in subtopics[name]:
                 if subtopic in names:
+                    exists = True
                     break
+            if not exists:
+                raise error.UnknownCommand(name)
 
         if not header:
             for topic in helptable: