Patchwork D7384: commands: necessary annotations and suppresssions to pass pytype

login
register
mail settings
Submitter phabricator
Date Nov. 14, 2019, 3:53 a.m.
Message ID <differential-rev-PHID-DREV-jf6lvye7af4ry2dwit7q-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43182/
State New
Headers show

Comments

phabricator - Nov. 14, 2019, 3:53 a.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As with other places, there are some places where our types are just
  too complicated for pytype, so we put some suppressions in place.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/commands.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 14, 2019, 9:45 a.m.
dlax added inline comments.

INLINE COMMENTS

> commands.py:1127
> +            bgood,  # pytype: disable=name-error
> +        )
>          return

This one is sad. I think this can be sorted out by replacing the `try:/finally:` with a context manager. (Can send a patch, if it sounds good to you.)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Nov. 14, 2019, 9:15 p.m.
durin42 added inline comments.

INLINE COMMENTS

> dlax wrote in commands.py:1127
> This one is sad. I think this can be sorted out by replacing the `try:/finally:` with a context manager. (Can send a patch, if it sounds good to you.)

By all means!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Nov. 15, 2019, 11:08 a.m.
This revision now requires changes to proceed.
dlax added a comment.
dlax requested changes to this revision.


  D7430 <https://phab.mercurial-scm.org/D7430> makes this changes unnecessary I think.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
phabricator - Nov. 15, 2019, 5:24 p.m.
durin42 added a comment.


  I still want to keep the annotations I added. :)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
phabricator - Nov. 15, 2019, 5:40 p.m.
dlax added inline comments.

INLINE COMMENTS

> commands.py:4742
> +            # warning about list not having a max() method.
> +            endrev = revs.max() + 1  # pytype: disable=attribute-error
>          getcopies = scmutil.getcopiesfn(repo, endrev=endrev)

`revs` is always a `smartset.baseset` per af9c73f26371 <https://phab.mercurial-scm.org/rHGaf9c73f263713a7d379c629006be6701dd9956c2> so there should be no attribute error. Or is it because `logcmdutil.getlinerangerevs()` has no type annotation (whereas `logcmdutil.getrevs()` has some)?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
phabricator - Jan. 30, 2020, 6:32 p.m.
marmoute added a comment.


  What's up on this ? It seemed on a good track, but I don't think it landed. @dlax I think you offer to use a context manager got a warm welcome, I would says, go ahead with it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers, dlax
Cc: marmoute, dlax, mercurial-devel
phabricator - Jan. 30, 2020, 7:26 p.m.
dlax added a comment.


  In D7384#118739 <https://phab.mercurial-scm.org/D7384#118739>, @marmoute wrote:
  
  > What's up on this ? It seemed on a good track, but I don't think it landed. @dlax I think you offer to use a context manager got a warm welcome, I would says, go ahead with it.
  
  The context manager got in through D7430 <https://phab.mercurial-scm.org/D7430>.

INLINE COMMENTS

> commands.py:4741
> +            # pytype isn't convinced, so we have to suppress the
> +            # warning about list not having a max() method.
>              endrev = revs.max() + 1

I think this comment should be removed since the `# pytype: disable` got removed in the last version of the diff.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers, dlax
Cc: marmoute, dlax, mercurial-devel
phabricator - Feb. 14, 2020, 8:21 a.m.
marmoute added a comment.
marmoute accepted this revision.


  There don't seems to be any remaininf feedback on this.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7384/new/

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

To: durin42, #hg-reviewers, dlax, marmoute
Cc: marmoute, dlax, mercurial-devel

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -72,6 +72,15 @@ 
     stringutil,
 )
 
+if not globals():
+    from typing import (
+        Any,
+        Dict,
+    )
+
+    for t in (Any, Dict):
+        assert t
+
 table = {}
 table.update(debugcommandsmod.command._table)
 
@@ -1108,7 +1117,14 @@ 
         finally:
             state[b'current'] = [node]
             hbisect.save_state(repo, state)
-        hbisect.printresult(ui, repo, state, displayer, nodes, bgood)
+        hbisect.printresult(
+            ui,
+            repo,
+            state,
+            displayer,
+            nodes,
+            bgood,  # pytype: disable=name-error
+        )
         return
 
     hbisect.checkstate(state)
@@ -2970,7 +2986,7 @@ 
     if opts.get(b'base'):
         basectx = scmutil.revsingle(repo, opts[b'base'], None)
     # a dict of data to be stored in state file
-    statedata = {}
+    statedata = {}  # type: Dict[bytes, Any]
     # list of new nodes created by ongoing graft
     statedata[b'newnodes'] = []
 
@@ -3240,7 +3256,8 @@ 
                 )
             # checking that newnodes exist because old state files won't have it
             elif statedata.get(b'newnodes') is not None:
-                statedata[b'newnodes'].append(node)
+                l = statedata[b'newnodes']
+                l.append(node)  # pytype: disable=attribute-error
 
     # remove state when we complete successfully
     if not opts.get(b'dry_run'):
@@ -4731,7 +4748,11 @@ 
     if copies:
         endrev = None
         if revs:
-            endrev = revs.max() + 1
+            # If we're here, we know revs is a smartset.baseset
+            # because we're not possibly in the linerange mode. Sadly,
+            # pytype isn't convinced, so we have to suppress the
+            # warning about list not having a max() method.
+            endrev = revs.max() + 1  # pytype: disable=attribute-error
         getcopies = scmutil.getcopiesfn(repo, endrev=endrev)
 
     ui.pager(b'log')
@@ -7177,11 +7198,13 @@ 
         t = []
         if incoming:
             t.append(_(b'1 or more incoming'))
-        o = outgoing.missing
+        o = outgoing.missing  # pytype: disable=attribute-error
         if o:
             t.append(_(b'%d outgoing') % len(o))
         other = dother or sother
-        if b'bookmarks' in other.listkeys(b'namespaces'):
+        if b'bookmarks' in other.listkeys(  # pytype: disable=attribute-error
+            b'namespaces'
+        ):
             counts = bookmarks.summary(repo, other)
             if counts[0] > 0:
                 t.append(_(b'%d incoming bookmarks') % counts[0])