Resolve some static typing errors
cover Resolve some static typing errors
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.
---
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,
Signature · 1 line
Tamir Duberstein <[email protected]>
PATCH 1/5 Add development dependencies
--- pyproject.toml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
pyproject.toml +7 -1
@@ -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"] }
Signature · 1 line
2.47.0
PATCH 2/5 Avoid file descriptor leak
In reply to Tamir Duberstein--- src/b4/diff.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
src/b4/diff.py +3 -3
@@ -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)
Signature · 1 line
2.47.0
PATCH 3/5 Correctly type annotate generators
In reply to Tamir Duberstein--- src/b4/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
src/b4/__init__.py +4 -4
@@ -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()
Signature · 1 line
2.47.0
PATCH 4/5 Provide overloads for git_run_command
In reply to Tamir DubersteinThis 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.
--- src/b4/__init__.py | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)
src/b4/__init__.py +26 -11
@@ -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]:
Signature · 1 line
2.47.0
PATCH 5/5 Add missing imports
In reply to Tamir Duberstein--- 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(-)
src/b4/__init__.py +5 -4
@@ -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
src/b4/diff.py +1 -0
@@ -11,6 +11,7 @@ import b4 import b4.mbox import mailbox import email +import email.message import email.parser import shutil import pathlib
src/b4/ez.py +2 -0
@@ -17,6 +17,8 @@ import datetime import json import shlex import email +import email.policy +import email.utils import pathlib import base64 import textwrap
src/b4/pr.py +2 -0
@@ -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
src/b4/ty.py +1 -0
@@ -13,6 +13,7 @@ import re import email import email.message import email.policy +import email.utils import json import argparse
src/tests/test___init__.py +1 -0
@@ -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]
Signature · 1 line
2.47.0
Konstantin Ryabitsev
AppliedQuoted 9 lines · 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.
[...]
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: 21904f395d068f0b957972d148fd5d326a904920Best regards,
Signature · 1 line
Konstantin Ryabitsev <[email protected]>
Quoted 11 lines · Konstantin Ryabitsev
Quoted 9 lines
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
Quoted 10 lines · Konstantin Ryabitsev
Quoted 8 lines · Konstantin Ryabitsev
Quoted 8 lines
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.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
[...]
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
Quoted 11 lines · Tamir Duberstein
Quoted 4 lines
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
Quoted 14 lines · Konstantin Ryabitsev
Quoted 11 lines · Tamir Duberstein
Quoted 4 lines
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.
Quoted 7 lines
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?