pw_presubmit: Separate format arguments function - Create the argument parser for format_code.py in a function. - Split the repository-aware version of formatting to a separate function, so users can decide whether to use Git paths or not. - Generalize some type annotations that were too specific. Change-Id: Ic09b15fee8de1578dc2f3ca43956796841ae6f90
diff --git a/pw_presubmit/py/pw_presubmit/format_code.py b/pw_presubmit/py/pw_presubmit/format_code.py index fcb8a0d..a12593b 100755 --- a/pw_presubmit/py/pw_presubmit/format_code.py +++ b/pw_presubmit/py/pw_presubmit/format_code.py
@@ -29,7 +29,7 @@ import subprocess import sys from typing import Callable, Collection, Dict, Iterable, List, NamedTuple -from typing import Optional, Sequence +from typing import Optional, Pattern, Sequence try: import pw_presubmit @@ -312,7 +312,7 @@ class CodeFormatter: """Checks or fixes the formatting of a set of files.""" - def __init__(self, files: Sequence[Path]): + def __init__(self, files: Collection[Path]): self.paths = list(files) self._formats: Dict[CodeFormat, List] = collections.defaultdict(list) @@ -347,13 +347,15 @@ return [] -def format_paths(paths: Sequence[Path], exclude, base: str, fix: bool) -> int: +def format_paths_in_repo(paths: Collection[Path], + exclude: Collection[Pattern[str]], fix: bool, + base: str) -> int: """Checks or fixes formatting for files in a Git repo.""" files = [path.resolve() for path in paths if path.is_file()] + repo = pw_presubmit.git_repo_path() if pw_presubmit.is_git_repo() else None # If this is a Git repo, list the original paths with git ls-files or diff. - if pw_presubmit.is_git_repo(): - repo = pw_presubmit.git_repo_path() + if repo: _LOG.info( 'Formatting %s', pw_presubmit.describe_files_in_repo(repo, Path.cwd(), base, paths, @@ -366,13 +368,19 @@ 'A base commit may only be provided if running from a Git repo') return 1 - formatter = CodeFormatter(files) + return format_files(files, fix, repo=repo) + + +def format_files(paths: Collection[Path], + fix: bool, + repo: Optional[Path] = None) -> int: + """Checks or fixes formatting for the specified files.""" + formatter = CodeFormatter(paths) _LOG.info('Checking formatting for %s', plural(formatter.paths, 'file')) - _LOG.debug('Files to format:\n%s', '\n'.join(str(f) for f in files)) + _LOG.debug('Files to format:\n%s', '\n'.join(str(f) for f in paths)) - for line in _file_summary( - files, repo if pw_presubmit.is_git_repo() else Path.cwd()): + for line in _file_summary(paths, repo if repo else Path.cwd()): print(line, file=sys.stderr) errors = formatter.check() @@ -392,15 +400,38 @@ return 0 -def main() -> int: - """Check and fix formatting for source files.""" +def arguments(git_paths: bool) -> argparse.ArgumentParser: + """Creates an argument parser for format_files or format_paths_in_repo.""" + parser = argparse.ArgumentParser(description=__doc__) - pw_presubmit.add_path_arguments(parser) + + if git_paths: + pw_presubmit.add_path_arguments(parser) + else: + + def existing_path(arg: str) -> Path: + path = Path(arg) + if not path.is_file(): + raise argparse.ArgumentTypeError( + f'{arg} is not a path to a file') + + return path + + parser.add_argument('paths', + metavar='path', + nargs='+', + type=existing_path, + help='File paths to check') + parser.add_argument('--fix', action='store_true', help='Apply formatting fixes in place.') + return parser - return format_paths(**vars(parser.parse_args())) + +def main() -> int: + """Check and fix formatting for source files.""" + return format_paths_in_repo(**vars(arguments(git_paths=True).parse_args())) if __name__ == '__main__':
diff --git a/pw_presubmit/py/pw_presubmit/tools.py b/pw_presubmit/py/pw_presubmit/tools.py index 3e3b543..b4c6f9e 100644 --- a/pw_presubmit/py/pw_presubmit/tools.py +++ b/pw_presubmit/py/pw_presubmit/tools.py
@@ -56,8 +56,8 @@ import subprocess import sys import time -from typing import Any, Callable, Dict, Iterable, Iterator, List, NamedTuple -from typing import Optional, Pattern, Sequence, Tuple, Union +from typing import Any, Callable, Collection, Dict, Iterable, Iterator, List +from typing import NamedTuple, Optional, Pattern, Sequence, Tuple, Union from inspect import signature _LOG: logging.Logger = logging.getLogger(__name__) @@ -98,14 +98,14 @@ check=True).stdout.decode().strip() -def _git_ls_files(args: Sequence[PathOrStr], repo: Path) -> List[Path]: +def _git_ls_files(args: Collection[PathOrStr], repo: Path) -> List[Path]: return [ repo.joinpath(path).resolve() for path in git_stdout( 'ls-files', '--', *args, repo=repo).splitlines() ] -def _git_diff_names(commit: str, paths: Sequence[PathOrStr], +def _git_diff_names(commit: str, paths: Collection[PathOrStr], repo: Path) -> List[Path]: """Returns absolute paths of files changed since the specified commit.""" root = git_repo_path(repo=repo) @@ -122,8 +122,8 @@ def list_git_files(commit: Optional[str] = None, - paths: Sequence[PathOrStr] = (), - exclude: Sequence[Pattern[str]] = (), + paths: Collection[PathOrStr] = (), + exclude: Collection[Pattern[str]] = (), repo: Optional[Path] = None) -> List[Path]: """Lists files with git ls-files or git diff --name-only. @@ -144,8 +144,8 @@ def _describe_constraints(root: Path, repo_path: Path, commit: Optional[str], - pathspecs: Sequence[PathOrStr], - exclude: Sequence[Pattern]) -> Iterator[str]: + pathspecs: Collection[PathOrStr], + exclude: Collection[Pattern]) -> Iterator[str]: if not root.samefile(repo_path): yield (f'under the {repo_path.resolve().relative_to(root.resolve())} ' 'subdirectory') @@ -163,8 +163,8 @@ def describe_files_in_repo(root: Path, repo_path: Path, commit: Optional[str], - pathspecs: Sequence[PathOrStr], - exclude: Sequence[Pattern]) -> str: + pathspecs: Collection[PathOrStr], + exclude: Collection[Pattern]) -> str: """Completes 'Doing something to ...' for a set of files in a Git repo.""" constraints = list( _describe_constraints(root, repo_path, commit, pathspecs, exclude)) @@ -486,12 +486,12 @@ parser.add_argument( 'paths', + metavar='path', nargs='*', type=Path, - help=( - 'Paths to which to restrict the presubmit checks. ' - 'Directories are expanded with git ls-files. ' - 'If --base is provided, all paths are interpreted as Git paths.')) + help=('Paths to which to restrict the checks. These are interpreted ' + 'as Git pathspecs. If --base is provided, only paths changed ' + 'since that commit are checked.')) parser.add_argument( '-b', '--base',