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

Add mypy plugin for NewType #55

Merged
merged 7 commits into from
Dec 11, 2019
Merged

Add mypy plugin for NewType #55

merged 7 commits into from
Dec 11, 2019

Conversation

selimb
Copy link
Contributor

@selimb selimb commented Dec 8, 2019

Took a stab at fixing #50. I had the same problem.

As in tests/test_mypy.py, the following:

from dataclasses import dataclass
import marshmallow as ma
from marshmallow_dataclass import NewType

Email = NewType("Email", str, validate=ma.validate.Email)
UserID = NewType("UserID", validate=ma.validate.Length(equal=32), typ=str)

@dataclass
class User:
    id: UserID
    email: Email

user = User(id="a"*32, email="user@email.com")

reveal_type(user.id)
reveal_type(user.email)
User(id=42, email="user@email.com")
User(id="a"*32, email=["not", "a", "string"])

now outputs:

main.py:14: note: Revealed type is 'builtins.str'
main.py:15: note: Revealed type is 'builtins.str'
main.py:17: error: Argument "id" to "User" has incompatible type "int"; expected "str"
main.py:18: error: Argument "email" to "User" has incompatible type "List[str]"; expected "str"
  • I can also add some documentation once you approve of the changes!

@selimb
Copy link
Contributor Author

selimb commented Dec 8, 2019

Hmm tests were failing on CI for pypy3 because typed-ast (a dependency of mypy) fails to install on pypy: python/typed_ast#111

I've tweaked setup.py and test_mypy.py to skip mypy tests on pypy.

@lovasoa
Copy link
Owner

lovasoa commented Dec 9, 2019

Great, this looks good ! Thank you @selimb

@sloria, what do you think ?

@lovasoa lovasoa requested a review from sloria December 9, 2019 12:11
Copy link
Collaborator

@sloria sloria left a 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!

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}")
Copy link
Collaborator

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

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Owner

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.

Copy link
Contributor Author

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.

tests/test_mypy.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
Copy link
Collaborator

@sloria sloria left a 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sloria @lovasoa I was a bit bummed by the lack of Windows support (I happen to be developing on Windows at the moment), but at least it's just for tests. I might raise an issue on pytest-mypy-plugins though, since there's no good reason to require capturer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@selimb
Copy link
Contributor Author

selimb commented Dec 10, 2019

Added note in documentation on enabling the mypy plugin.

Maybe just add a TODO somewhere--or just create a new issue--to port over existing tests to pytest style?

#56

@sloria sloria merged commit e823a6b into lovasoa:master Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants