[PATCH b4 0/5] Resolve some static typing errors

[PATCH b4 0/5] Resolve some static typing errors

[Tamir Duberstein]

This series slightly improves the quality of type annotations in the
project. It also adds development dependencies so they can more easily
be installed.

Unfortunately this is only a drop in the bucket; there are still 356
errors as reported by pyright. Without minimal tooling to ensure these
don't gress, progress will be difficult.

I'd be happy to contribute such tooling, but I didn't find where to put
it - there's no obvious place from which tests are run, for example.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Tamir Duberstein (5):
      Add development dependencies
      Avoid file descriptor leak
      Correctly type annotate generators
      Provide overloads for git_run_command
      Add missing imports

 pyproject.toml             |  8 ++++++-
 src/b4/__init__.py         | 52 ++++++++++++++++++++++++++++++----------------
 src/b4/diff.py             |  7 ++++---
 src/b4/ez.py               |  2 ++
 src/b4/pr.py               |  2 ++
 src/b4/ty.py               |  1 +
 src/tests/test___init__.py |  1 +
 7 files changed, 51 insertions(+), 22 deletions(-)
---
base-commit: 2a6338e451a0c1e81f214f48c820c1e52d76b2f1
change-id: 20241025-better-type-annotations-e732fbdb2ff5

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH b4 1/5] Add development dependencies

[Tamir Duberstein]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 pyproject.toml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/pyproject.toml b/pyproject.toml
index 31bfe3033650b2b251fcdcbab0456f4a67e914df..7c65c30efdb95fa40cc1f22933bcd62073db4f26 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -22,7 +22,13 @@ classifiers = [
     "Topic :: Software Development",
     "Topic :: Utilities",
 ]
-dynamic = ["dependencies", "optional-dependencies"]
+dynamic = ["dependencies"]
+
+[project.optional-dependencies]
+dev = [
+    "pytest",
+    "types-requests",
+]
 
 [tool.setuptools.dynamic]
 dependencies = { file = ["requirements.in"] }

-- 
2.47.0

[PATCH b4 2/5] Avoid file descriptor leak

[Tamir Duberstein]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 src/b4/diff.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/b4/diff.py b/src/b4/diff.py
index 63e60f2fc82c95145a897c2dd319283996ed5a8a..743127ef0a1922eace81686e27cd0366f8e42f9c 100644
--- a/src/b4/diff.py
+++ b/src/b4/diff.py
@@ -174,8 +174,8 @@ def main(cmdargs: argparse.Namespace) -> None:
         sys.exit(1)
     if cmdargs.outdiff is not None:
         logger.info('Writing %s', cmdargs.outdiff)
-        fh = open(cmdargs.outdiff, 'w')
+        with open(cmdargs.outdiff, 'w') as fh:
+            fh.write(rdiff)
     else:
         logger.info('---')
-        fh = sys.stdout
-    fh.write(rdiff)
+        sys.stdout.write(rdiff)

-- 
2.47.0

[PATCH b4 3/5] Correctly type annotate generators

[Tamir Duberstein]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 src/b4/__init__.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/b4/__init__.py b/src/b4/__init__.py
index 5115a9cd9b8746f84eb104a2be69fe121724167e..d1f0fc5b56693d7d234bc4aced65166244928728 100644
--- a/src/b4/__init__.py
+++ b/src/b4/__init__.py
@@ -36,7 +36,7 @@ import requests
 
 from pathlib import Path
 from contextlib import contextmanager
-from typing import Optional, Tuple, Set, List, BinaryIO, Union, Sequence, Literal, Iterator, Dict
+from typing import Optional, Tuple, Set, List, BinaryIO, Union, Sequence, Literal, Iterator, Dict, overload
 
 from email import charset
 
@@ -2758,7 +2758,7 @@ def git_get_repo_status(gitdir: Optional[str] = None, untracked: bool = False) -
 
 
 @contextmanager
-def git_temp_worktree(gitdir: Optional[str] = None, commitish: Optional[str] = None) -> Optional[Iterator[Path]]:
+def git_temp_worktree(gitdir: Optional[str] = None, commitish: Optional[str] = None) -> Generator[str]:
     """Context manager that creates a temporary work tree and chdirs into it. The
     worktree is deleted when the contex manager is closed. Taken from gj_tools."""
     dfn = None
@@ -2776,7 +2776,7 @@ def git_temp_worktree(gitdir: Optional[str] = None, commitish: Optional[str] = N
 
 
 @contextmanager
-def git_temp_clone(gitdir: Optional[str] = None) -> Optional[Iterator[Path]]:
+def git_temp_clone(gitdir: Optional[str] = None) -> Generator[str]:
     """Context manager that creates a temporary shared clone."""
     if gitdir is None:
         topdir = git_get_toplevel()
@@ -2794,7 +2794,7 @@ def git_temp_clone(gitdir: Optional[str] = None) -> Optional[Iterator[Path]]:
 
 
 @contextmanager
-def in_directory(dirname: str) -> Iterator[bool]:
+def in_directory(dirname: str) -> Generator[bool]:
     """Context manager that chdirs into a directory and restores the original
     directory when closed. Taken from gj_tools."""
     cdir = os.getcwd()

-- 
2.47.0

[PATCH b4 4/5] Provide overloads for git_run_command

[Tamir Duberstein]

This requires explicit use of kwargs to allow decode to be a required
parameter in the negative case (which is necessary to disambiguate the
overloads).

Callers now know for certain whether they get str or bytes.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 src/b4/__init__.py | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/b4/__init__.py b/src/b4/__init__.py
index d1f0fc5b56693d7d234bc4aced65166244928728..e71fbe7d8ea8d4ca19228690ce7c186cc91b81ad 100644
--- a/src/b4/__init__.py
+++ b/src/b4/__init__.py
@@ -36,7 +36,7 @@ import requests
 
 from pathlib import Path
 from contextlib import contextmanager
-from typing import Optional, Tuple, Set, List, BinaryIO, Union, Sequence, Literal, Iterator, Dict, overload
+from typing import Optional, Tuple, Set, List, BinaryIO, Union, Sequence, Literal, Iterator, Dict, TypeVar, overload
 
 from email import charset
 
@@ -2697,14 +2697,27 @@ def gpg_run_command(args: List[str], stdin: Optional[bytes] = None) -> Tuple[int
     return _run_command(cmdargs, stdin=stdin)
 
 
+@overload
+def git_run_command(gitdir: Optional[Union[str, Path]], args: List[str], stdin: Optional[bytes] = ...,
+                    *, logstderr: bool = ..., decode: Literal[False],
+                    rundir: Optional[str] = ...) -> Tuple[int, bytes]:
+    ...
+
+
+@overload
+def git_run_command(gitdir: Optional[Union[str, Path]], args: List[str], stdin: Optional[bytes] = ...,
+                    *, logstderr: bool = ..., decode: Literal[True] = ...,
+                    rundir: Optional[str] = ...) -> Tuple[int, str]:
+    ...
+
 def git_run_command(gitdir: Optional[Union[str, Path]], args: List[str], stdin: Optional[bytes] = None,
-                    logstderr: bool = False, decode: bool = True,
+                    *, logstderr: bool = False, decode: bool = True,
                     rundir: Optional[str] = None) -> Tuple[int, Union[str, bytes]]:
     cmdargs = ['git', '--no-pager']
     if gitdir:
         if os.path.exists(os.path.join(gitdir, '.git')):
             gitdir = os.path.join(gitdir, '.git')
-        cmdargs += ['--git-dir', gitdir]
+        cmdargs += ['--git-dir', str(gitdir)]
 
     # counteract some potential local settings
     if args[0] == 'log':
@@ -2714,16 +2727,18 @@ def git_run_command(gitdir: Optional[Union[str, Path]], args: List[str], stdin:
 
     ecode, out, err = _run_command(cmdargs, stdin=stdin, rundir=rundir)
 
-    if decode:
-        out = out.decode(errors='replace')
+    U = TypeVar('U', str, bytes)
+
+    def _handle(out: U, err: U) -> Tuple[int, Union[str, bytes]]:
+        if logstderr and len(err.strip()):
+            logger.debug('Stderr: %s', err)
+            out += err
 
-    if logstderr and len(err.strip()):
-        if decode:
-            err = err.decode(errors='replace')
-        logger.debug('Stderr: %s', err)
-        out += err
+        return ecode, out
 
-    return ecode, out
+    if decode:
+        return _handle(out.decode(errors='replace'), err.decode(errors='replace'))
+    return _handle(out, err)
 
 
 def git_credential_fill(gitdir: Optional[str], protocol: str, host: str, username: str) -> Optional[str]:

-- 
2.47.0

[PATCH b4 5/5] Add missing imports

[Tamir Duberstein]

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 src/b4/__init__.py         | 9 +++++----
 src/b4/diff.py             | 1 +
 src/b4/ez.py               | 2 ++
 src/b4/pr.py               | 2 ++
 src/b4/ty.py               | 1 +
 src/tests/test___init__.py | 1 +
 6 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/b4/__init__.py b/src/b4/__init__.py
index e71fbe7d8ea8d4ca19228690ce7c186cc91b81ad..7b1925da93e1b3feadf669cc188ab6fb030f67f4 100644
--- a/src/b4/__init__.py
+++ b/src/b4/__init__.py
@@ -8,12 +8,13 @@ import sys
 import gzip
 import os
 import fnmatch
-import email.utils
-import email.policy
-import email.parser
-import email.header
 import email.generator
+import email.header
+import email.message
+import email.parser
+import email.policy
 import email.quoprimime
+import email.utils
 import tempfile
 import pathlib
 import argparse
diff --git a/src/b4/diff.py b/src/b4/diff.py
index 743127ef0a1922eace81686e27cd0366f8e42f9c..1de3063ea0b3c73e4e3d8bfd4ec91b4c6f90cce6 100644
--- a/src/b4/diff.py
+++ b/src/b4/diff.py
@@ -11,6 +11,7 @@ import b4
 import b4.mbox
 import mailbox
 import email
+import email.message
 import email.parser
 import shutil
 import pathlib
diff --git a/src/b4/ez.py b/src/b4/ez.py
index 970631592d2139b12ee4a6a9a112081eee1de721..9494b005a4f47a7be9cb053cc6075dede29d4cb0 100644
--- a/src/b4/ez.py
+++ b/src/b4/ez.py
@@ -17,6 +17,8 @@ import datetime
 import json
 import shlex
 import email
+import email.policy
+import email.utils
 import pathlib
 import base64
 import textwrap
diff --git a/src/b4/pr.py b/src/b4/pr.py
index b6eb5d3933d7e0bb1224877f071bb19facd8dbd8..60f46f1e3fa96a52a9d667bffb43c2a75b89cc4e 100644
--- a/src/b4/pr.py
+++ b/src/b4/pr.py
@@ -13,7 +13,9 @@ import b4
 import re
 import json
 import email
+import email.message
 import email.parser
+import email.utils
 import argparse
 
 import urllib.parse
diff --git a/src/b4/ty.py b/src/b4/ty.py
index e1afb61280d2161a054d40ae78df0d4b33dbb41d..021588ff9e559cfd8aa7c6818cdeba750931e8f6 100644
--- a/src/b4/ty.py
+++ b/src/b4/ty.py
@@ -13,6 +13,7 @@ import re
 import email
 import email.message
 import email.policy
+import email.utils
 import json
 import argparse
 
diff --git a/src/tests/test___init__.py b/src/tests/test___init__.py
index 297e5120a8c8adf241660f06e9a83c1ff4a0fd3a..3e01c2f5e127cd1123d0fb8503493e396148ee73 100644
--- a/src/tests/test___init__.py
+++ b/src/tests/test___init__.py
@@ -35,6 +35,7 @@ def test_save_git_am_mbox(sampledir, tmp_path, source, regex, flags, ismbox):
             msgs = list(mbx)
         else:
             import email
+            import email.parser
             with open(f'{sampledir}/{source}.txt', 'rb') as fh:
                 msg = email.parser.BytesParser(policy=b4.emlpolicy, _class=email.message.EmailMessage).parse(fh)
             msgs = [msg]

-- 
2.47.0

Re: [PATCH b4 0/5] Resolve some static typing errors

[Konstantin Ryabitsev]

On Fri, 25 Oct 2024 16:16:02 -0400, Tamir Duberstein wrote:
> This series slightly improves the quality of type annotations in the
> project. It also adds development dependencies so they can more easily
> be installed.
> 
> Unfortunately this is only a drop in the bucket; there are still 356
> errors as reported by pyright. Without minimal tooling to ensure these
> don't gress, progress will be difficult.
> 
> [...]

Applied, thanks!

[1/5] Add development dependencies
      commit: 66fa5e85a5768f24687f806413a6ff3774d43f8c
[2/5] Avoid file descriptor leak
      commit: d99337f1aa6ec60398539d772c5eace25d665ac8
[3/5] Correctly type annotate generators
      commit: 2801becd386bf9b3c8947421f3ef20442def2a58
[4/5] Provide overloads for git_run_command
      commit: 846254faae45c20a1c0721d8c139c0b5d26a9bf2
[5/5] Add missing imports
      commit: 21904f395d068f0b957972d148fd5d326a904920

Best regards,
-- 
Konstantin Ryabitsev <konstantin@linuxfoundation.org>

Re: [PATCH b4 0/5] Resolve some static typing errors

[Konstantin Ryabitsev]

On Wed, Jan 22, 2025 at 10:28:45AM -0500, Konstantin Ryabitsev wrote:
> > This series slightly improves the quality of type annotations in the
> > project. It also adds development dependencies so they can more easily
> > be installed.
> > 
> > Unfortunately this is only a drop in the bucket; there are still 356
> > errors as reported by pyright. Without minimal tooling to ensure these
> > don't gress, progress will be difficult.
> > 
> > [...]
> 
> Applied, thanks!

Note, that I had to do a follow-up fix for the missing Generator type import.
I should have caught that during review, but missed it.

-K

Re: [PATCH b4 0/5] Resolve some static typing errors

[Tamir Duberstein]

On Wed, Jan 22, 2025 at 5:36 PM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Wed, Jan 22, 2025 at 10:28:45AM -0500, Konstantin Ryabitsev wrote:
> > > This series slightly improves the quality of type annotations in the
> > > project. It also adds development dependencies so they can more easily
> > > be installed.
> > >
> > > Unfortunately this is only a drop in the bucket; there are still 356
> > > errors as reported by pyright. Without minimal tooling to ensure these
> > > don't gress, progress will be difficult.
> > >
> > > [...]
> >
> > Applied, thanks!
>
> Note, that I had to do a follow-up fix for the missing Generator type import.
> I should have caught that during review, but missed it.
>
> -K

Cheers, thanks for that.

Would you be interested in a Github Actions config that runs the
static analyzer? As I mentioned in the cover letter I think we'll want
a regression to stop so we can make steady progress. If you're open to
it, I'll work it up.

Thanks again for creating b4!

Tamir

Re: [PATCH b4 0/5] Resolve some static typing errors

[Konstantin Ryabitsev]

On Thu, Jan 23, 2025 at 10:28:21PM +0200, Tamir Duberstein wrote:
> > Note, that I had to do a follow-up fix for the missing Generator type import.
> > I should have caught that during review, but missed it.
> >
> > -K
> 
> Cheers, thanks for that.
> 
> Would you be interested in a Github Actions config that runs the
> static analyzer? As I mentioned in the cover letter I think we'll want
> a regression to stop so we can make steady progress. If you're open to
> it, I'll work it up.

I'm pretty reluctant to introduce a hard dependency on Github, because I'm
frankly pessimistic about Microsoft's dedication to free software in the
current political climate that is settling in the US. I would love to be
proven wrong, but at this point I'd prefer not to introduce any further
integrations with large US-hosted platforms.

-K

Re: [PATCH b4 0/5] Resolve some static typing errors

[Tamir Duberstein]

On Thu, Feb 6, 2025 at 12:53 PM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Thu, Jan 23, 2025 at 10:28:21PM +0200, Tamir Duberstein wrote:
> > > Note, that I had to do a follow-up fix for the missing Generator type import.
> > > I should have caught that during review, but missed it.
> > >
> > > -K
> >
> > Cheers, thanks for that.
> >
> > Would you be interested in a Github Actions config that runs the
> > static analyzer? As I mentioned in the cover letter I think we'll want
> > a regression to stop so we can make steady progress. If you're open to
> > it, I'll work it up.
>
> I'm pretty reluctant to introduce a hard dependency on Github, because I'm
> frankly pessimistic about Microsoft's dedication to free software in the
> current political climate that is settling in the US. I would love to be
> proven wrong, but at this point I'd prefer not to introduce any further
> integrations with large US-hosted platforms.
>
> -K

Makes sense. It wouldn't be difficult to put almost all the config in
an agnostic location, with a tiny bit of glue for Github Actions (or
whatever CI platform you prefer). Does that suit?