diff --git a/CHANGES.rst b/CHANGES.rst index 268e1299a..ba73f0069 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,6 +12,10 @@ Added ``--target-version`` command line option - In ``README.rst``, link to GitHub searches which find public repositories that use Darker. +- Linters are now run twice: once for ``rev1`` to get a baseline, and another time for + ``rev2`` to get the current situation. Old linter messages which fall on unmodified + lines are hidden, so effectively the user gets new linter messages introduced by + latest changes, as well as persistent linter messages on modified lines. Fixed ----- diff --git a/README.rst b/README.rst index 32184585d..c73b1496d 100644 --- a/README.rst +++ b/README.rst @@ -31,8 +31,13 @@ What? ===== This utility reformats and checks Python source code files. -However, when run in a Git repository, it only applies reformatting and reports errors -in regions which have changed in the Git working tree since the last commit. +However, when run in a Git repository, it compares an old revision of the source tree +to a newer revision (or the working tree). It then + +- only applies reformatting in regions which have changed in the Git working tree + between the two revisions, and +- only reports those linting messages which appeared after the modifications to the + source code files. The reformatters supported are: @@ -726,7 +731,8 @@ It defaults to ``"--check --diff --color"``. You can e.g. add ``"--isort"`` to sort imports, or ``"--verbose"`` for debug logging. To run linters through Darker, you can provide a comma separated list of linters using -the ``lint:`` option. Only ``flake8``, ``pylint`` and ``mypy`` are supported. +the ``lint:`` option. Only ``flake8``, ``pylint`` and ``mypy`` are supported. Other +linters may or may not work with Darker, depending on their message output format. Versions can be constrained using ``pip`` syntax, e.g. ``"flake8>=3.9.2"``. *New in version 1.1.0:* @@ -745,7 +751,9 @@ The ``lint:`` option. Using linters ============= -One way to use Darker is to filter linter output to modified lines only. +One way to use Darker is to filter linter output to only those linter messages +which appeared after the modifications to source code files, +as well as old messages which concern modified lines. Darker supports any linter with output in one of the following formats:: :: @@ -892,7 +900,9 @@ are applied to the edited file. Also, in case the ``--isort`` option was specified, isort_ is run on each edited file before applying Black_. Similarly, each linter requested using the `--lint ` option is run, -and only linting errors/warnings on modified lines are displayed. +and only those linting messages are displayed which appeared after the modifications to +the source code files, +or which were there already before but now fall on modified lines. License diff --git a/src/darker/__main__.py b/src/darker/__main__.py index 500ad8170..190c27d25 100644 --- a/src/darker/__main__.py +++ b/src/darker/__main__.py @@ -350,11 +350,14 @@ def main( # pylint: disable=too-many-locals,too-many-branches,too-many-statemen 8. verify that the resulting reformatted source code parses to an identical AST as the original edited to-file 9. write the reformatted source back to the original file - 10. run linter subprocesses for all edited files (10.-13. optional) - 11. diff the given revision and worktree (after isort and Black reformatting) for - each file reported by a linter - 12. extract line numbers in each file reported by a linter for changed lines - 13. print only linter error lines which fall on changed lines + 10. run linter subprocesses twice for all modified and unmodified files which are + mentioned on the command line: first establish a baseline by running against + ``rev1``, then get current linting status by running against the working tree + (steps 10.-12. are optional) + 11. create a mapping from line numbers of unmodified lines in the current versions + to corresponding line numbers in ``rev1`` + 12. hide linter messages which appear in the current versions and identically on + corresponding lines in ``rev1``, and show all other linter messages :param argv: The command line arguments to the ``darker`` command :return: 1 if the ``--check`` argument was provided and at least one file was (or @@ -423,38 +426,35 @@ def main( # pylint: disable=too-many-locals,too-many-branches,too-many-statemen f"Error: Path(s) {missing_reprs} do not exist in {rev2_repr}", ) - # These are absolute paths: + # These paths are relative to `root`: files_to_process = filter_python_files(paths, root, {}) files_to_blacken = filter_python_files(paths, root, black_config) + # Now decide which files to reformat (Black & isort). Note that this doesn't apply + # to linting. if output_mode == OutputMode.CONTENT: - # With `-d` / `--stdout`, process the file whether modified or not. Paths have + # With `-d` / `--stdout`, reformat the file whether modified or not. Paths have # previously been validated to contain exactly one existing file. - changed_files_to_process = { - p.resolve().relative_to(root) for p in files_to_process - } + changed_files_to_reformat = files_to_process black_exclude = set() else: - # In other modes, only process files which have been modified. + # In other modes, only reformat files which have been modified. if git_is_repository(root): - changed_files_to_process = git_get_modified_python_files( + changed_files_to_reformat = git_get_modified_python_files( files_to_process, revrange, root ) else: - changed_files_to_process = { - path.relative_to(root) for path in files_to_process - } + changed_files_to_reformat = files_to_process black_exclude = { str(path) - for path in changed_files_to_process - if root / path not in files_to_blacken + for path in changed_files_to_reformat + if path not in files_to_blacken } - use_color = should_use_color(config["color"]) formatting_failures_on_modified_lines = False for path, old, new in sorted( format_edited_parts( root, - changed_files_to_process, + changed_files_to_reformat, Exclusions(black=black_exclude, isort=set() if args.isort else {"**/*"}), revrange, black_config, @@ -472,7 +472,8 @@ def main( # pylint: disable=too-many-locals,too-many-branches,too-many-statemen linter_failures_on_modified_lines = run_linters( args.lint, root, - changed_files_to_process, + # paths to lint are not limited to modified files or just Python files: + {p.resolve().relative_to(root) for p in paths}, revrange, use_color, ) diff --git a/src/darker/black_diff.py b/src/darker/black_diff.py index 5e90eef99..ab63d81cb 100644 --- a/src/darker/black_diff.py +++ b/src/darker/black_diff.py @@ -146,8 +146,8 @@ def filter_python_files( :param root: A common root directory for all ``paths`` :param black_config: Black configuration which contains the exclude options read from Black's configuration files - :return: Absolute paths of files which should be reformatted according to - ``black_config`` + :return: Paths of files which should be reformatted according to + ``black_config``, relative to ``root``. """ sig = inspect.signature(gen_python_files) @@ -172,7 +172,7 @@ def filter_python_files( **kwargs, # type: ignore[arg-type] ) ) - return files_from_directories | files + return {p.resolve().relative_to(root) for p in files_from_directories | files} def run_black(src_contents: TextDocument, black_config: BlackConfig) -> TextDocument: diff --git a/src/darker/diff.py b/src/darker/diff.py index dad5c5db1..738f04d73 100644 --- a/src/darker/diff.py +++ b/src/darker/diff.py @@ -66,7 +66,7 @@ import logging from difflib import SequenceMatcher -from typing import Generator, List, Sequence, Tuple +from typing import Dict, Generator, List, Sequence, Tuple from darker.multiline_strings import find_overlap from darker.utils import DiffChunk, TextDocument @@ -203,3 +203,36 @@ def diff_chunks(src: TextDocument, dst: TextDocument) -> List[DiffChunk]: """ opcodes = diff_and_get_opcodes(src, dst) return list(opcodes_to_chunks(opcodes, src, dst)) + + +def map_unmodified_lines(src: TextDocument, dst: TextDocument) -> Dict[int, int]: + """Return a mapping of line numbers of unmodified lines between dst and src docs + + After doing a diff between ``src`` and ``dst``, some identical chunks of lines may + be identified. For each such chunk, a mapping from every line number of the chunk in + ``dst`` to corresponding line number in ``src`` is added. + + :param src: The original text document + :param dst: The modified text document + :return: A mapping from ``dst`` lines to corresponding unmodified ``src`` lines. + Line numbers are 1-based. + + """ + opcodes = diff_and_get_opcodes(src, dst) + _validate_opcodes(opcodes) + if not src.string and not dst.string: + # empty files may get linter messages on line 1 + return {1: 1} + result = {} + for tag, src_start, src_end, dst_start, dst_end in opcodes: + if tag != "equal": + continue + for line_delta in range(dst_end - dst_start): + result[dst_start + line_delta + 1] = src_start + line_delta + 1 + if line_delta != src_end - src_start - 1: + raise RuntimeError( + "Something is wrong, 'equal' diff blocks should have the same length." + f" src_start={src_start}, src_end={src_end}," + f" dst_start={dst_start}, dst_end={dst_end}" + ) + return result diff --git a/src/darker/git.py b/src/darker/git.py index 6e5fc39e7..2c226b945 100644 --- a/src/darker/git.py +++ b/src/darker/git.py @@ -362,7 +362,7 @@ def get_missing_at_revision(paths: Iterable[Path], rev2: str, cwd: Path) -> Set[ def _git_diff_name_only( - rev1: str, rev2: str, relative_paths: Set[Path], cwd: Path + rev1: str, rev2: str, relative_paths: Iterable[Path], cwd: Path ) -> Set[Path]: """Collect names of changed files between commits from Git @@ -389,7 +389,7 @@ def _git_diff_name_only( return {Path(line) for line in lines} -def _git_ls_files_others(relative_paths: Set[Path], cwd: Path) -> Set[Path]: +def _git_ls_files_others(relative_paths: Iterable[Path], cwd: Path) -> Set[Path]: """Collect names of untracked non-excluded files from Git This will return those files in ``relative_paths`` which are untracked and not @@ -419,21 +419,69 @@ def git_get_modified_python_files( - ``git diff --name-only --relative -- `` - ``git ls-files --others --exclude-standard -- `` - :param paths: Paths to the files to diff + :param paths: Relative paths to the files to diff :param revrange: Git revision range to compare :param cwd: The Git repository root :return: File names relative to the Git repository root """ - relative_paths = {p.resolve().relative_to(cwd) for p in paths} - changed_paths = _git_diff_name_only( - revrange.rev1, revrange.rev2, relative_paths, cwd - ) + changed_paths = _git_diff_name_only(revrange.rev1, revrange.rev2, paths, cwd) if revrange.rev2 == WORKTREE: - changed_paths.update(_git_ls_files_others(relative_paths, cwd)) + changed_paths.update(_git_ls_files_others(paths, cwd)) return {path for path in changed_paths if should_reformat_file(cwd / path)} +def git_clone_local(source_repository: Path, revision: str, destination: Path) -> Path: + """Clone a local repository and check out the given revision + + :param source_repository: Path to the root of the local repository checkout + :param revision: The revision to check out, or ``HEAD`` + :param destination: Directory to create for the clone + :return: Path to the root of the new clone + + """ + clone_path = destination / source_repository.name + _ = _git_check_output( + [ + "clone", + "--quiet", + str(source_repository), + str(clone_path), + ], + Path("."), + ) + if revision != "HEAD": + _ = _git_check_output(["checkout", revision], clone_path) + return clone_path + + +def git_get_root(path: Path) -> Optional[Path]: + """Get the root directory of a local Git repository clone based on a path inside it + + :param path: A file or directory path inside the Git repository clone + :return: The root of the clone, or ``None`` if none could be found + + """ + try: + return Path( + _git_check_output( + ["rev-parse", "--show-toplevel"], + cwd=path if path.is_dir() else path.parent, + encoding="utf-8", + exit_on_error=False, + ).rstrip() + ) + except CalledProcessError as exc_info: + if exc_info.returncode == 128 and exc_info.stderr.splitlines()[0].startswith( + "fatal: not a git repository (or any " + ): + # The error string differs a bit in different Git versions, but up to the + # point above it's identical in recent versions. + return None + sys.stderr.write(exc_info.stderr) + raise + + def _revision_vs_lines( root: Path, path_in_repo: Path, rev1: str, content: TextDocument, context_lines: int ) -> List[int]: diff --git a/src/darker/linting.py b/src/darker/linting.py index cd111a8cf..93f129430 100644 --- a/src/darker/linting.py +++ b/src/darker/linting.py @@ -21,18 +21,104 @@ import logging import shlex +from collections import defaultdict from contextlib import contextmanager +from dataclasses import dataclass from pathlib import Path from subprocess import PIPE, Popen # nosec -from typing import IO, Generator, List, Set, Tuple - -from darker.git import WORKTREE, EditedLinenumsDiffer, RevisionRange, shlex_join +from tempfile import TemporaryDirectory +from typing import IO, Collection, Dict, Generator, Iterable, List, Set, Tuple, Union + +from darker.diff import map_unmodified_lines +from darker.git import ( + WORKTREE, + RevisionRange, + git_clone_local, + git_get_content_at_revision, + git_get_root, + shlex_join, +) from darker.highlighting import colorize -from darker.utils import WINDOWS +from darker.utils import WINDOWS, fix_py37_win_tempdir_permissions logger = logging.getLogger(__name__) +@dataclass(eq=True, frozen=True, order=True) +class MessageLocation: + """A file path, line number and column number for a linter message + + Line and column numbers a 0-based, and zero is used for an unspecified column, and + for the non-specified location. + + """ + + path: Path + line: int + column: int = 0 + + def __str__(self) -> str: + """Convert file path, line and column into a linter line prefix string + + :return: Either ``"path:line:column"`` or ``"path:line"`` (if column is zero) + + """ + if self.column: + return f"{self.path}:{self.line}:{self.column}" + return f"{self.path}:{self.line}" + + +NO_MESSAGE_LOCATION = MessageLocation(Path(""), 0, 0) + + +@dataclass +class LinterMessage: + """Information about a linter message""" + + linter: str + description: str + + +class DiffLineMapping: + """A mapping from unmodified lines in new and old versions of files""" + + def __init__(self) -> None: + self._mapping: Dict[Tuple[Path, int], Tuple[Path, int]] = {} + + def __setitem__( + self, new_location: MessageLocation, old_location: MessageLocation + ) -> None: + """Add a pointer from new to old line to the mapping + + :param new_location: The file path and linenum of the message in the new version + :param old_location: The file path and linenum of the message in the old version + + """ + self._mapping[new_location.path, new_location.line] = ( + old_location.path, + old_location.line, + ) + + def get(self, new_location: MessageLocation) -> MessageLocation: + """Get the old location of the message based on the mapping + + The mapping is between line numbers, so the column number of the message in the + new file is injected into the corresponding location in the old version of the + file. + + :param new_location: The path, line and column number of a linter message in the + new version of a file + :return: The path, line and column number of the same message in the old version + of the file + + """ + key = (new_location.path, new_location.line) + if key in self._mapping: + (old_path, old_line) = self._mapping[key] + return MessageLocation(old_path, old_line, new_location.column) + return NO_MESSAGE_LOCATION + + def _strict_nonneg_int(text: str) -> int: """Strict parsing of strings to non-negative integers @@ -48,11 +134,13 @@ def _strict_nonneg_int(text: str) -> int: return int(text) -def _parse_linter_line(line: str, root: Path) -> Tuple[Path, int, str, str]: +def _parse_linter_line( + linter: str, line: str, cwd: Path +) -> Tuple[MessageLocation, LinterMessage]: """Parse one line of linter output Only parses lines with - - a file path (without leading-trailing whitespace), + - a relative or absolute file path (without leading-trailing whitespace), - a non-negative line number (without leading/trailing whitespace), - optionally a column number (without leading/trailing whitespace), and - a description. @@ -60,25 +148,25 @@ def _parse_linter_line(line: str, root: Path) -> Tuple[Path, int, str, str]: Examples of successfully parsed lines:: path/to/file.py:42: Description - path/to/file.py:42:5: Description + /absolute/path/to/file.py:42:5: Description - Given a root of ``Path("path/")``, these would be parsed into:: + Given ``cwd=Path("/absolute")``, these would be parsed into:: - (Path("to/file.py"), 42, "path/to/file.py:42:", "Description") - (Path("to/file.py"), 42, "path/to/file.py:42:5:", "Description") + (Path("path/to/file.py"), 42, "path/to/file.py:42:", "Description") + (Path("path/to/file.py"), 42, "path/to/file.py:42:5:", "Description") For all other lines, a dummy entry is returned: an empty path, zero as the line number, an empty location string and an empty description. Such lines should be simply ignored, since many linters display supplementary information insterspersed with the actual linting notifications. + :param linter: The name of the linter :param line: The linter output line to parse. May have a trailing newline. - :param root: The root directory to resolve full file paths against - :return: A 4-tuple of - - a ``root``-relative file path, - - the line number, - - the path and location string, and - - the description. + :param cwd: The directory in which the linter was run, and relative to which paths + are returned + :return: A 2-tuple of + - the file path, line and column numbers of the linter message, and + - the linter name and message description. """ try: @@ -96,26 +184,29 @@ def _parse_linter_line(line: str, root: Path) -> Tuple[Path, int, str, str]: if len(rest) > 1: raise ValueError("Too many colon-separated tokens in {location!r}") if len(rest) == 1: - # Make sure it column looks like an int on "::" - _column = _strict_nonneg_int(rest[0]) # noqa: F841 + # Make sure the column looks like an int in "::" + column = _strict_nonneg_int(rest[0]) # noqa: F841 + else: + column = 0 except ValueError: # Encountered a non-parsable line which doesn't express a linting error. # For example, on Mypy: # "Found XX errors in YY files (checked ZZ source files)" # "Success: no issues found in 1 source file" logger.debug("Unparsable linter output: %s", line[:-1]) - return Path(), 0, "", "" - path_from_cwd = Path(path_str).absolute() - try: - path_in_repo = path_from_cwd.relative_to(root) - except ValueError: - logger.warning( - "Linter message for a file %s outside requested directory %s", - path_from_cwd, - root, - ) - return Path(), 0, "", "" - return path_in_repo, linenum, location + ":", description + return (NO_MESSAGE_LOCATION, LinterMessage(linter, "")) + path = Path(path_str) + if path.is_absolute(): + try: + path = path.relative_to(cwd) + except ValueError: + logger.warning( + "Linter message for a file %s outside root directory %s", + path_str, + cwd, + ) + return (NO_MESSAGE_LOCATION, LinterMessage(linter, "")) + return (MessageLocation(path, linenum, column), LinterMessage(linter, description)) def _require_rev2_worktree(rev2: str) -> None: @@ -135,24 +226,29 @@ def _require_rev2_worktree(rev2: str) -> None: @contextmanager def _check_linter_output( - cmdline: str, root: Path, paths: Set[Path] + cmdline: Union[str, List[str]], + root: Path, + paths: Collection[Path], ) -> Generator[IO[str], None, None]: """Run a linter as a subprocess and return its standard output stream :param cmdline: The command line for running the linter :param root: The common root of all files to lint - :param paths: Paths of files to check, relative to ``git_root`` + :param paths: Paths of files to check, relative to ``root`` :return: The standard output stream of the linter subprocess """ - cmdline_and_paths = shlex.split(cmdline, posix=not WINDOWS) + [ - str(root / path) for path in sorted(paths) - ] - logger.debug("[%s]$ %s", Path.cwd(), shlex_join(cmdline_and_paths)) + if isinstance(cmdline, str): + cmdline_parts = shlex.split(cmdline, posix=not WINDOWS) + else: + cmdline_parts = cmdline + cmdline_and_paths = cmdline_parts + [str(path) for path in sorted(paths)] + logger.debug("[%s]$ %s", root, shlex_join(cmdline_and_paths)) with Popen( # nosec cmdline_and_paths, stdout=PIPE, encoding="utf-8", + cwd=root, ) as linter_process: # condition needed for MyPy (see https://stackoverflow.com/q/57350490/15770) if linter_process.stdout is None: @@ -161,9 +257,11 @@ def _check_linter_output( def run_linter( # pylint: disable=too-many-locals - cmdline: str, root: Path, paths: Set[Path], revrange: RevisionRange, use_color: bool -) -> int: - """Run the given linter and print linting errors falling on changed lines + cmdline: Union[str, List[str]], + root: Path, + paths: Collection[Path], +) -> Dict[MessageLocation, LinterMessage]: + """Run the given linter and return linting errors falling on changed lines :param cmdline: The command line for running the linter :param root: The common root of all files to lint @@ -172,76 +270,189 @@ def run_linter( # pylint: disable=too-many-locals :return: The number of modified lines with linting errors from this linter """ - _require_rev2_worktree(revrange.rev2) - if not paths: - return 0 - error_count = 0 - edited_linenums_differ = EditedLinenumsDiffer(root, revrange) missing_files = set() + result = {} + if isinstance(cmdline, str): + linter = shlex.split(cmdline, posix=not WINDOWS)[0] + cmdline_str = cmdline + else: + linter = cmdline[0] + cmdline_str = shlex_join(cmdline) with _check_linter_output(cmdline, root, paths) as linter_stdout: - prev_path, prev_linenum = None, 0 for line in linter_stdout: - ( - path_in_repo, - linter_error_linenum, - location, - description, - ) = _parse_linter_line(line, root) - if ( - path_in_repo is None - or path_in_repo in missing_files - or linter_error_linenum == 0 - ): + (location, message) = _parse_linter_line(linter, line, root) + if location is NO_MESSAGE_LOCATION or location.path in missing_files: continue - if path_in_repo.suffix != ".py": + if location.path.suffix != ".py": logger.warning( - "Linter message for a non-Python file: %s %s", + "Linter message for a non-Python file: %s: %s", location, - description, + message.description, ) continue - try: - edited_linenums = edited_linenums_differ.compare_revisions( - path_in_repo, context_lines=0 - ) - except FileNotFoundError: - logger.warning("Missing file %s from %s", path_in_repo, cmdline) - missing_files.add(path_in_repo) + if not location.path.is_file() and not location.path.is_symlink(): + logger.warning("Missing file %s from %s", location.path, cmdline_str) + missing_files.add(location.path) continue - if linter_error_linenum in edited_linenums: - if path_in_repo != prev_path or linter_error_linenum > prev_linenum + 1: - print() - prev_path, prev_linenum = path_in_repo, linter_error_linenum - print(colorize(location, "lint_location", use_color), end=" ") - print(colorize(description, "lint_description", use_color)) - error_count += 1 - return error_count + result[location] = message + return result def run_linters( - linter_cmdlines: List[str], + linter_cmdlines: List[Union[str, List[str]]], root: Path, paths: Set[Path], revrange: RevisionRange, use_color: bool, ) -> int: - """Run the given linters on a set of files in the repository + """Run the given linters on a set of files in the repository, filter messages + + Linter message filtering works by + + - running linters once in ``rev1`` to establish a baseline, + - running them again in ``rev2`` to get linter messages after user changes, and + - printing out only new messages which were not present in the baseline. + + If the source tree is not a Git repository, a baseline is not used, and all linter + messages are printed :param linter_cmdlines: The command lines for linter tools to run on the files :param root: The root of the relative paths - :param paths: The files to check, relative to ``root``. This should only include - files which have been modified in the repository between the given Git - revisions. + :param paths: The files and directories to check, relative to ``root`` :param revrange: The Git revisions to compare + :param use_color: ``True`` to use syntax highlighting for linter output :return: Total number of linting errors found on modified lines """ - # 10. run linter subprocesses for all edited files (10.-13. optional) - # 11. diff the given revision and worktree (after isort and Black reformatting) - # for each file reported by a linter - # 12. extract line numbers in each file reported by a linter for changed lines - # 13. print only linter error lines which fall on changed lines - return sum( - run_linter(linter_cmdline, root, paths, revrange, use_color) - for linter_cmdline in linter_cmdlines + if not linter_cmdlines: + return 0 + _require_rev2_worktree(revrange.rev2) + git_root = git_get_root(root) + if not git_root: + # In a non-Git root, don't use a baseline + messages = _get_messages_from_linters( + linter_cmdlines, + root, + paths, + ) + return _print_new_linter_messages( + baseline={}, + new_messages=messages, + diff_line_mapping=DiffLineMapping(), + use_color=use_color, + ) + git_paths = {(root / path).relative_to(git_root) for path in paths} + baseline = _get_messages_from_linters_for_baseline( + linter_cmdlines, git_root, git_paths, revrange.rev1 ) + messages = _get_messages_from_linters(linter_cmdlines, git_root, git_paths) + files_with_messages = {location.path for location in messages} + diff_line_mapping = _create_line_mapping(git_root, files_with_messages, revrange) + return _print_new_linter_messages(baseline, messages, diff_line_mapping, use_color) + + +def _get_messages_from_linters( + linter_cmdlines: Iterable[Union[str, List[str]]], + root: Path, + paths: Collection[Path], +) -> Dict[MessageLocation, List[LinterMessage]]: + """Run given linters for the given directory and return linting errors + + :param linter_cmdlines: The command lines for running the linters + :param root: The common root of all files to lint + :param paths: Paths of files to check, relative to ``root`` + :param revrange: The Git revision rango to compare + :return: Linter messages + + """ + result = defaultdict(list) + for cmdline in linter_cmdlines: + for message_location, message in run_linter(cmdline, root, paths).items(): + result[message_location].append(message) + return result + + +def _print_new_linter_messages( + baseline: Dict[MessageLocation, List[LinterMessage]], + new_messages: Dict[MessageLocation, List[LinterMessage]], + diff_line_mapping: DiffLineMapping, + use_color: bool, +) -> int: + """Print all linter messages except those same as before on unmodified lines + + :param baseline: Linter messages and their locations for a previous version + :param new_messages: New linter messages in a new version of the source file + :param diff_line_mapping: Mapping between unmodified lines in old and new versions + :param use_color: ``True`` to highlight linter messages in the output + :return: The number of linter errors displayed + + """ + error_count = 0 + prev_location = NO_MESSAGE_LOCATION + for message_location, messages in sorted(new_messages.items()): + old_location = diff_line_mapping.get(message_location) + is_modified_line = old_location == NO_MESSAGE_LOCATION + old_messages: List[LinterMessage] = baseline.get(old_location, []) + for message in messages: + if not is_modified_line and message in old_messages: + # Only hide messages when + # - they occurred previously on the corresponding line + # - the line hasn't been modified + continue + if ( + message_location.path != prev_location.path + or message_location.line > prev_location.line + 1 + ): + print() + prev_location = message_location + print(colorize(f"{message_location}:", "lint_location", use_color), end=" ") + print(colorize(message.description, "lint_description", use_color), end=" ") + print(f"[{message.linter}]") + error_count += 1 + return error_count + + +def _get_messages_from_linters_for_baseline( + linter_cmdlines: List[Union[str, List[str]]], + root: Path, + paths: Collection[Path], + revision: str, +) -> Dict[MessageLocation, List[LinterMessage]]: + """Clone the Git repository at a given revision and run linters against it + + :param linter_cmdlines: The command lines for linter tools to run on the files + :param root: The root of the Git repository + :param paths: The files and directories to check, relative to ``root`` + :param revision: The revision to check out + :return: Linter messages + + """ + with TemporaryDirectory() as tmp_path: + clone_root = git_clone_local(root, revision, Path(tmp_path)) + result = _get_messages_from_linters(linter_cmdlines, clone_root, paths) + fix_py37_win_tempdir_permissions(tmp_path) + return result + + +def _create_line_mapping( + root: Path, files_with_messages: Iterable[Path], revrange: RevisionRange +) -> DiffLineMapping: + """Create a mapping from unmodified lines in new files to same lines in old versions + + :param root: The root of the repository + :param files_with_messages: Paths to files which have linter messages + :param revrange: The revisions to compare + :return: A dict which maps the line number of each unmodified line in the new + versions of files to corresponding line numbers in old versions of the same + files + + """ + diff_line_mapping = DiffLineMapping() + for path in files_with_messages: + doc1 = git_get_content_at_revision(path, revrange.rev1, root) + doc2 = git_get_content_at_revision(path, revrange.rev2, root) + for linenum2, linenum1 in map_unmodified_lines(doc1, doc2).items(): + location1 = MessageLocation(path, linenum1) + location2 = MessageLocation(path, linenum2) + diff_line_mapping[location2] = location1 + return diff_line_mapping diff --git a/src/darker/tests/conftest.py b/src/darker/tests/conftest.py index be830b3ae..2760ec90d 100644 --- a/src/darker/tests/conftest.py +++ b/src/darker/tests/conftest.py @@ -101,7 +101,11 @@ def expand_root(self, lines: Iterable[str]) -> List[str]: """ return [ - re.sub(r"\{root/(.*?)\}", lambda m: str(self.root / str(m.group(1))), line) + re.sub( + r"\{root(/.*?)?\}", + lambda m: str(self.root / (str(m.group(1)[1:]) if m.group(1) else "")), + line, + ) for line in lines ] diff --git a/src/darker/tests/test_black_diff.py b/src/darker/tests/test_black_diff.py index 1e5185669..04b9014f0 100644 --- a/src/darker/tests/test_black_diff.py +++ b/src/darker/tests/test_black_diff.py @@ -201,9 +201,7 @@ def test_filter_python_files( # pylint: disable=too-many-arguments result = filter_python_files({Path(".")} | explicit, tmp_path, black_config) - expect_paths = {tmp_path / f"{path}.py" for path in expect} | { - tmp_path / p for p in explicit - } + expect_paths = {Path(f"{path}.py") for path in expect} | explicit assert result == expect_paths diff --git a/src/darker/tests/test_diff.py b/src/darker/tests/test_diff.py index 20cbd326f..5c0bdbe5e 100644 --- a/src/darker/tests/test_diff.py +++ b/src/darker/tests/test_diff.py @@ -7,6 +7,7 @@ from darker.diff import ( diff_and_get_opcodes, + map_unmodified_lines, opcodes_to_chunks, opcodes_to_edit_linenums, ) @@ -266,3 +267,38 @@ def test_opcodes_to_edit_linenums_empty_opcodes(): ) assert result == [] # pylint: disable=use-implicit-booleaness-not-comparison + + +@pytest.mark.kwparametrize( + dict( + expect={1: 1}, + ), + dict( + lines2=["file", "was", "empty", "but", "eventually", "not"], + expect={}, + ), + dict( + lines1=["file", "had", "content", "but", "becomes", "empty"], + expect={}, + ), + dict( + lines1=["1 unmoved", "2 modify", "3 to 4 moved"], + lines2=["1 unmoved", "2 modified", "3 inserted", "3 to 4 moved"], + expect={1: 1, 4: 3}, + ), + dict( + lines1=["can't", "follow", "both", "when", "order", "is", "changed"], + lines2=["when", "order", "is", "changed", "can't", "follow", "both"], + expect={1: 4, 2: 5, 3: 6, 4: 7}, + ), + lines1=[], + lines2=[], +) +def test_map_unmodified_lines(lines1, lines2, expect): + """``map_unmodified_lines`` returns a 1-based mapping from new to old linenums""" + doc1 = TextDocument.from_lines(lines1) + doc2 = TextDocument.from_lines(lines2) + + result = map_unmodified_lines(doc1, doc2) + + assert result == expect diff --git a/src/darker/tests/test_git.py b/src/darker/tests/test_git.py index 8956baa25..25c45668c 100644 --- a/src/darker/tests/test_git.py +++ b/src/darker/tests/test_git.py @@ -1,6 +1,7 @@ """Unit tests for :mod:`darker.git`""" # pylint: disable=redefined-outer-name,protected-access,too-many-arguments +# pylint: disable=too-many-lines import os import re @@ -9,7 +10,7 @@ from subprocess import DEVNULL, PIPE, CalledProcessError, check_call # nosec from textwrap import dedent # nosec from typing import List, Union -from unittest.mock import call, patch +from unittest.mock import ANY, Mock, call, patch import pytest @@ -787,6 +788,112 @@ def test_git_get_modified_python_files_revision_range( assert {path.name for path in result} == expect +@pytest.mark.kwparametrize( + dict(branch="first", expect="first"), + dict(branch="second", expect="second"), + dict(branch="third", expect="third"), + dict(branch="HEAD", expect="third"), +) +def test_git_clone_local_branch(git_repo, tmp_path, branch, expect): + """``git_clone_local()`` checks out the specified branch""" + git_repo.add({"a.py": "first"}, commit="first") + git_repo.create_branch("first", "HEAD") + git_repo.create_branch("second", "HEAD") + git_repo.add({"a.py": "second"}, commit="second") + git_repo.create_branch("third", "HEAD") + git_repo.add({"a.py": "third"}, commit="third") + + clone = git.git_clone_local(git_repo.root, branch, tmp_path) + + assert (clone / "a.py").read_text() == expect + + +@pytest.mark.kwparametrize( + dict(branch="HEAD", expect_checkout_call=False), + dict(branch="mybranch", expect_checkout_call=True), +) +def test_git_clone_local_command(git_repo, tmp_path, branch, expect_checkout_call): + """``git_clone_local()`` issues the correct Git command and options""" + git_repo.add({"a.py": "first"}, commit="first") + git_repo.create_branch("mybranch", "HEAD") + check_output = Mock(wraps=git.check_output) # type: ignore[attr-defined] + with patch.object(git, "check_output", check_output): + + clone = git.git_clone_local(git_repo.root, branch, tmp_path) + + assert clone == tmp_path / git_repo.root.name + expect_calls = [ + call( + ["git", "clone", "--quiet", str(git_repo.root), str(clone)], + cwd=".", + encoding=None, + stderr=PIPE, + env=ANY, + ) + ] + if expect_checkout_call: + expect_calls.append( + call( + ["git", "checkout", branch], + cwd=str(git_repo.root / git_repo.root.name), + encoding=None, + stderr=PIPE, + env=ANY, + ) + ) + check_output.assert_has_calls(expect_calls) + + +@pytest.mark.parametrize( + "path", + [ + ".", + "root.py", + "subdir", + "subdir/sub.py", + "subdir/subsubdir", + "subdir/subsubdir/subsub.py", + ], +) +def test_git_get_root(git_repo, path): + """``git_get_root()`` returns repository root for any file or directory inside""" + git_repo.add( + { + "root.py": "root", + "subdir/sub.py": "sub", + "subdir/subsubdir/subsub.py": "subsub", + }, + commit="Initial commit", + ) + + root = git.git_get_root(git_repo.root / path) + + assert root == git_repo.root + + +@pytest.mark.parametrize( + "path", + [ + ".", + "root.py", + "subdir", + "subdir/sub.py", + "subdir/subsubdir", + "subdir/subsubdir/subsub.py", + ], +) +def test_git_get_root_not_found(tmp_path, path): + """``git_get_root()`` returns ``None`` for any file or directory outside of Git""" + (tmp_path / "subdir" / "subsubdir").mkdir(parents=True) + (tmp_path / "root.py").touch() + (tmp_path / "subdir" / "sub.py").touch() + (tmp_path / "subdir" / "subsubdir" / "subsub.py").touch() + + root = git.git_get_root(tmp_path / path) + + assert root is None + + @pytest.mark.kwparametrize( dict( environ={}, diff --git a/src/darker/tests/test_linting.py b/src/darker/tests/test_linting.py index 4ef66d08d..8a1e189fb 100644 --- a/src/darker/tests/test_linting.py +++ b/src/darker/tests/test_linting.py @@ -5,12 +5,13 @@ import os from pathlib import Path from textwrap import dedent -from unittest.mock import call, patch +from typing import Dict, Iterable, List, Tuple, Union import pytest from darker import linting from darker.git import WORKTREE, RevisionRange +from darker.linting import DiffLineMapping, LinterMessage, MessageLocation from darker.tests.helpers import raises_if_exception from darker.utils import WINDOWS @@ -18,63 +19,111 @@ SKIP_ON_UNIX = [] if WINDOWS else [pytest.mark.skip] +@pytest.mark.kwparametrize( + dict(column=0, expect=f"{Path('/path/to/file.py')}:42"), + dict(column=5, expect=f"{Path('/path/to/file.py')}:42:5"), +) +def test_message_location_str(column, expect): + """Null column number is hidden from string representation of message location""" + location = MessageLocation(Path("/path/to/file.py"), 42, column) + + result = str(location) + + assert result == expect + + +@pytest.mark.kwparametrize( + dict( + new_location=("/path/to/new_file.py", 43, 8), + old_location=("/path/to/old_file.py", 42, 13), + get_location=("/path/to/new_file.py", 43, 21), + expect_location=("/path/to/old_file.py", 42, 21), + ), + dict( + new_location=("/path/to/new_file.py", 43, 8), + old_location=("/path/to/old_file.py", 42, 13), + get_location=("/path/to/a_different_file.py", 43, 21), + expect_location=("", 0, 0), + ), + dict( + new_location=("/path/to/file.py", 43, 8), + old_location=("/path/to/file.py", 42, 13), + get_location=("/path/to/file.py", 42, 21), + expect_location=("", 0, 0), + ), +) +def test_diff_line_mapping_ignores_column( + new_location, old_location, get_location, expect_location +): + """Diff location mapping ignores column and attaches column of queried location""" + mapping = linting.DiffLineMapping() + new_location_ = MessageLocation(Path(new_location[0]), *new_location[1:]) + old_location = MessageLocation(Path(old_location[0]), *old_location[1:]) + get_location = MessageLocation(Path(get_location[0]), *get_location[1:]) + expect = MessageLocation(Path(expect_location[0]), *expect_location[1:]) + + mapping[new_location_] = old_location + result = mapping.get(get_location) + + assert result == expect + + @pytest.mark.kwparametrize( dict( line="module.py:42: Just a line number\n", - expect=(Path("module.py"), 42, "module.py:42:", "Just a line number"), + expect=(Path("module.py"), 42, 0, "Just a line number"), ), dict( line="module.py:42:5: With column \n", - expect=(Path("module.py"), 42, "module.py:42:5:", "With column"), + expect=(Path("module.py"), 42, 5, "With column"), ), dict( line="{git_root_absolute}{sep}mod.py:42: Full path\n", - expect=( - Path("mod.py"), - 42, - "{git_root_absolute}{sep}mod.py:42:", - "Full path", - ), + expect=(Path("mod.py"), 42, 0, "Full path"), ), dict( line="{git_root_absolute}{sep}mod.py:42:5: Full path with column\n", - expect=( - Path("mod.py"), - 42, - "{git_root_absolute}{sep}mod.py:42:5:", - "Full path with column", - ), + expect=(Path("mod.py"), 42, 5, "Full path with column"), ), dict( line="mod.py:42: 123 digits start the description\n", - expect=(Path("mod.py"), 42, "mod.py:42:", "123 digits start the description"), + expect=(Path("mod.py"), 42, 0, "123 digits start the description"), ), dict( line="mod.py:42: indented description\n", - expect=(Path("mod.py"), 42, "mod.py:42:", " indented description"), + expect=(Path("mod.py"), 42, 0, " indented description"), ), dict( line="mod.py:42:5: indented description\n", - expect=(Path("mod.py"), 42, "mod.py:42:5:", " indented description"), + expect=(Path("mod.py"), 42, 5, " indented description"), ), dict( line="nonpython.txt:5: Non-Python file\n", - expect=(Path("nonpython.txt"), 5, "nonpython.txt:5:", "Non-Python file"), + expect=(Path("nonpython.txt"), 5, 0, "Non-Python file"), + ), + dict(line="mod.py: No line number\n", expect=(Path(), 0, 0, "")), + dict(line="mod.py:foo:5: Invalid line number\n", expect=(Path(), 0, 0, "")), + dict(line="mod.py:42:bar: Invalid column\n", expect=(Path(), 0, 0, "")), + dict( + line="/outside/mod.py:5: Outside the repo\n", + expect=(Path(), 0, 0, ""), + marks=SKIP_ON_WINDOWS, + ), + dict( + line="C:\\outside\\mod.py:5: Outside the repo\n", + expect=(Path(), 0, 0, ""), + marks=SKIP_ON_UNIX, ), - dict(line="mod.py: No line number\n", expect=(Path(), 0, "", "")), - dict(line="mod.py:foo:5: Invalid line number\n", expect=(Path(), 0, "", "")), - dict(line="mod.py:42:bar: Invalid column\n", expect=(Path(), 0, "", "")), - dict(line="/outside/mod.py:5: Outside the repo\n", expect=(Path(), 0, "", "")), - dict(line="invalid linter output\n", expect=(Path(), 0, "", "")), - dict(line=" leading:42: whitespace\n", expect=(Path(), 0, "", "")), - dict(line=" leading:42:5 whitespace and column\n", expect=(Path(), 0, "", "")), - dict(line="trailing :42: filepath whitespace\n", expect=(Path(), 0, "", "")), - dict(line="leading: 42: linenum whitespace\n", expect=(Path(), 0, "", "")), - dict(line="trailing:42 : linenum whitespace\n", expect=(Path(), 0, "", "")), - dict(line="plus:+42: before linenum\n", expect=(Path(), 0, "", "")), - dict(line="minus:-42: before linenum\n", expect=(Path(), 0, "", "")), - dict(line="plus:42:+5 before column\n", expect=(Path(), 0, "", "")), - dict(line="minus:42:-5 before column\n", expect=(Path(), 0, "", "")), + dict(line="invalid linter output\n", expect=(Path(), 0, 0, "")), + dict(line=" leading:42: whitespace\n", expect=(Path(), 0, 0, "")), + dict(line=" leading:42:5 whitespace and column\n", expect=(Path(), 0, 0, "")), + dict(line="trailing :42: filepath whitespace\n", expect=(Path(), 0, 0, "")), + dict(line="leading: 42: linenum whitespace\n", expect=(Path(), 0, 0, "")), + dict(line="trailing:42 : linenum whitespace\n", expect=(Path(), 0, 0, "")), + dict(line="plus:+42: before linenum\n", expect=(Path(), 0, 0, "")), + dict(line="minus:-42: before linenum\n", expect=(Path(), 0, 0, "")), + dict(line="plus:42:+5 before column\n", expect=(Path(), 0, 0, "")), + dict(line="minus:42:-5 before column\n", expect=(Path(), 0, 0, "")), ) def test_parse_linter_line(git_repo, monkeypatch, line, expect): """Linter output is parsed correctly""" @@ -82,15 +131,9 @@ def test_parse_linter_line(git_repo, monkeypatch, line, expect): root_abs = git_repo.root.absolute() line_expanded = line.format(git_root_absolute=root_abs, sep=os.sep) - result = linting._parse_linter_line(line_expanded, git_repo.root) + result = linting._parse_linter_line("linter", line_expanded, git_repo.root) - expect_expanded = ( - expect[0], - expect[1], - expect[2].format(git_root_absolute=root_abs, sep=os.sep), - expect[3], - ) - assert result == expect_expanded + assert result == (MessageLocation(*expect[:3]), LinterMessage("linter", expect[3])) @pytest.mark.kwparametrize( @@ -123,127 +166,104 @@ def test_require_rev2_worktree(rev2, expect): ), dict(cmdline="echo eat spaces", expect=["eat spaces first.py the 2nd.py\n"]), ) -def test_check_linter_output(cmdline, expect): +def test_check_linter_output(tmp_path, cmdline, expect): """``_check_linter_output()`` runs linter and returns the stdout stream""" with linting._check_linter_output( - cmdline, Path("root/of/repo"), {Path("first.py"), Path("the 2nd.py")} + cmdline, tmp_path, {Path("first.py"), Path("the 2nd.py")} ) as stdout: lines = list(stdout) - assert lines == [ - line.replace("first.py", str(Path("root/of/repo/first.py"))).replace( - "the 2nd.py", str(Path("root/of/repo/the 2nd.py")) - ) - for line in expect - ] + assert lines == expect @pytest.mark.kwparametrize( dict( - _descr="No files to check, no output", - paths=[], - location="test.py:1:", - expect_output=[], - expect_log=[], - ), - dict( - _descr="Check one file, report on a modified line in test.py", - paths=["one.py"], - location="test.py:1:", - expect_output=["", "test.py:1: {root/one.py}"], - expect_log=[], + _descr="New message for test.py", + messages_after=["test.py:1: new message"], + expect_output=["", "test.py:1: new message [cat]"], ), dict( - _descr="Check one file, report on a column of a modified line in test.py", - paths=["one.py"], - location="test.py:1:42:", - expect_output=["", "test.py:1:42: {root/one.py}"], - expect_log=[], + _descr="New message for test.py, including column number", + messages_after=["test.py:1:42: new message with column number"], + expect_output=["", "test.py:1:42: new message with column number [cat]"], ), dict( - _descr="No output if report is on an unmodified line in test.py", - paths=["one.py"], - location="test.py:2:42:", - expect_output=[], - expect_log=[], + _descr="Identical message on an unmodified unmoved line in test.py", + messages_before=["test.py:1:42: same message on same line"], + messages_after=["test.py:1:42: same message on same line"], ), dict( - _descr="No output if report is on a column of an unmodified line in test.py", - paths=["one.py"], - location="test.py:2:42:", - expect_output=[], - expect_log=[], + _descr="Identical message on an unmodified moved line in test.py", + messages_before=["test.py:3:42: same message on a moved line"], + messages_after=["test.py:4:42: same message on a moved line"], ), dict( - _descr="Check two files, report on a modified line in test.py", - paths=["one.py", "two.py"], - location="test.py:1:", - expect_output=["", "test.py:1: {root/one.py} {root/two.py}"], - expect_log=[], - ), - dict( - _descr="Check two files, rpeort on a column of a modified line in test.py", - paths=["one.py", "two.py"], - location="test.py:1:42:", - expect_output=["", "test.py:1:42: {root/one.py} {root/two.py}"], - expect_log=[], + _descr="Additional message on an unmodified moved line in test.py", + messages_before=["test.py:3:42: same message"], + messages_after=[ + "test.py:4:42: same message", + "test.py:4:42: additional message", + ], + expect_output=["", "test.py:4:42: additional message [cat]"], ), dict( - _descr="No output if 2-file report is on an unmodified line in test.py", - paths=["one.py", "two.py"], - location="test.py:2:", - expect_output=[], - expect_log=[], + _descr="Changed message on an unmodified moved line in test.py", + messages_before=["test.py:4:42: old message"], + messages_after=["test.py:4:42: new message"], + expect_output=["", "test.py:4:42: new message [cat]"], ), dict( - _descr="No output if 2-file report is on a column of an unmodified line", - paths=["one.py", "two.py"], - location="test.py:2:42:", - expect_output=[], - expect_log=[], + _descr="Identical message but on an inserted line in test.py", + messages_before=["test.py:1:42: same message also on an inserted line"], + messages_after=[ + "test.py:1:42: same message also on an inserted line", + "test.py:2:42: same message also on an inserted line", + ], + expect_output=["", "test.py:2:42: same message also on an inserted line [cat]"], ), dict( _descr="Warning for a file missing from the working tree", - paths=["missing.py"], - location="missing.py:1:", - expect_output=[], - expect_log=["WARNING Missing file missing.py from echo missing.py:1:"], + messages_after=["missing.py:1: a missing Python file"], + expect_log=["WARNING Missing file missing.py from cat messages"], ), dict( _descr="Linter message for a non-Python file is ignored with a warning", - paths=["one.py"], - location="nonpython.txt:1:", - expect_output=[], + messages_after=["nonpython.txt:1: non-py"], expect_log=[ - "WARNING Linter message for a non-Python file: " - "nonpython.txt:1: {root/one.py}" + "WARNING Linter message for a non-Python file: nonpython.txt:1: non-py" ], ), dict( _descr="Message for file outside common root is ignored with a warning (Unix)", - paths=["one.py"], - location="/elsewhere/mod.py:1:", - expect_output=[], + messages_after=["/elsewhere/mod.py:1: elsewhere"], expect_log=[ - "WARNING Linter message for a file /elsewhere/mod.py " - "outside requested directory {root/}" + "WARNING Linter message for a file /elsewhere/mod.py outside root" + " directory {root}" ], marks=SKIP_ON_WINDOWS, ), dict( _descr="Message for file outside common root is ignored with a warning (Win)", - paths=["one.py"], - location="C:\\elsewhere\\mod.py:1:", - expect_output=[], + messages_after=["C:\\elsewhere\\mod.py:1: elsewhere"], expect_log=[ - "WARNING Linter message for a file C:\\elsewhere\\mod.py " - "outside requested directory {root/}" + "WARNING Linter message for a file C:\\elsewhere\\mod.py outside root" + " directory {root}" ], marks=SKIP_ON_UNIX, ), + messages_before=[], + expect_output=[], + expect_log=[], ) -def test_run_linter( - git_repo, capsys, caplog, _descr, paths, location, expect_output, expect_log +def test_run_linters( + git_repo, + capsys, + caplog, + _descr, + messages_before, + messages_after, + expect_output, + expect_log, ): """Linter gets correct paths on command line and outputs just changed lines @@ -259,14 +279,22 @@ def test_run_linter( """ src_paths = git_repo.add( - {"test.py": "1\n2\n", "nonpython.txt": "hello\n"}, commit="Initial commit" + { + "test.py": "1 unmoved\n2 modify\n3 to 4 moved\n", + "nonpython.txt": "hello\n", + "messages": "\n".join(messages_before), + }, + commit="Initial commit", ) - src_paths["test.py"].write_bytes(b"one\n2\n") - cmdline = f"echo {location}" + src_paths["test.py"].write_bytes( + b"1 unmoved\n2 modified\n3 inserted\n3 to 4 moved\n" + ) + src_paths["messages"].write_text("\n".join(messages_after)) + cmdlines: List[Union[str, List[str]]] = ["cat messages"] revrange = RevisionRange("HEAD", ":WORKTREE:") - linting.run_linter( - cmdline, git_repo.root, {Path(p) for p in paths}, revrange, use_color=False + linting.run_linters( + cmdlines, git_repo.root, {Path("dummy path")}, revrange, use_color=False ) # We can now verify that the linter received the correct paths on its command line @@ -282,12 +310,12 @@ def test_run_linter( assert logs == git_repo.expand_root(expect_log) -def test_run_linter_non_worktree(): - """``run_linter()`` doesn't support linting commits, only the worktree""" +def test_run_linters_non_worktree(): + """``run_linters()`` doesn't support linting commits, only the worktree""" with pytest.raises(NotImplementedError): - linting.run_linter( - "dummy-linter", + linting.run_linters( + ["dummy-linter"], Path("/dummy"), {Path("dummy.py")}, RevisionRange.parse_with_common_ancestor("..HEAD", Path("dummy cwd")), @@ -296,21 +324,21 @@ def test_run_linter_non_worktree(): @pytest.mark.parametrize( - "location, expect", + "message, expect", [ ("", 0), - ("test.py:1:", 1), - ("test.py:2:", 0), + ("test.py:1: message on modified line", 1), + ("test.py:2: message on unmodified line", 0), ], ) -def test_run_linter_return_value(git_repo, location, expect): - """``run_linter()`` returns the number of linter errors on modified lines""" +def test_run_linters_return_value(git_repo, message, expect): + """``run_linters()`` returns the number of linter errors on modified lines""" src_paths = git_repo.add({"test.py": "1\n2\n"}, commit="Initial commit") src_paths["test.py"].write_bytes(b"one\n2\n") - cmdline = f"echo {location}" + cmdline = f"echo {message}" - result = linting.run_linter( - cmdline, + result = linting.run_linters( + [cmdline], git_repo.root, {Path("test.py")}, RevisionRange("HEAD", ":WORKTREE:"), @@ -320,72 +348,8 @@ def test_run_linter_return_value(git_repo, location, expect): assert result == expect -@pytest.mark.kwparametrize( - dict( - linter_cmdlines=[], - linters_return=[], - expect_result=0, - ), - dict( - linter_cmdlines=["linter"], - linters_return=[0], - expect_result=0, - ), - dict( - linter_cmdlines=["linter"], - linters_return=[1], - expect_result=1, - ), - dict( - linter_cmdlines=["linter"], - linters_return=[42], - expect_result=42, - ), - dict( - linter_cmdlines=["linter1", "linter2"], - linters_return=[0, 0], - expect_result=0, - ), - dict( - linter_cmdlines=["linter1", "linter2"], - linters_return=[0, 42], - expect_result=42, - ), - dict( - linter_cmdlines=["linter1", "linter2 command line"], - linters_return=[42, 42], - expect_result=84, - ), -) -def test_run_linters(linter_cmdlines, linters_return, expect_result): - """Unit test for ``run_linters()``""" - with patch.object(linting, "run_linter") as run_linter: - run_linter.side_effect = linters_return - - result = linting.run_linters( - linter_cmdlines, - Path("dummy root"), - {Path("dummy paths")}, - RevisionRange("dummy rev1", "dummy rev2"), - use_color=False, - ) - - expect_calls = [ - call( - linter_cmdline, - Path("dummy root"), - {Path("dummy paths")}, - RevisionRange("dummy rev1", "dummy rev2"), - False, - ) - for linter_cmdline in linter_cmdlines - ] - assert run_linter.call_args_list == expect_calls - assert result == expect_result - - -def test_run_linter_on_new_file(git_repo, capsys): - """``run_linter()`` considers file missing from history as empty +def test_run_linters_on_new_file(git_repo, capsys): + """``run_linters()`` considers file missing from history as empty Passes through all linter errors as if the original file was empty. @@ -394,8 +358,8 @@ def test_run_linter_on_new_file(git_repo, capsys): git_repo.create_tag("initial") (git_repo.root / "file2.py").write_bytes(b"1\n2\n") - linting.run_linter( - "echo file2.py:1:", + linting.run_linters( + ["echo file2.py:1: message on a file not seen in Git history"], Path(git_repo.root), {Path("file2.py")}, RevisionRange("initial", ":WORKTREE:"), @@ -403,11 +367,14 @@ def test_run_linter_on_new_file(git_repo, capsys): ) output = capsys.readouterr().out.splitlines() - assert output == ["", f"file2.py:1: {git_repo.root / 'file2.py'}"] + assert output == [ + "", + "file2.py:1: message on a file not seen in Git history file2.py [echo]", + ] -def test_run_linter_line_separation(git_repo, capsys): - """``run_linter`` separates contiguous blocks of linter output with empty lines""" +def test_run_linters_line_separation(git_repo, capsys): + """``run_linters`` separates contiguous blocks of linter output with empty lines""" paths = git_repo.add({"a.py": "1\n2\n3\n4\n5\n6\n"}, commit="Initial commit") paths["a.py"].write_bytes(b"a\nb\nc\nd\ne\nf\n") linter_output = git_repo.root / "dummy-linter-output.txt" @@ -421,22 +388,143 @@ def test_run_linter_line_separation(git_repo, capsys): """ ) ) + cat_command = "cmd /c type" if WINDOWS else "cat" - linting.run_linter( - f"cat {linter_output}", - Path(git_repo.root), + linting.run_linters( + [f"{cat_command} {linter_output}"], + git_repo.root, {Path(p) for p in paths}, RevisionRange("HEAD", ":WORKTREE:"), use_color=False, ) result = capsys.readouterr().out + cat_cmd = "cmd" if WINDOWS else "cat" assert result == dedent( + f""" + a.py:2: first block [{cat_cmd}] + a.py:3: of linter output [{cat_cmd}] + + a.py:5: second block [{cat_cmd}] + a.py:6: of linter output [{cat_cmd}] """ - a.py:2: first block - a.py:3: of linter output + ) + - a.py:5: second block - a.py:6: of linter output +def _build_messages( + lines_and_messages: Iterable[Union[Tuple[int, str], Tuple[int, str, str]]], +) -> Dict[MessageLocation, List[LinterMessage]]: + return { + MessageLocation(Path("a.py"), line, 0): [ + LinterMessage(*msg.split(":")) for msg in msgs + ] + for line, *msgs in lines_and_messages + } + + +def test_print_new_linter_messages(capsys): + """`linting._print_new_linter_messages()` hides old intact linter messages""" + baseline = _build_messages( + [ + (2, "mypy:single message on an unmodified line"), + (4, "mypy:single message on a disappearing line"), + (6, "mypy:single message on a moved line"), + (8, "mypy:single message on a modified line"), + (10, "mypy:multiple messages", "pylint:on the same moved line"), + ( + 12, + "mypy:old message which will be replaced", + "pylint:on an unmodified line", + ), + (14, "mypy:old message on a modified line"), + ] + ) + new_messages = _build_messages( + [ + (2, "mypy:single message on an unmodified line"), + (5, "mypy:single message on a moved line"), + (8, "mypy:single message on a modified line"), + (11, "mypy:multiple messages", "pylint:on the same moved line"), + ( + 12, + "mypy:new message replacing the old one", + "pylint:on an unmodified line", + ), + (14, "mypy:new message on a modified line"), + (16, "mypy:multiple messages", "pylint:on the same new line"), + ] + ) + diff_line_mapping = DiffLineMapping() + for new_line, old_line in {2: 2, 5: 6, 11: 10, 12: 12}.items(): + diff_line_mapping[MessageLocation(Path("a.py"), new_line)] = MessageLocation( + Path("a.py"), old_line + ) + + linting._print_new_linter_messages( + baseline, new_messages, diff_line_mapping, use_color=False + ) + + result = capsys.readouterr().out.splitlines() + assert result == [ + "", + "a.py:8: single message on a modified line [mypy]", + "", + "a.py:12: new message replacing the old one [mypy]", + "", + "a.py:14: new message on a modified line [mypy]", + "", + "a.py:16: multiple messages [mypy]", + "a.py:16: on the same new line [pylint]", + ] + + +LINT_EMPTY_LINES_CMD = [ + "python", + "-c", + dedent( """ + from pathlib import Path + for path in Path(".").glob("**/*.py"): + for linenum, line in enumerate(path.open(), start=1): + if not line.strip(): + print(f"{path}:{linenum}: EMPTY") + """ + ), +] + +LINT_NONEMPTY_LINES_CMD = [ + "python", + "-c", + dedent( + """ + from pathlib import Path + for path in Path(".").glob("**/*.py"): + for linenum, line in enumerate(path.open(), start=1): + if line.strip(): + print(f"{path}:{linenum}: {line.strip()}") + """ + ), +] + + +def test_get_messages_from_linters_for_baseline(git_repo): + """Test for `linting._get_messages_from_linters_for_baseline`""" + git_repo.add({"a.py": "First line\n\nThird line\n"}, commit="Initial commit") + initial = git_repo.get_hash() + git_repo.add({"a.py": "Just one line\n"}, commit="Second commit") + git_repo.create_branch("baseline", initial) + + result = linting._get_messages_from_linters_for_baseline( + linter_cmdlines=[LINT_EMPTY_LINES_CMD, LINT_NONEMPTY_LINES_CMD], + root=git_repo.root, + paths=[Path("a.py"), Path("subdir/b.py")], + revision="baseline", ) + + a_py = Path("a.py") + expect = { + MessageLocation(a_py, 1): [LinterMessage("python", "First line")], + MessageLocation(a_py, 2): [LinterMessage("python", "EMPTY")], + MessageLocation(a_py, 3): [LinterMessage("python", "Third line")], + } + assert result == expect diff --git a/src/darker/tests/test_main.py b/src/darker/tests/test_main.py index b42cda82c..01a341712 100644 --- a/src/darker/tests/test_main.py +++ b/src/darker/tests/test_main.py @@ -10,7 +10,7 @@ from pathlib import Path from textwrap import dedent from types import SimpleNamespace -from unittest.mock import call, patch +from unittest.mock import ANY, call, patch import pytest @@ -422,14 +422,14 @@ def test_blacken_single_file( dict( arguments=["--check", "--lint", "echo subdir/a.py:1: message"], # Windows compatible path assertion using `pathlib.Path()` - expect_stdout=["", f"subdir/a.py:1: message {Path('/subdir/a.py')}"], + expect_stdout=["", f"{Path('subdir/a.py')}:1: message {Path('subdir')} [echo]"], expect_retval=1, ), dict( arguments=["--diff", "--lint", "echo subdir/a.py:1: message"], # Windows compatible path assertion using `pathlib.Path()` expect_stdout=A_PY_DIFF_BLACK - + ["", f"subdir/a.py:1: message {Path('/subdir/a.py')}"], + + ["", f"{Path('subdir/a.py')}:1: message {Path('subdir')} [echo]"], expect_retval=1, ), dict( @@ -559,21 +559,18 @@ def test_main_in_plain_directory(tmp_path, capsys): (subdir_a / "non-python file.txt").write_text("not reformatted\n") (subdir_a / "python file.py").write_text("import sys, os\nprint('ok')") (subdir_c / "another python file.py").write_text("a =5") - with patch.object(darker.linting, "run_linter") as run_linter: + with patch.object(darker.__main__, "run_linters") as run_linters: retval = darker.__main__.main( - ["--diff", "--check", "--isort", "--lint", "my-linter", str(tmp_path)] + ["--diff", "--check", "--isort", "--lint", "echo", str(tmp_path)] ) assert retval == 1 - assert run_linter.call_args_list == [ + assert run_linters.call_args_list == [ call( - "my-linter", + ["echo"], tmp_path, - { - Path("subdir_a/python file.py"), - Path("subdir_b/subdir_c/another python file.py"), - }, + {Path(".")}, RevisionRange(rev1="HEAD", rev2=":WORKTREE:"), False, ) @@ -709,6 +706,20 @@ def test_main_vscode_tmpfile(git_repo, capsys): ] +def test_main_lint_unchanged(git_repo): + """Linters are run on all ``src`` command line options, modified or not""" + git_repo.add({"src/a.py": "foo\n", "src/subdir/b.py": "bar\n"}, commit="Initial") + with patch.object(darker.__main__, "run_linters") as run_linters: + run_linters.return_value = 0 + + retval = darker.__main__.main(["--check", "--lint=mylint", "src"]) + + run_linters.assert_called_once_with( + ["mylint"], Path("src").absolute(), {Path(".")}, ANY, ANY + ) + assert retval == 0 + + def test_print_diff(tmp_path, capsys): """print_diff() prints Black-style diff output with 5 lines of context""" Path(tmp_path / "a.py").write_text("dummy\n", encoding="utf-8")