Repository navigation
implement autofix for TRIO100 - #149
Conversation
|
It be done! Very happy with this new autofix testing infrastructure, and I even just now realized that I can exclude autofix files from being shed'd by excluding them in pre-commit ^^ Uses a similar approach to Zac-HD/shed#52 ; and shed would normally codemod away the useless pass statements that are inserted in the autofix files sometimes ... which would cause the autofix tests to fail.
|
jakkdl
left a comment
jakkdl
left a comment
There was a problem hiding this comment.
Some comments to ease reviewing
| @@ -100,20 +100,20 @@ def visit(self, node: ast.AST): | |||
|
|
|||
|
|
|||
| class Flake8TrioRunner_cst: | |||
There was a problem hiding this comment.
the runner is kind of .. not necessary and clunky, esp as currently __init__ does a bunch of logic itself. Might warrant some restructuring.
|
|
||
| def run(self, module: Module) -> Iterable[Error]: | ||
| def run(self) -> Iterable[Error]: | ||
| if not self.visitors: | ||
| return | ||
| wrapper = cst.MetadataWrapper(module) | ||
| for v in self.visitors: | ||
| _ = wrapper.visit(v) | ||
| self.module = cst.MetadataWrapper(self.module).visit(v) | ||
| yield from self.state.problems |
There was a problem hiding this comment.
now saves and updates self.module after visiting.
|
|
||
| with trio.move_on_after(10): # error: 5, "trio", "move_on_after" | ||
| pass | ||
| ... |
There was a problem hiding this comment.
replaced pass with ellipsis partly in attempts to avoid shed headaches, but means you can see a difference with ellipsis's transferred over, vs injected passs, in the autofix file.
| @@ -0,0 +1,84 @@ | |||
| # type: ignore | |||
There was a problem hiding this comment.
there's not a ton to see here ("trio100_simple_autofix.py" is easier to read), but if you want to review it you should view this and tests/eval_files/trio100.py with a difftool of your choice.
| # extreme case I'm not gonna care about, i.e. one item in the with, but it's multiline. | ||
| # Only these leading comments, and the last one, are kept, the rest are lost. | ||
| with ( # a | ||
| # b | ||
| # c | ||
| trio.move_on_after( # error: 4, "trio", "move_on_after" | ||
| # e | ||
| 9999999999999999999999999999999999999999999999999999999 # f | ||
| # g | ||
| ) # h | ||
| # i | ||
| ): # this comment is kept | ||
| ... |
There was a problem hiding this comment.
I can maybe detect this and abort the autofix, rather than eating the comments. But I'm not sure if this happens ever? Maybe if you're passing a lot of parameters to one of fail_after/fail_at/move_on_after/CancelScope so it's gotten split across lines by Black.
There was a problem hiding this comment.
Fixed, comments are now preserved.
| monkeypatch.chdir(tmp_path) | ||
| monkeypatch.setattr( | ||
| sys, "argv", [tmp_path / "flake8_trio", "--autofix", "./example.py"] | ||
| ) |
There was a problem hiding this comment.
might deserve a helper function, and/or helper fixture
| def test_assure_all_autofix_files_used(): | ||
| assert set(autofix_files.keys()) - checked_autofix == set() |
There was a problem hiding this comment.
this one is a minor pain since I usually develop locally with --failed-first, but didn't see any simple way of forcing this to be run late without installing pytest plugins (and that might still get overridden by --failed-first). But should help the occasional messed up file management by making sure all files in autofixed_files are used.
(although ig that could be done by static analysis of directory contents....)
There was a problem hiding this comment.
fixed, now statically checking the contents of the file lists.
7b54bf6 to
522c1db
Compare
|
I realize now I added this comment in #124
that's ... really really hard to implement properly - at least until everything is converted to libcst. So that will have to wait for a later PR, if it turns out to actually be of use. I was probably mainly thinking of missing |
Zac-HD
left a comment
Zac-HD
left a comment
There was a problem hiding this comment.
This all looks good to me! Rebase on master and then merge?
| for res in self.node_dict[original_node]: | ||
| self.error(res.node, res.base, res.function) | ||
| # if: autofixing is enabled for this code | ||
| # then: remove the with and pop out it's body | ||
|
|
||
| if self.options.autofix and len(updated_node.items) == 1: | ||
| return flatten_preserving_comments(updated_node) |
There was a problem hiding this comment.
I assume that we don't actually emit the error for autofixed code?
There was a problem hiding this comment.
This currently always does, I'll introduce ways of configuring that soon :)
|
merged! 🚀 |
Before jumping on autofixing TRIO91X I decided to start with the simpler TRIO100.
Turns out I'd forgotten how messy it is to manipulate code and preserve whitespace, comments, lines, etc etc etc 🙃
But I remember solving it in shed, so I can probably crib that from there and write some helper function.Also remaining is actually changing the file on disk, and adding a flag.
shed/Black/isort is also back to being incredibly annoying, I'll have to figure out a better way of writing autofix files so they don't get fucked with as much as is done currently.