Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement autofix for 91x #158

Merged
merged 1 commit into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ To check that all codes are tested and documented there's a test that error code
## Test generator
Tests are automatically generated for files in the `tests/eval_files/` directory, with the code that it's testing interpreted from the file name. The file extension is split off, if there's a match for for `_py\d*` it strips that off and uses it to determine if there's a minimum python version for which the test should only run.

### autofix files
Checks that have autofixing can have a file in the `tests/autofix_files` directory matching the filename in `tests/eval_files`. The result of running the checker on the eval file with autofix enabled will then be compared to the content of the autofix file and will print a diff (if `-s` is on) and assert that the content is the same. `--generate-autofix` is added as a pytest flag to ease development, which will print a diff (with `-s`) and overwrite the content of the autofix file. Also see the magic line marker `pass # AUTOFIX_LINE ` below
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of spitting out the resulting .py files, can we save the diff between before and after as a .py.diff? It's much easier to get a sense of "what it's doing" in this format, and I'm concerned that without that code review is not going to be effective.

(although instead of constructing a three-way diff, I'd just let Pytest's assertion helper show the diff between the actual and expected diffs)

((diff diff diff, it hardly seems to diff mean anything anymore. diff)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the generated files for checking that they're valid and passes linters, and when developing they're easier to read and throw into cst.parse_module, but I'll generate diff files as well 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yep, both sounds good!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wohoo, bigger commits! 😁
They're still not perfect, as they're not showing e.g. where checkpoints aren't inserted - but that will be covered by future tests that autofixed files don't generate any errors. (and will have to figure out a solution for errors that are expected not to get autofixed)


### `error:`
Lines containing `error:` are parsed as expecting an error of the code matching the file name, with everything on the line after the colon `eval`'d and passed as arguments to `flake8_trio.Error_codes[<error_code>].str_format`. The `globals` argument to `eval` contains a `lineno` variable assigned the current line number, and the `flake8_trio.Statement` namedtuple. The first element after `error:` *must* be an integer containing the column where the error on that line originates.
#### `TRIOxxx:`
Expand Down
16 changes: 14 additions & 2 deletions flake8_trio/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@

import libcst as cst

from .visitors import ERROR_CLASSES, ERROR_CLASSES_CST, utility_visitors
from .visitors import (
ERROR_CLASSES,
ERROR_CLASSES_CST,
utility_visitors,
utility_visitors_cst,
)

if TYPE_CHECKING:
from argparse import Namespace
Expand Down Expand Up @@ -104,6 +109,13 @@ def __init__(self, options: Namespace, module: Module):
super().__init__()
self.state = SharedState(options)
self.options = options

# Could possibly enable/disable utility visitors here, if visitors declared
# dependencies
self.utility_visitors: tuple[Flake8TrioVisitor_cst, ...] = tuple(
v(self.state) for v in utility_visitors_cst
)

self.visitors: tuple[Flake8TrioVisitor_cst, ...] = tuple(
v(self.state) for v in ERROR_CLASSES_CST if self.selected(v.error_codes)
)
Expand All @@ -112,7 +124,7 @@ def __init__(self, options: Namespace, module: Module):
def run(self) -> Iterable[Error]:
if not self.visitors:
return
for v in self.visitors:
for v in (*self.utility_visitors, *self.visitors):
self.module = cst.MetadataWrapper(self.module).visit(v)
yield from self.state.problems

Expand Down
11 changes: 9 additions & 2 deletions flake8_trio/visitors/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,18 @@
if TYPE_CHECKING:
from .flake8triovisitor import Flake8TrioVisitor, Flake8TrioVisitor_cst

__all__ = ["ERROR_CLASSES", "default_disabled_error_codes", "utility_visitors"]
__all__ = [
"ERROR_CLASSES",
"ERROR_CLASSES_CST",
"default_disabled_error_codes",
"utility_visitors",
"utility_visitors_cst",
]
ERROR_CLASSES: set[type[Flake8TrioVisitor]] = set()
ERROR_CLASSES_CST: set[type[Flake8TrioVisitor_cst]] = set()
utility_visitors: set[type[Flake8TrioVisitor]] = set()
default_disabled_error_codes: list[str] = []
utility_visitors: set[type[Flake8TrioVisitor]] = set()
utility_visitors_cst: set[type[Flake8TrioVisitor_cst]] = set()

# Import all visitors so their decorators run, filling the above containers
# This has to be done at the end to avoid circular imports
Expand Down
10 changes: 10 additions & 0 deletions flake8_trio/visitors/flake8triovisitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,3 +227,13 @@ def error(
*args,
)
)

@property
def library(self) -> tuple[str, ...]:
return self.__state.library if self.__state.library else ("trio",)

# library_str not used in cst yet

def add_library(self, name: str) -> None:
if name not in self.__state.library:
self.__state.library = self.__state.library + (name,)
8 changes: 8 additions & 0 deletions flake8_trio/visitors/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
ERROR_CLASSES_CST,
default_disabled_error_codes,
utility_visitors,
utility_visitors_cst,
)

if TYPE_CHECKING:
Expand Down Expand Up @@ -58,6 +59,13 @@ def utility_visitor(c: type[T]) -> type[T]:
return c


def utility_visitor_cst(c: type[T_CST]) -> type[T_CST]:
assert not hasattr(c, "error_codes")
c.error_codes = {}
utility_visitors_cst.add(c)
return c


def _get_identifier(node: ast.expr) -> str:
if isinstance(node, ast.Name):
return node.id
Expand Down
Loading