-
Notifications
You must be signed in to change notification settings - Fork 76
Add mypy plugin for NewType #55
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
Conversation
Hmm tests were failing on CI for I've tweaked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits.
I don't have experience with mypy plugins and therefore don't have high confidence in evaluating plugin code, but the test output seems to verify the correct behavior.
Thank for working on this!
tests/test_mypy.py
Outdated
Also cd into it for the duration of the test to get simple filenames in mypy output. | ||
""" | ||
testname = self.id().split(".")[-1] | ||
self.testdir = os.path.join(TEST_OUTPUT_DIR, f"test_mypy-{testname}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use tempfile.TemporaryDirectory
for this?
Sidenote: this is another good time to re-evaluate pytest, which has built-in functionality for this sort of thing. Not a blocker, but worth considering. @lovasoa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I argued against pytest in the past because I felt like it wasn't providing anything we needed for this project. But if there is a plugin that allows testing the mypy plugin easily, then I have nothing against switching to pytest and using it. It would definitely make the transition worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh. Deleted previous comment by accident. Copied below for reference.
Hah, yeah I must admit I was bit surprised that this was using unittest. Note that there's also a pytest plugin to easily test mypy plugins, which would effectively remove the need for all the code in setup, teardown and assert_mypy_output.
Do you want me to leave a TODO somewhere, as a reminder to switch to (or at least consider) pytest-mypy-plugins if the project moves to pytest?
Also note that pytest supports unittest, so you could use the pytest runner without necessarily "rewriting" the existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lovasoa Cool. How do you want to proceed re: existing tests and re: this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since switching to pytest doesn't require rewriting any of the existing tests, I think we can do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! Will work on that later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Maybe just add a TODO somewhere--or just create a new issue--to port over existing tests to pytest style?
"pytest", | ||
# re: pypy: typed-ast (a dependency of mypy) fails to install on pypy | ||
# https://github.com/python/typed_ast/issues/111 | ||
# re: win32: pytest-mypy-plugins depends on capturer, which isn't supported on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added note in documentation on enabling the mypy plugin.
|
Took a stab at fixing #50. I had the same problem.
As in
tests/test_mypy.py
, the following:now outputs: