make format command (#221)

* setup ability to print diff with whitespace only changes highlighted

* got it working

* dog-food itself

* remove un-needed import

* don't cause confusion with stdlib

* get tests working

* format

* make format produce something the linter is happy with

* update readme

* s/fix/format

* add global exception handling/logging

* rename this back

* get import sorting to work the way we want

* cleanup fix module

* dog-food

* silence formatting calls

* make test output consistent

* ni-python-styleguide format!

* add better-diff as dep

* switch over to using better_diff

* pass down file contents instead of file handles

* also specify the fromfile name

* update snapshots

* type-hint the extend_ignore all the way down

* handle the correct types on extend_ignore

* reduce diff

* dog-food format

* didn't use this

Author: mshafer-NI

README.md

@@ -63,9 +63,9 @@ application-import-names = "<app_name>"
 
 ### Formatting
 
-`ni-python-styleguide` in the future will have a `format` command which we intend to fix as many lint issues as possible.
+`ni-python-styleguide` has a subcommand `format` which will run [black](https://pypi.org/project/black/) and [isort](https://pycqa.github.io/isort/).
 
-Until then you'll want to set the following to get `black` formatting as the styleguide expects.
+If you wish to be able to invoke black directly, you'll want to set the following to get `black` formatting as the styleguide expects.
 
 ```toml
 # pyproject.toml

ni_python_styleguide/_acknowledge_existing_errors/__init__.py

@@ -1,6 +1,7 @@
 import logging
 import pathlib
 import re
+import typing
 from collections import defaultdict
 
 from ni_python_styleguide import _format, _utils
@@ -41,7 +42,12 @@ def _filter_suppresion_from_line(line: str):
 
 
 def acknowledge_lint_errors(
-    exclude, app_import_names, extend_ignore, file_or_dir, *_, aggressive=False
+    exclude,
+    app_import_names,
+    extend_ignore: typing.Optional[str],
+    file_or_dir,
+    *_,
+    aggressive=False,
 ):
     """Adds a "noqa" comment for each of existing errors (unless excluded).
 

ni_python_styleguide/_cli.py

@@ -1,13 +1,11 @@
 import pathlib
 import sys
+import typing
 
 import click
 import toml
 
-from ni_python_styleguide import _acknowledge_existing_errors
-from ni_python_styleguide import _fix
-from ni_python_styleguide import _Flake8Error
-from ni_python_styleguide import _lint
+from ni_python_styleguide import _acknowledge_existing_errors, _fix, _Flake8Error, _lint
 
 
 def _qs_or_vs(verbosity):
@@ -181,12 +179,30 @@ def acknowledge_existing_violations(obj, extend_ignore, file_or_dir, aggressive)
     help="Remove any existing acknowledgments, fix what can be fixed, and re-acknowledge remaining.",
 )
 @click.pass_obj
-def fix(obj, extend_ignore, file_or_dir, aggressive):
-    """Fix basic linter/formatting errors in file(s)/directory(s) given."""  # noqa: D4
+def fix(obj, extend_ignore: typing.Optional[str], file_or_dir, aggressive):
+    """Fix basic linter/formatting errors in file(s)/directory(s) given."""
     _fix.fix(
         exclude=obj["EXCLUDE"],
         app_import_names=obj["APP_IMPORT_NAMES"],
         extend_ignore=extend_ignore,
         file_or_dir=file_or_dir or [pathlib.Path.cwd()],
         aggressive=aggressive,
     )
+
+
+@main.command()
+@click.option("--diff", is_flag=True, help="Show a diff of the changes that would be made")
+@click.option("--check", is_flag=True, help="Error if files would be changed")
+@click.argument("file_or_dir", nargs=-1)
+@click.pass_obj
+def format(obj, file_or_dir, check: bool, diff: bool):
+    """Format the file(s)/directory(s) given."""
+    _fix.fix(
+        exclude=obj["EXCLUDE"],
+        app_import_names=obj["APP_IMPORT_NAMES"],
+        extend_ignore=None,
+        file_or_dir=file_or_dir or [pathlib.Path.cwd()],
+        aggressive=False,
+        check=check,
+        diff=diff,
+    )

ni_python_styleguide/_fix.py

@@ -1,8 +1,10 @@
 import logging
 import pathlib
+import typing
 from collections import defaultdict
 from typing import Iterable
 
+import better_diff.unified_plus
 import isort
 import pathspec
 
@@ -12,59 +14,10 @@
     _format,
     _utils,
 )
+from ni_python_styleguide._utils import temp_file
 
 _module_logger = logging.getLogger(__name__)
-
-
-def _split_imports_line(lines: str, *_, **__):
-    r"""Splits multi-import lines to multiple lines.
-
-    >>> _split_imports_line("import os, collections\n")
-    'import os\nimport collections\n'
-
-    >>> _split_imports_line("import os\n")
-    'import os\n'
-
-    >>> _split_imports_line("from ni_python_styleguide import"
-    ... " _acknowledge_existing_errors, _format")
-    'from ni_python_styleguide import _acknowledge_existing_errors\nfrom ni_python_styleguide import _format\n'
-
-    >>> _split_imports_line("from ni_python_styleguide import _acknowledge_existing_errors")
-    'from ni_python_styleguide import _acknowledge_existing_errors\n'
-
-    >>> _split_imports_line("import os, collections\nimport pathlib")
-    'import os\nimport collections\nimport pathlib\n'
-
-    >>> _split_imports_line("import os, collections\nimport pathlib, os")
-    'import os\nimport collections\nimport pathlib\nimport os\n'
-
-    >>> _split_imports_line("\n")
-    '\n'
-    """  # noqa W505: long lines...
-    result_parts = []
-    for line in lines.splitlines(keepends=True):
-        code_portion_of_line, *non_code = line.split("#", maxsplit=1)
-        first, _, rest = code_portion_of_line.partition(",")
-        if not all(
-            [
-                rest,
-                "import " in code_portion_of_line,
-                code_portion_of_line.strip().startswith("import ")
-                or code_portion_of_line.strip().startswith("from "),
-            ]
-        ):
-            result_parts.append(code_portion_of_line)
-            continue
-        prefix, first = " ".join(first.split()[:-1]), first.split()[-1]
-        split_up = [first] + rest.split(",")
-        result_parts.extend([prefix + " " + part.strip() for part in split_up])
-    suffix = ""
-    if non_code:
-        suffix = "#" + "".join(non_code)
-    result = "\n".join(result_parts) + suffix
-    if result.strip():
-        return result.rstrip() + "\n"
-    return result
+_module_logger.addHandler(logging.NullHandler())
 
 
 def _sort_imports(file: pathlib.Path, app_import_names):
@@ -82,27 +35,38 @@ def _sort_imports(file: pathlib.Path, app_import_names):
 
 def _format_imports(file: pathlib.Path, app_import_names: Iterable[str]) -> None:
     _sort_imports(file, app_import_names=app_import_names)
-    _format.format(file)
+    _format.format(file, "-q")
+
+
+def _posix_relative_if_under(file: pathlib.Path, base: pathlib.Path) -> str:
+    file_resolved = file.resolve()
+    base_resolved = base.resolve()
+    if file_resolved.as_posix().startswith(base_resolved.as_posix()):
+        return file_resolved.relative_to(base_resolved).as_posix()
+    return file_resolved.as_posix()
 
 
 def fix(
     exclude: str,
     app_import_names: str,
-    extend_ignore,
+    extend_ignore: typing.Optional[str],
     file_or_dir,
     *_,
     aggressive=False,
+    diff=False,
+    check=False,
 ):
     """Fix basic linter errors and format."""
     file_or_dir = file_or_dir or ["."]
-    extend_ignore = extend_ignore or ""
-    exclude = exclude or ""
+    if diff or check:
+        if aggressive:
+            raise Exception("Cannot use --aggressive with --diff or --check")
     if aggressive:
         glob_spec = pathspec.PathSpec.from_lines(
             "gitwildmatch",
             ["*.py"]
             + [f"!{exclude_}" for exclude_ in exclude.split(",") if exclude_]
-            + [f"!{ignore_}" for ignore_ in extend_ignore.split(",") if ignore_],
+            + [f"!{ignore_}" for ignore_ in (extend_ignore or "").split(",") if ignore_],
         )
         all_files = []
         for file_or_dir_ in file_or_dir:
@@ -130,25 +94,45 @@ def fix(
         lint_errors_by_file[pathlib.Path(error.file)].append(error)
 
     failed_files = []
+    make_changes = not (diff or check)
     for bad_file, errors_in_file in lint_errors_by_file.items():
         try:
-            _format.format(bad_file)
-            _format_imports(file=bad_file, app_import_names=app_import_names)
-            remaining_lint_errors_in_file = _utils.lint.get_errors_to_process(
-                exclude,
-                app_import_names,
-                extend_ignore,
-                [bad_file],
-                excluded_errors=[],
-            )
-            if remaining_lint_errors_in_file and aggressive:
-                _acknowledge_existing_errors.acknowledge_lint_errors(
-                    exclude=exclude,
-                    app_import_names=app_import_names,
-                    extend_ignore=extend_ignore,
-                    aggressive=aggressive,
-                    file_or_dir=[bad_file],
+            if make_changes:
+                _format.format(bad_file, "-q")
+                _format_imports(file=bad_file, app_import_names=app_import_names)
+                remaining_lint_errors_in_file = _utils.lint.get_errors_to_process(
+                    exclude,
+                    app_import_names,
+                    extend_ignore,
+                    [bad_file],
+                    excluded_errors=[],
                 )
+                if remaining_lint_errors_in_file and aggressive:
+                    _acknowledge_existing_errors.acknowledge_lint_errors(
+                        exclude=exclude,
+                        app_import_names=app_import_names,
+                        extend_ignore=extend_ignore,
+                        aggressive=aggressive,
+                        file_or_dir=[bad_file],
+                    )
+            else:
+                with temp_file.multi_access_tempfile(suffix="__" + bad_file.name) as working_file:
+                    working_file.write_text(bad_file.read_text())
+                    _format.format(working_file, "-q")
+                    _format_imports(file=working_file, app_import_names=app_import_names)
+
+                    diff_lines = better_diff.unified_plus.format_diff(
+                        bad_file.read_text(),
+                        working_file.read_text(),
+                        fromfile=f"{_posix_relative_if_under(bad_file, pathlib.Path.cwd())}",
+                        tofile=f"{_posix_relative_if_under(bad_file, pathlib.Path.cwd())}_formatted",
+                    )
+                    if diff:
+                        print(diff_lines)
+                    if check and diff_lines:
+                        print("Error: file would be changed:", str(bad_file))
+                        failed_files.append((bad_file, "File would be changed."))
+
         except Exception as e:
             failed_files.append((bad_file, e))
     if failed_files:

ni_python_styleguide/_lint.py

@@ -2,13 +2,16 @@
 
 import contextlib
 import io
+import typing
 
 import flake8.main.application
 
 from ni_python_styleguide import _config_constants, _Flake8Error
 
 
-def lint(qs_or_vs, exclude, app_import_names, format, extend_ignore, file_or_dir):
+def lint(
+    qs_or_vs, exclude, app_import_names, format, extend_ignore: typing.Optional[str], file_or_dir
+):
     """Run the linter."""
     app = flake8.main.application.Application()
     args = [
@@ -31,7 +34,9 @@ def lint(qs_or_vs, exclude, app_import_names, format, extend_ignore, file_or_dir
 
 # Note: tried to use functools.wraps
 #  - but VSCode did not properly identify the wrapped method's signature :(
-def get_lint_output(qs_or_vs, exclude, app_import_names, format, extend_ignore, file_or_dir) -> str:
+def get_lint_output(
+    qs_or_vs, exclude, app_import_names, format, extend_ignore: typing.Optional[str], file_or_dir
+) -> str:
     "Return the output from running the linter."
     capture = io.TextIOWrapper(io.BytesIO())
     with contextlib.redirect_stdout(capture):

ni_python_styleguide/_utils/lint.py

@@ -5,7 +5,9 @@
 from ni_python_styleguide import _lint
 
 
-def get_errors_to_process(exclude, app_import_names, extend_ignore, file_or_dir, excluded_errors):
+def get_errors_to_process(
+    exclude, app_import_names, extend_ignore: typing.Optional[str], file_or_dir, excluded_errors
+):
     """Get lint errors to process."""
     lint_errors = sorted(
         _lint.get_lint_output(

ni_python_styleguide/config.toml

@@ -8,3 +8,4 @@ line-length = 100
 [tool.isort]
 profile = "black"
 combine_as_imports = true
+force_alphabetical_sort_within_sections = true

poetry.lock

@@ -26,6 +26,7 @@ pycodestyle = [
     {version = "^2.11", python = "^3.12"},
 ]
 black = ">=23.1"
+better-diff = "^0.1.3"
 
 # Additional support libraries
 # These dependencies were selected because they are already used by black.