pw_presubmit: Update mypy execution
- Have mypy execute on whole Python modules, which gives better results.
- Update the Python module finding code to include the Python files
themselves.
- Fix a few mypy errors.
- Add a missing empty __init__.py that's necessary for mypy to pass.
- Update mypy and pylint versions since they're known to work.
Change-Id: Ib91faadbfc38e2312136be78cfff0086738bf2d4
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/15903
Commit-Queue: Wyatt Hepler <hepler@google.com>
Reviewed-by: Joe Ethier <jethier@google.com>
diff --git a/pw_presubmit/py/pw_presubmit/git_repo.py b/pw_presubmit/py/pw_presubmit/git_repo.py
index 8b1c8ee..0daaaea 100644
--- a/pw_presubmit/py/pw_presubmit/git_repo.py
+++ b/pw_presubmit/py/pw_presubmit/git_repo.py
@@ -13,14 +13,15 @@
# the License.
"""Helpful commands for working with a Git repository."""
-import collections
+import logging
from pathlib import Path
import subprocess
-from typing import Collection, Dict, Iterable, List, Optional
-from typing import Pattern, Union
+from typing import Collection, Iterable, Iterator, List, NamedTuple, Optional
+from typing import Pattern, Set, Tuple, Union
from pw_presubmit.tools import log_run, plural
+_LOG = logging.getLogger(__name__)
PathOrStr = Union[Path, str]
@@ -143,7 +144,7 @@
return Path(
git_stdout('rev-parse',
'--show-toplevel',
- repo=repo_path,
+ repo=repo_path if repo_path.is_dir() else repo_path.parent,
show_stderr=show_stderr))
@@ -167,25 +168,73 @@
return root(repo).joinpath(repo_path, *additional_repo_paths)
-def find_python_packages(python_paths: Iterable[PathOrStr],
- repo: PathOrStr = '.') -> Dict[Path, List[Path]]:
- """Returns Python package directories for the files in python_paths."""
- setup_pys = [
- file.parent.as_posix()
+class PythonPackage(NamedTuple):
+ root: Path # Path to the file containing the setup.py
+ package: Path # Path to the main package directory
+ packaged_files: Tuple[Path, ...] # All sources in the main package dir
+ other_files: Tuple[Path, ...] # Other Python files under root
+
+ def all_files(self) -> Tuple[Path, ...]:
+ return self.packaged_files + self.other_files
+
+
+def all_python_packages(repo: PathOrStr = '.') -> Iterator[PythonPackage]:
+ """Finds all Python packages in the repo based on setup.py locations."""
+ root_py_dirs = [
+ file.parent
for file in _ls_files(['setup.py', '*/setup.py'], Path(repo))
]
- package_dirs: Dict[Path, List[Path]] = collections.defaultdict(list)
+ for py_dir in root_py_dirs:
+ all_packaged_files = _ls_files([py_dir / '*' / '*.py'], repo=py_dir)
+ common_dir: Optional[str] = None
- for python_path in (Path(p).resolve().as_posix() for p in python_paths):
- try:
- setup_dir = max(setup for setup in setup_pys
- if python_path.startswith(setup))
- package_dirs[Path(setup_dir).resolve()].append(Path(python_path))
- except ValueError:
- continue
+ # Make there is only one package directory with Python files in it.
+ for file in all_packaged_files:
+ package_dir = file.relative_to(py_dir).parts[0]
- return package_dirs
+ if common_dir is None:
+ common_dir = package_dir
+ elif common_dir != package_dir:
+ _LOG.warning(
+ 'There are multiple Python package directories in %s: %s '
+ 'and %s. This is not supported by pw presubmit. Each '
+ 'setup.py should correspond with a single Python package',
+ py_dir, common_dir, package_dir)
+ break
+
+ if common_dir is not None:
+ packaged_files = tuple(_ls_files(['*/*.py'], repo=py_dir))
+ other_files = tuple(
+ f for f in _ls_files(['*.py'], repo=py_dir)
+ if f.name != 'setup.py' and f not in packaged_files)
+
+ yield PythonPackage(py_dir, py_dir / common_dir, packaged_files,
+ other_files)
+
+
+def python_packages_containing(
+ python_paths: Iterable[Path],
+ repo: PathOrStr = '.') -> Tuple[List[PythonPackage], List[Path]]:
+ """Finds all Python packages containing the provided Python paths.
+
+ Returns:
+ ([packages], [files_not_in_packages])
+ """
+ all_packages = list(all_python_packages(repo))
+
+ packages: Set[PythonPackage] = set()
+ files_not_in_packages: List[Path] = []
+
+ for python_path in python_paths:
+ for package in all_packages:
+ if package.root in python_path.parents:
+ packages.add(package)
+ break
+ else:
+ files_not_in_packages.append(python_path)
+
+ return list(packages), files_not_in_packages
def commit_message(commit: str = 'HEAD', repo: PathOrStr = '.') -> str:
diff --git a/pw_presubmit/py/pw_presubmit/presubmit.py b/pw_presubmit/py/pw_presubmit/presubmit.py
index 3f7b532..f0231b2 100644
--- a/pw_presubmit/py/pw_presubmit/presubmit.py
+++ b/pw_presubmit/py/pw_presubmit/presubmit.py
@@ -178,10 +178,18 @@
output_dir: Path
paths: Tuple[Path, ...]
- def relative_paths(self, start: Optional[Path] = None):
+ def relative_paths(self, start: Optional[Path] = None) -> Tuple[Path, ...]:
return tuple(
tools.relative_paths(self.paths, start if start else self.root))
+ def paths_by_repo(self) -> Dict[Path, List[Path]]:
+ repos = collections.defaultdict(list)
+
+ for path in self.paths:
+ repos[git_repo.root(path)].append(path)
+
+ return repos
+
class _Filter(NamedTuple):
endswith: Tuple[str, ...] = ('', )
diff --git a/pw_presubmit/py/pw_presubmit/python_checks.py b/pw_presubmit/py/pw_presubmit/python_checks.py
index 22083f2..1f1e0cd 100644
--- a/pw_presubmit/py/pw_presubmit/python_checks.py
+++ b/pw_presubmit/py/pw_presubmit/python_checks.py
@@ -32,7 +32,8 @@
import pw_presubmit
from pw_presubmit import call, filter_paths
-from pw_presubmit.git_repo import find_python_packages, list_files
+from pw_presubmit.git_repo import python_packages_containing, list_files
+from pw_presubmit.git_repo import PythonPackage
_LOG = logging.getLogger(__name__)
@@ -41,27 +42,29 @@
return call('python', '-m', *args, **kwargs)
+TEST_PATTERNS = ('*_test.py', )
+
+
@filter_paths(endswith='.py')
def test_python_packages(ctx: pw_presubmit.PresubmitContext,
- patterns: Iterable[str] = '*_test.py') -> None:
+ patterns: Iterable[str] = TEST_PATTERNS) -> None:
"""Finds and runs test files in Python package directories.
Finds the Python packages containing the affected paths, then searches
within that package for test files. All files matching the provided patterns
are executed with Python.
"""
- test_globs = [patterns] if isinstance(patterns, str) else list(patterns)
-
- packages: List[Path] = []
+ packages: List[PythonPackage] = []
for repo in ctx.repos:
- packages += find_python_packages(ctx.paths, repo=repo)
+ packages += python_packages_containing(ctx.paths, repo=repo)[0]
if not packages:
_LOG.info('No Python packages were found.')
return
for package in packages:
- for test in list_files(pathspecs=test_globs, repo_path=package):
+ for test in list_files(pathspecs=tuple(patterns),
+ repo_path=package.root):
call('python', test)
@@ -87,17 +90,27 @@
@filter_paths(endswith='.py')
def mypy(ctx: pw_presubmit.PresubmitContext) -> None:
+ """Runs mypy on all paths and their packages."""
+ packages: List[PythonPackage] = []
+ other_files: List[Path] = []
+
+ for repo, paths in ctx.paths_by_repo().items():
+ new_packages, files = python_packages_containing(paths, repo=repo)
+ packages += new_packages
+ other_files += files
+
+ for package in new_packages:
+ other_files += package.other_files
+
# Under some circumstances, mypy cannot check multiple Python files with the
# same module name. Group filenames so that no duplicates occur in the same
# mypy invocation. Also, omit setup.py from mypy checks.
filename_sets: List[Set[str]] = [set()]
- path_sets: List[List[Path]] = [[]]
+ path_sets: List[List[Path]] = [list(p.package for p in packages)]
- duplicates_ok = '__init__.py', '__main__.py'
-
- for path in (p for p in ctx.paths if p.name != 'setup.py'):
+ for path in (p for p in other_files if p.name != 'setup.py'):
for filenames, paths in zip(filename_sets, path_sets):
- if path.name in duplicates_ok or path.name not in filenames:
+ if path.name not in filenames:
paths.append(path)
filenames.add(path.name)
break
@@ -118,7 +131,8 @@
'--color-output',
'--show-error-codes',
# TODO(pwbug/146): Some imports from installed packages fail. These
- # imports should be fixed and this option removed.
+ # imports should be fixed and this option removed. See
+ # https://mypy.readthedocs.io/en/stable/installed_packages.html
'--ignore-missing-imports',
env=env)
diff --git a/pw_presubmit/py/setup.py b/pw_presubmit/py/setup.py
index 791691a..855082c 100644
--- a/pw_presubmit/py/setup.py
+++ b/pw_presubmit/py/setup.py
@@ -22,8 +22,8 @@
author_email='pigweed-developers@googlegroups.com',
description='Presubmit tools and a presubmit script for Pigweed',
install_requires=[
- 'mypy==0.770',
- 'pylint==2.5.2',
+ 'mypy==0.782',
+ 'pylint==2.5.3',
'yapf==0.30.0',
],
packages=setuptools.find_packages(),
diff --git a/pw_protobuf/py/pw_protobuf/__init__.py b/pw_protobuf/py/pw_protobuf/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/pw_protobuf/py/pw_protobuf/__init__.py
diff --git a/pw_protobuf/py/pw_protobuf/codegen_pwpb.py b/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
index 4588753..faf6bb9 100644
--- a/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
+++ b/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
@@ -18,11 +18,13 @@
import os
import sys
from typing import Dict, Iterable, List, Tuple
+from typing import cast
import google.protobuf.descriptor_pb2 as descriptor_pb2
from pw_protobuf.output_file import OutputFile
-from pw_protobuf.proto_tree import ProtoMessageField, ProtoNode
+from pw_protobuf.proto_tree import ProtoEnum, ProtoMessage, ProtoMessageField
+from pw_protobuf.proto_tree import ProtoNode
from pw_protobuf.proto_tree import build_node_tree
PLUGIN_NAME = 'pw_protobuf'
@@ -113,8 +115,12 @@
def _relative_type_namespace(self, from_root: bool = False) -> str:
"""Returns relative namespace between method's scope and field type."""
scope = self._root if from_root else self._scope
- ancestor = scope.common_ancestor(self._field.type_node())
- return self._field.type_node().cpp_namespace(ancestor)
+ type_node = self._field.type_node()
+ assert type_node is not None
+ ancestor = scope.common_ancestor(type_node)
+ namespace = type_node.cpp_namespace(ancestor)
+ assert namespace is not None
+ return namespace
class SubMessageMethod(ProtoMethod):
@@ -501,7 +507,7 @@
}
-def generate_code_for_message(message: ProtoNode, root: ProtoNode,
+def generate_code_for_message(message: ProtoMessage, root: ProtoNode,
output: OutputFile) -> None:
"""Creates a C++ class for a protobuf message."""
assert message.type() == ProtoNode.Type.MESSAGE
@@ -544,7 +550,7 @@
output.write_line('};')
-def define_not_in_class_methods(message: ProtoNode, root: ProtoNode,
+def define_not_in_class_methods(message: ProtoMessage, root: ProtoNode,
output: OutputFile) -> None:
"""Defines methods for a message class that were previously declared."""
assert message.type() == ProtoNode.Type.MESSAGE
@@ -567,7 +573,7 @@
output.write_line('}')
-def generate_code_for_enum(enum: ProtoNode, root: ProtoNode,
+def generate_code_for_enum(enum: ProtoEnum, root: ProtoNode,
output: OutputFile) -> None:
"""Creates a C++ enum for a proto enum."""
assert enum.type() == ProtoNode.Type.ENUM
@@ -579,12 +585,9 @@
output.write_line('};')
-def forward_declare(node: ProtoNode, root: ProtoNode,
+def forward_declare(node: ProtoMessage, root: ProtoNode,
output: OutputFile) -> None:
"""Generates code forward-declaring entities in a message's namespace."""
- if node.type() != ProtoNode.Type.MESSAGE:
- return
-
namespace = node.cpp_namespace(root)
output.write_line()
output.write_line(f'namespace {namespace} {{')
@@ -602,7 +605,7 @@
for child in node.children():
if child.type() == ProtoNode.Type.ENUM:
output.write_line()
- generate_code_for_enum(child, node, output)
+ generate_code_for_enum(cast(ProtoEnum, child), node, output)
output.write_line(f'}} // namespace {namespace}')
@@ -639,25 +642,28 @@
output.write_line(f'\nnamespace {file_namespace} {{')
for node in package:
- forward_declare(node, package, output)
+ if node.type() == ProtoNode.Type.MESSAGE:
+ forward_declare(cast(ProtoMessage, node), package, output)
# Define all top-level enums.
for node in package.children():
if node.type() == ProtoNode.Type.ENUM:
output.write_line()
- generate_code_for_enum(node, package, output)
+ generate_code_for_enum(cast(ProtoEnum, node), package, output)
# Run through all messages in the file, generating a class for each.
for node in package:
if node.type() == ProtoNode.Type.MESSAGE:
output.write_line()
- generate_code_for_message(node, package, output)
+ generate_code_for_message(cast(ProtoMessage, node), package,
+ output)
# Run a second pass through the classes, this time defining all of the
# methods which were previously only declared.
for node in package:
if node.type() == ProtoNode.Type.MESSAGE:
- define_not_in_class_methods(node, package, output)
+ define_not_in_class_methods(cast(ProtoMessage, node), package,
+ output)
if package.cpp_namespace():
output.write_line(f'\n}} // namespace {package.cpp_namespace()}')
diff --git a/pw_rpc/py/pw_rpc/codegen_nanopb.py b/pw_rpc/py/pw_rpc/codegen_nanopb.py
index 5aa21b8..5f1d1e5 100644
--- a/pw_rpc/py/pw_rpc/codegen_nanopb.py
+++ b/pw_rpc/py/pw_rpc/codegen_nanopb.py
@@ -16,9 +16,10 @@
from datetime import datetime
import os
from typing import Iterable
+from typing import cast
from pw_protobuf.output_file import OutputFile
-from pw_protobuf.proto_tree import ProtoNode, ProtoServiceMethod
+from pw_protobuf.proto_tree import ProtoNode, ProtoService, ProtoServiceMethod
from pw_protobuf.proto_tree import build_node_tree
import pw_rpc.ids
@@ -115,7 +116,8 @@
'Only unary and server streaming RPCs are currently supported')
-def _generate_method_lookup_function(service: ProtoNode, output: OutputFile):
+def _generate_method_lookup_function(service: ProtoService,
+ output: OutputFile):
"""Generates a function that gets the Method from a function pointer."""
output.write_line('template <auto impl_method>')
output.write_line(
@@ -141,7 +143,7 @@
output.write_line('}')
-def _generate_code_for_service(service: ProtoNode, root: ProtoNode,
+def _generate_code_for_service(service: ProtoService, root: ProtoNode,
output: OutputFile) -> None:
"""Generates a C++ derived class for a nanopb RPC service."""
@@ -255,7 +257,8 @@
for node in package:
if node.type() == ProtoNode.Type.SERVICE:
- _generate_code_for_service(node, package, output)
+ _generate_code_for_service(cast(ProtoService, node), package,
+ output)
output.write_line('\n} // namespace generated')
diff --git a/pw_rpc/py/pw_rpc/descriptors.py b/pw_rpc/py/pw_rpc/descriptors.py
index c9e8607..1713960 100644
--- a/pw_rpc/py/pw_rpc/descriptors.py
+++ b/pw_rpc/py/pw_rpc/descriptors.py
@@ -233,7 +233,7 @@
super().__init__(services)
-def get_method(service_accessor: ServiceAccessor[T], name: str) -> T:
+def get_method(service_accessor: ServiceAccessor, name: str):
"""Returns a method matching the given full name in a ServiceAccessor.
Args: