[TYP] Add tool for running stubtest #26928
Conversation
Walks the ast to find delete_parameter calls and automatically ignores them as their signature is modified at runtime Could potentially be further modified to respect the alias behavior, which is the majority of the current allowlist Makes running stubtest easier as now it is just 'python tools/stubtest.py', rather than a long incantation with multiple arguments
|
I'm curious whether this works when matplotlib is installed normally (non-editable), as that is something that I needed to do on the Meson branch. |
|
Yes, it does appear to work on installed mpl |
|
dunno if it's tangential to this, but would the preference be to use this to make a pre-commit hook or to use mypy-mirror? been looking at https://jaredkhan.com/blog/mypy-pre-commit |
|
Stubtest takes longer than I'm really willing to put in a pre-commit hook (it runs mypy first and then does its checks which involve importing) On my machine this takes over a minute in total:
I could maybe justify normal mypy as a pre-commit (though running all of our precommit hooks ( Stubtest (well, at least this script as written) does not have as good of ways to only run on certain files. (you also specify by the import path because it is checking runtime behavior, so at least some manipulation would have to be done to the filepaths, though possible). All of these numbers are on my (pretty beefy) desktop machine, and would hesitate to do something that makes it worse. In all, not totally opposed to mypy, especially since I think it scopes to the edited files, so the worst case of 15 seconds is not likely to happen. but stubtest is probably good where it is at CI level and a tool that you can run locally if you either expect it to possibly fail or are diagnosing a failure from CI. |
Even if it does, it's faster for me to fix locally then running through push/ci cycles |
|
True, but whether it is worth it is dependent on not only the speedup of the failing path, but also the slowing down of the passing path and the proportion of changes which go down each path. I am probably a bit of an outlier in that my changes are very bimodal with really small changes (where an extra 15 seconds is frustrating) and really large changes (where I'm running these tools locally myself multiple times before I get to committing even), so perhaps not the best one to make that judgement. Again, I think I'd support adding mypy, but not stubtest, as a happy medium (especially if it scopes to edited files by default). |
tools/stubtest.py
Outdated
| self.visit(child) | ||
|
|
||
|
|
||
| with tempfile.NamedTemporaryFile("wt", delete=False) as f: |
There was a problem hiding this comment.
Ensure this cleans up like the context manager:
| with tempfile.NamedTemporaryFile("wt", delete=False) as f: | |
| # Use delete_on_close when Python 3.12 is minimum. | |
| f = tempfile.NamedTemporaryFile("wt", delete=False) | |
| try: |
There was a problem hiding this comment.
Alternatively you can just use a TemporaryDirectory instead and create a file with whatever name you want in that directory, to get both proper autodeletion without problems with close()ing the file. We use that pattern quite a few times in the library itself.
c818393 to
06e4731
Compare
| try: | ||
| os.unlink(f.name) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
Oops, I just realized this is still here; it can probably be deleted now that you're using TemporaryDirectory?
PR summary
Walks the ast to find delete_parameter calls and automatically ignores them as their signature is modified at runtime
Could potentially be further modified to respect the alias behavior, which is the majority of the current allowlist(Added this feature)Makes running stubtest easier as now it is just 'python tools/stubtest.py', rather than a long incantation with multiple arguments
This was discussed in part in #26894
PR checklist