test for autofix_files, fix some 91x autofixes, rejig eval_files #159
Conversation
jakkdl
force-pushed
the
autofix_infra
branch
2 times, most recently
from
bce4027
to
b3d8a6e
Compare
There was a problem hiding this comment.
first review, during which I decided to change the behaviour so instead of tracking existence of files in the autofix dir it's now a magic comment # AUTOFIX. This gets rid of empty diff files and files in the autofix dir that are identical to the ones in eval_files, and simultaneously added checks that eval files without that marker doesn't change the source code - which should catch if one forgets to add the magic comment to an eval file for an autofixing visitor (and other esoteric errors)
| @@ -90,9 +91,125 @@ def checkpoint_statement(library: str) -> cst.SimpleStatementLine: | |||
| ) | |||
|
|
|||
|
|
|||
| class CommonVisitors(cst.CSTTransformer, ABC): | |||
There was a problem hiding this comment.
The subclassing needed to be expanded to move some import logic into it, since otherwise a file only with yields that can't be autofixed, inside loop bodies, (i.e. tests/eval_files/trio91x_noautofix) had no way of figuring out whether to add an import or not
There was a problem hiding this comment.
most of the methods are moved up and only lightly modified
| @@ -248,7 +317,7 @@ def check_function_exit( | |||
| # Add this as a node potentially needing checkpoints only if it | |||
| # missing checkpoints solely depends on whether the artificial statement is | |||
| # "real" | |||
| if len(self.uncheckpointed_statements) == 1: | |||
| if len(self.uncheckpointed_statements) == 1 and not self.noautofix: | |||
There was a problem hiding this comment.
noautofix is currently only set by boolops, but that can easily be expanded.
| # TODO: generate an error in these two if transforming+visiting is done in a single | ||
| # pass and emit-error-on-transform can be enabled/disabled. The error can't be | ||
| # generated in the yield/return since it doesn't know if it will be autofixed. | ||
|
|
||
| # SimpleStatementSuite and multi-statement lines can probably be autofixed, but | ||
| # for now just don't insert checkpoints in the wrong place. | ||
| def leave_SimpleStatementSuite( | ||
| self, | ||
| original_node: cst.SimpleStatementSuite, | ||
| updated_node: cst.SimpleStatementSuite, | ||
| ) -> cst.SimpleStatementSuite: | ||
| self.add_statement = None | ||
| return updated_node | ||
|
|
||
| # insert checkpoint before yield with a flattensentinel, if indicated | ||
| def leave_SimpleStatementLine( | ||
| self, | ||
| original_node: cst.SimpleStatementLine, | ||
| updated_node: cst.SimpleStatementLine, | ||
| ) -> cst.SimpleStatementLine | cst.FlattenSentinel[cst.SimpleStatementLine]: | ||
| if self.add_statement is None: | ||
| return updated_node | ||
|
|
||
| # multiple statements on a single line is not handled | ||
| if len(updated_node.body) > 1: | ||
| self.add_statement = None | ||
| return updated_node | ||
|
|
||
| res = cst.FlattenSentinel([self.add_statement, updated_node]) | ||
| self.add_statement = None | ||
| return res # noqa: R504 | ||
|
|
jakkdl
force-pushed
the
autofix_infra
branch
2 times, most recently
from
168c1df
to
c94994f
Compare
|
Okay I think I'm finally happy with this, gotta resist the temptation to add more features to this already messy PR. |
|
I might let you rebase this before I try to review it 😅 |
… accomodate the new tests
|
Ah, you could just have reviewed the second commit - I can try to be more clear about that in the future. But rebased! |
| @@ -2,24 +2,24 @@ | ||
| @@ -3,24 +3,24 @@ |
There was a problem hiding this comment.
We might want to replace these numbers with x so that inserting or deleting unchanged code doesn't affect the diff? Definitely a follow-up PR if we decide to do that though.
There was a problem hiding this comment.
ye it's very ugly - but wasn't sure if they might still be useful. But I guess if you actually want to find the line numbers you can look at the non-diff autofix file or so.
branched on top of #158
F_ixes the sixth checkbox in #124 - "test that autofixed files don't generate errors", and also tests that autofixed files don't get further autofixes applied if run through the transformer again.
Finished, but somewhat messy - want to take a fresh look atvisitor91x.pyand the class structure there another day to see if I can make it less awful.eval/autofix diffs are quite irritating when line numbers change, both in showing up in the diff and when developing when I manually edit the files - so it's possible I tweak something there in the future.
Feel free to review if you want, but I plan on cleaning it up and adding review comments so it should be easier after that.