Repository: ilevkivskyi/com2ann

Issue:
Accepting multiple input files
It would be much easier to use this tool if it accepted mutliple files as input. I imagine that in that case, `-o` would be forbidden to avoid ambiguity.

I might do a PR, just wanted to check if it would be welcome before doing it.

Discussion:
ilevkivskyi: Generally I like the idea. Although, I would prefer if you can add some tests.
ewjoachim: I agree, but from looking (maybe too) briefly at the existing tests, I didn't find other tests around the CLI part. Does this mean we should add test on all of the CLI or just that part ?
ilevkivskyi: > Does this mean we should add test on all of the CLI or just that part ?

This depends on how much time and energy do you have :-)
ewjoachim: It may be worth introducing pre-commit to avoid going back and forth trying to find what the CI will complain about next :D
ewjoachim: Oh, btw, the alternative to this PR is to call `com2ann` with xargs this way:
```
#!/bin/bash
set -eu

# This script is a simple wrapper around com2ann which doesn't accept 0 or multiple
# arguments.

echo "$@" | xargs --no-run-if-empty --max-args=1 -- com2ann --add-future-imports --python-minor-version=7
```

(Of course, when using a wrapper like this, you hardcode the flags of `com2ann`. making it able to pass flags as well would make the bash script look much uglier than it already is)
ewjoachim: I've done it. That said, I'm really not convinced that the time spent had any positive consquence on either test correctness, readabilility, maintenability. While type annotations are really useful on the code itself, and I can understand the argument for making mypy really lound (making typing mandatory etc, which is the case here), for the tests, it really feels like this is taking it too far.

Well, here it is anyway.
ewjoachim: I've done it, but I think we're getting quite far past the line where this PR does one thing. There was a few evolutions that needed to be done in order for me to contribute, but I'm feeling that at this point, new unrelated improvements, even if they're welcome, belong in their own PR :)

Patch:
diff --git a/.github/workflows/lint_python.yml b/.github/workflows/lint_python.yml
index b916402..e709693 100644
--- a/.github/workflows/lint_python.yml
+++ b/.github/workflows/lint_python.yml
@@ -9,7 +9,7 @@ jobs:
         with:
           python-version: 3.x
       - run: pip install --upgrade pip wheel
-      - run: pip install bandit black codespell flake8 flake8-bugbear flake8-pyi isort mypy pytest pyupgrade safety
+      - run: pip install -r test-requirements.txt
       - run: bandit --recursive --skip B101 .  # B101 is assert statements
       - run: black --check . || true
       - run: codespell --ignore-words-list="fo"
diff --git a/src/com2ann.py b/src/com2ann.py
index b84deb1..7f3b5ef 100644
--- a/src/com2ann.py
+++ b/src/com2ann.py
@@ -9,19 +9,20 @@ We are especially careful about assignment statements, and keep the placement
 of additional (non-type) comments. For function definitions, we might introduce
 some formatting modifications, if the original formatting was too tricky.
 """
-import re
-import os
+from __future__ import annotations
+
+import argparse
 import ast
+import pathlib
+import re
 import sys
-import argparse
 import tokenize
-from tokenize import TokenInfo
-from enum import Enum, auto
 from collections import defaultdict
-from io import BytesIO
 from dataclasses import dataclass
-
-from typing import List, DefaultDict, Tuple, Optional, Union, Set
+from enum import Enum, auto
+from io import BytesIO
+from tokenize import TokenInfo
+from typing import Any, DefaultDict, Sequence, Union
 
 __all__ = ['com2ann', 'TYPE_COM']
 
@@ -91,8 +92,8 @@ class ArgComment:
 @dataclass
 class FunctionData:
     """Location data for translating function comment."""
-    arg_types: List[ArgComment]
-    ret_type: Optional[str]
+    arg_types: list[ArgComment]
+    ret_type: str | None
 
     # The line where 'def' appears.
     header_start_line: int
@@ -102,7 +103,7 @@ class FunctionData:
 
 class FileData:
     """Internal class describing global data on file."""
-    def __init__(self, lines: List[str], tokens: List[TokenInfo], tree: ast.AST) -> None:
+    def __init__(self, lines: list[str], tokens: list[TokenInfo], tree: ast.AST) -> None:
         # Source code lines.
         self.lines = lines
         # Tokens for the source code.
@@ -112,17 +113,17 @@ class FileData:
 
         # Map line number to token numbers. For example {1: [0, 1, 2, 3], 2: [4, 5]}
         # means that first to fourth tokens are on the first line.
-        token_tab: DefaultDict[int, List[int]] = defaultdict(list)
+        token_tab: DefaultDict[int, list[int]] = defaultdict(list)
         for i, tok in enumerate(tokens):
             token_tab[tok.start[0]].append(i)
         self.token_tab = token_tab
 
         # Basic translation logging.
-        self.success: List[int] = []  # list of lines where type comments where processed
-        self.fail: List[int] = []  # list of lines where type comments where rejected
+        self.success: list[int] = []  # list of lines where type comments where processed
+        self.fail: list[int] = []  # list of lines where type comments where rejected
 
         # Types we have inserted during translation.
-        self.seen: Set[str] = set()
+        self.seen: set[str] = set()
 
 
 class TypeCommentCollector(ast.NodeVisitor):
@@ -135,9 +136,9 @@ class TypeCommentCollector(ast.NodeVisitor):
         super().__init__()
         self.silent = silent
         # Type comments we can translate.
-        self.found: List[Union[AssignData, FunctionData]] = []
+        self.found: list[AssignData | FunctionData] = []
         # Type comments that are not supported yet (for reporting).
-        self.found_unsupported: List[int] = []
+        self.found_unsupported: list[int] = []
 
     def visit_Assign(self, s: ast.Assign) -> None:
         if s.type_comment:
@@ -204,7 +205,7 @@ class TypeCommentCollector(ast.NodeVisitor):
 
             args = self.process_per_arg_comments(fdef, num_non_defs, kw_non_defs)
 
-            ret: Optional[str]
+            ret: str | None
             if fdef.type_comment:
                 res = split_function_comment(fdef.type_comment, self.silent)
                 if not res:
@@ -244,7 +245,7 @@ class TypeCommentCollector(ast.NodeVisitor):
 
     def process_per_arg_comments(self, fdef: Function,
                                  num_non_defs: int,
-                                 kw_non_defs: Set[int]) -> List[ArgComment]:
+                                 kw_non_defs: set[int]) -> list[ArgComment]:
         """Collect information about per-argument function comments.
 
         These comments look like:
@@ -255,7 +256,7 @@ class TypeCommentCollector(ast.NodeVisitor):
             ):
                 ...
         """
-        args: List[ArgComment] = []
+        args: list[ArgComment] = []
 
         for i, a in enumerate(fdef.args.args):
             if a.type_comment:
@@ -285,15 +286,15 @@ class TypeCommentCollector(ast.NodeVisitor):
         return args
 
     def process_function_comment(self, fdef: Function,
-                                 f_args: List[str],
-                                 num_non_defs: int) -> Optional[List[ArgComment]]:
+                                 f_args: list[str],
+                                 num_non_defs: int) -> list[ArgComment] | None:
         """Combine location data for function arguments with types from a comment.
 
         f_args contains already split argument strings from the function type comment,
         for example if the comment is # type: (int, str) -> None, the f_args should be
         ['int', 'str'].
         """
-        args: List[ArgComment] = []
+        args: list[ArgComment] = []
 
         tot_args = len(fdef.args.args) + len(fdef.args.kwonlyargs)
         if fdef.args.vararg:
@@ -339,7 +340,7 @@ class TypeCommentCollector(ast.NodeVisitor):
         return args
 
 
-def split_sub_comment(comment: str) -> Tuple[str, Optional[str]]:
+def split_sub_comment(comment: str) -> tuple[str, str | None]:
     """Split extra comment from a type comment.
 
     The only non-trivial thing here is to take care of literal types,
@@ -359,7 +360,7 @@ def split_sub_comment(comment: str) -> Tuple[str, Optional[str]]:
 
 
 def split_function_comment(comment: str,
-                           silent: bool = False) -> Optional[Tuple[List[str], str]]:
+                           silent: bool = False) -> tuple[list[str], str] | None:
     """Split function type comment into argument types and return types.
 
     This also removes any additional sub-comment. For example:
@@ -388,7 +389,7 @@ def split_function_comment(comment: str,
         return None
 
     arg_list = arg_list[1:-1]
-    args: List[str] = []
+    args: list[str] = []
 
     # TODO: use tokenizer to guard against Literal[','].
     next_arg = ''
@@ -530,7 +531,7 @@ def process_assign(comment: AssignData, data: FileData,
                                           lvalue_line[comment.lvalue_end_offset:])
 
 
-def insert_arg_type(line: str, arg: ArgComment, seen: Set[str]) -> str:
+def insert_arg_type(line: str, arg: ArgComment, seen: set[str]) -> str:
     """Insert the argument type at a given location.
 
     Also record the type we translated.
@@ -552,7 +553,7 @@ def insert_arg_type(line: str, arg: ArgComment, seen: Set[str]) -> str:
     return new_line + ' = ' + rest
 
 
-def wrap_function_header(header: str) -> List[str]:
+def wrap_function_header(header: str) -> list[str]:
     """Wrap long function signature (header) one argument per line.
 
     Currently only headers that are initially one-line are supported.
@@ -568,11 +569,11 @@ def wrap_function_header(header: str) -> List[str]:
             ...
     """
     # TODO: use tokenizer to guard against Literal[','].
-    parts: List[str] = []
+    parts: list[str] = []
     next_part = ''
     nested = 0
     complete = False  # Did we split all the arguments inside (...)?
-    indent: Optional[int] = None
+    indent: int | None = None
 
     for i, c in enumerate(header):
         if c in '([{':
@@ -740,7 +741,7 @@ def com2ann(code: str, *,
             silent: bool = False,
             add_future_imports: bool = False,
             wrap_sig: int = 0,
-            python_minor_version: int = -1) -> Optional[Tuple[str, FileData]]:
+            python_minor_version: int = -1) -> tuple[str, FileData] | None:
     """Translate type comments to type annotations in code.
 
     Take code as string and return this string where::
@@ -832,65 +833,111 @@ def translate_file(infile: str, outfile: str, options: Options) -> None:
         fo.write(new_code.encode(enc))
 
 
-def main() -> None:
+def parse_cli_args(args: Sequence[str] | None = None) -> dict[str, Any]:
     parser = argparse.ArgumentParser(description=__doc__)
+    parser.add_argument("infiles",
+                        nargs="*",
+                        type=pathlib.Path,
+                        help="input files or directories for translation, must\n"
+                             "contain no syntax errors;\n"
+                             "if --outfile is not given or multiple input files are\n"
+                             "given, translation is made *in place*")
+
     parser.add_argument("-o", "--outfile",
+                        type=pathlib.Path,
                         help="output file or directory, will be overwritten if exists,\n"
-                             "defaults to input file or directory")
-    parser.add_argument("infile",
-                        help="input file or directory for translation, must\n"
-                             "contain no syntax errors;\n"
-                             "if --outfile is not given, translation is\n"
-                             "made *in place*")
+                             "defaults to input file or directory. Cannot be used\n"
+                             "when multiple input files are defined. infiles and\n"
+                             "outfile must be of the same type (file vs. directory)")
+
     parser.add_argument("-s", "--silent",
                         help="do not print summary for line numbers of\n"
                              "translated and rejected comments",
                         action="store_true")
+
     parser.add_argument("-n", "--drop-none",
                         help="drop any None as assignment value during\n"
                         "translation if it is annotated by a type comment",
                         action="store_true")
+
     parser.add_argument("-e", "--drop-ellipsis",
                         help="drop any Ellipsis (...) as assignment value during\n"
                         "translation if it is annotated by a type comment",
                         action="store_true")
+
     parser.add_argument("-i", "--add-future-imports",
                         help="add 'from __future__ import annotations' to any file\n"
                         "where type comments were successfully translated",
                         action="store_true")
+
     parser.add_argument("-w", "--wrap-signatures",
                         help="wrap function headers that are longer than given length",
                         type=int, default=0)
+
     parser.add_argument("-v", "--python-minor-version",
                         help="Python 3 minor version to use to parse the files",
                         type=int, default=-1)
 
-    args = parser.parse_args()
-    if args.outfile is None:
-        args.outfile = args.infile
+    options = parser.parse_args(args)
+
+    infiles: list[pathlib.Path] = options.infiles
+    outfile: pathlib.Path | None = options.outfile
+
+    if not infiles:
+        parser.exit(status=0, message="No input file, exiting")
 
-    options = Options(args.drop_none, args.drop_ellipsis,
-                      args.silent, args.add_future_imports,
-                      args.wrap_signatures,
-                      args.python_minor_version)
+    missing_files = [file for file in infiles if not file.exists()]
+    if missing_files:
+        parser.error(f"File(s) not found: {', '.join(str(e) for e in missing_files)}")
 
-    if os.path.isfile(args.infile):
-        translate_file(args.infile, args.outfile, options)
+    if len(infiles) == 1:
+        if outfile and outfile.exists() and infiles[0].is_dir() != outfile.is_dir():
+            parser.error("Infile must be the same type as outfile (file vs. directory)")
     else:
-        if os.path.isfile(args.outfile):
-            print("If input is a directory, output must not be a file",
-                  file=sys.stderr)
-            exit(2)
-        for root, _, files in os.walk(args.infile):
-            rel_root = os.path.relpath(root, args.infile)
-            out_root = os.path.join(args.outfile, rel_root)
-            os.makedirs(out_root, exist_ok=True)
-            for file in files:
-                _, ext = os.path.splitext(file)
-                if ext == '.py' or ext == '.pyi':
-                    file_name = os.path.join(root, file)
-                    out_file_name = os.path.join(out_root, file)
-                    translate_file(file_name, out_file_name, options)
+        if outfile is not None:
+            parser.error("Cannot use --outfile if multiple infiles are given")
+
+    return vars(options)
+
+
+def rebase_path(
+    path: pathlib.Path, root: pathlib.Path, new_root: pathlib.Path
+) -> pathlib.Path:
+    """
+    Generate a path that is at the same location relative to `new_root` as `path`
+    was relative to `root`
+    """
+    return new_root / path.relative_to(root)
+
+
+def process_single_entry(
+    in_path: pathlib.Path, out_path: pathlib.Path, options: Options
+) -> None:
+
+    if in_path.is_file():
+        translate_file(infile=str(in_path), outfile=str(out_path), options=options)
+    else:
+        for in_file in sorted(in_path.glob("**/*.py*")):
+            if in_file.suffix not in [".py", ".pyi"] or in_file.is_dir():
+                continue
+
+            out_file = rebase_path(path=in_file, root=in_path, new_root=out_path)
+            if out_file != in_file:
+                out_file.parent.mkdir(parents=True, exist_ok=True)
+
+            translate_file(infile=str(in_file), outfile=str(out_file), options=options)
+
+
+def main() -> None:
+    args = parse_cli_args()
+
+    infiles = args.pop("infiles")
+    outfile = args.pop("outfile")
+    options = Options(**args)
+
+    for infile in infiles:
+        outfile = outfile or infile
+        process_single_entry(in_path=infile, out_path=outfile, options=options)
 
 
 if __name__ == '__main__':


Test patch:
diff --git a/src/test_cli.py b/src/test_cli.py
new file mode 100644
index 0000000..b6c52c4
--- /dev/null
+++ b/src/test_cli.py
@@ -0,0 +1,218 @@
+from __future__ import annotations
+
+import os
+import pathlib
+from typing import Any, Callable, Iterable, TypedDict
+
+import pytest
+
+import com2ann
+
+
+@pytest.fixture
+def test_path(tmp_path: pathlib.Path) -> Iterable[pathlib.Path]:
+    old_path = pathlib.Path.cwd()
+    os.chdir(tmp_path)
+    yield tmp_path
+    os.chdir(old_path)
+
+
+class Exited(Exception):
+    pass
+
+
+class ParseResult(TypedDict, total=False):
+    status: int
+    args: dict[str, Any]
+    error: bool
+    out: str
+    err: str
+
+
+ParseCallable = Callable[..., ParseResult]
+
+
+@pytest.fixture
+def parse(
+    capsys: pytest.CaptureFixture[str],
+) -> ParseCallable:
+    # parse is a pytext fixture returning a function
+    # This function will:
+    # - Ensure that SystemExit exceptions raised by ArgParse are
+    #   caught before they exit pytest
+    # - Save the result of the arg parsing if successful
+    # - Save the status code otherwise
+    # - Save and print whatever argparse printed to stdout and stderr
+
+    def _(*args: str) -> ParseResult:
+        result: ParseResult = {"status": 0}
+        try:
+            result.update({"args": com2ann.parse_cli_args(args), "error": False})
+        except SystemExit as exc:
+            result.update({"status": exc.code})
+
+        out, err = capsys.readouterr()
+        # Will only display if the test fails
+        if out:
+            print("stdout:", out)
+            result["out"] = out
+        if err:
+            print("stderr:", err)
+            result["err"] = err
+
+        return result
+
+    return _
+
+
+def test_parse_cli_args__minimal(parse: ParseCallable, test_path: pathlib.Path) -> None:
+    (test_path / "a.py").touch()
+
+    assert parse("a.py")["args"] == {
+        "add_future_imports": False,
+        "drop_ellipsis": False,
+        "drop_none": False,
+        "infiles": [pathlib.Path("a.py")],
+        "outfile": None,
+        "python_minor_version": -1,
+        "silent": False,
+        "wrap_signatures": 0,
+    }
+
+
+def test_parse_cli_args__maximal(parse: ParseCallable, test_path: pathlib.Path) -> None:
+    (test_path / "a.py").touch()
+
+    assert parse(
+        "a.py",
+        "--outfile=b.py",
+        "--add-future-imports",
+        "--drop-ellipsis",
+        "--drop-none",
+        "--python-minor-version=7",
+        "--silent",
+        "--wrap-signatures=88",
+    )["args"] == {
+        "add_future_imports": True,
+        "drop_ellipsis": True,
+        "drop_none": True,
+        "infiles": [pathlib.Path("a.py")],
+        "outfile": pathlib.Path("b.py"),
+        "python_minor_version": 7,
+        "silent": True,
+        "wrap_signatures": 88,
+    }
+
+
+def test_parse_cli_args__no_infile(parse: ParseCallable) -> None:
+    result = parse()
+
+    assert result == {"err": "No input file, exiting", "status": 0}
+
+
+def test_parse_cli_args__missing_file(parse: ParseCallable) -> None:
+
+    result = parse("a.py")
+    assert result["status"] == 2
+    assert "File(s) not found: a.py" in result["err"]
+
+
+@pytest.mark.parametrize(
+    "infile, outfile",
+    [
+        ("file.py", "dir"),
+        ("dir", "file.py"),
+    ],
+)
+def test_parse_cli_args__in_out_type_mismatch(
+    parse: ParseCallable, test_path: pathlib.Path, infile: str, outfile: str
+) -> None:
+    (test_path / "file.py").touch()
+    (test_path / "dir").mkdir()
+
+    result = parse(infile, "--outfile", outfile)
+    assert result["status"] == 2
+    assert "Infile must be the same type" in result["err"]
+
+
+def test_parse_cli_args__outfile_doesnt_exist(
+    parse: ParseCallable, test_path: pathlib.Path
+) -> None:
+    (test_path / "dir").mkdir()
+
+    result = parse("dir", "--outfile", "other_dir")
+    assert result["status"] == 0
+
+
+def test_parse_cli_args__multiple_inputs_and_output(
+    parse: ParseCallable, test_path: pathlib.Path
+) -> None:
+    (test_path / "a.py").touch()
+    (test_path / "b.py").touch()
+
+    result = parse("a.py", "b.py", "--outfile", "c.py")
+    assert result["status"] == 2
+    assert "Cannot use --outfile if multiple infiles are given" in result["err"]
+
+
+def test_rebase_path() -> None:
+    assert com2ann.rebase_path(
+        path=pathlib.Path("a/b/c/d"),
+        root=pathlib.Path("a/b/"),
+        new_root=pathlib.Path("e/f"),
+    ) == pathlib.Path("e/f/c/d")
+
+
+@pytest.fixture
+def options() -> com2ann.Options:
+    return com2ann.Options(drop_none=True, drop_ellipsis=True, silent=False)
+
+
+@pytest.fixture
+def translate_file(mocker: Any) -> Any:
+    return mocker.patch("com2ann.translate_file", autospec=True)
+
+
+def test_process_single_entry__file(
+    test_path: pathlib.Path, translate_file: Any, options: com2ann.Options
+) -> None:
+    in_path = test_path / "a.py"
+    in_path.touch()
+
+    out_path = test_path / "b.py"
+
+    com2ann.process_single_entry(in_path=in_path, out_path=out_path, options=options)
+
+    translate_file.assert_called_with(
+        infile=str(in_path),
+        outfile=str(out_path),
+        options=options,
+    )
+
+
+def test_process_single_entry__dir(
+    test_path: pathlib.Path, translate_file: Any, options: com2ann.Options, mocker: Any
+) -> None:
+    in_path = test_path / "a"
+    (in_path / "c/d/e").mkdir(parents=True)
+    (in_path / "c/d/e/f.txt").touch()
+    (in_path / "c/d/e/f.py").touch()
+    (in_path / "c/d/e/f.pyi").touch()
+    (in_path / "c/d/e/f.pyx").touch()
+
+    out_path = test_path / "b"
+
+    com2ann.process_single_entry(in_path=in_path, out_path=out_path, options=options)
+
+    assert translate_file.mock_calls == [
+        mocker.call(
+            infile=str(in_path / "c/d/e/f.py"),
+            outfile=str(out_path / "c/d/e/f.py"),
+            options=options,
+        ),
+        mocker.call(
+            infile=str(in_path / "c/d/e/f.pyi"),
+            outfile=str(out_path / "c/d/e/f.pyi"),
+            options=options,
+        ),
+    ]
diff --git a/test-requirements.txt b/test-requirements.txt
index 8fef360..5bd3f7a 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -1,7 +1,15 @@
-flake8; python_version >= '3.6'
-flake8-bugbear; python_version >= '3.6'
-flake8-pyi; python_version >= '3.6'
-pytest>=2.8; python_version >= '3.3'
-pytest-xdist>=1.13; python_version >= '3.3'
-pytest-cov>=2.4.0; python_version >= '3.3'
-mypy; python_version <= '3.7'
+bandit
+black
+codespell
+flake8
+flake8-bugbear
+flake8-pyi
+isort
+mypy
+pytest
+pytest-cov>=2.4.0
+pytest-mock>=3.7.0
+pytest-xdist>=1.13
+pyupgrade
+safety
+types-setuptools