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

Mypy plugin #722

Merged
merged 18 commits into from Oct 31, 2019
Merged

Mypy plugin #722

merged 18 commits into from Oct 31, 2019

Conversation

@dmontagu
Copy link
Collaborator

dmontagu commented Aug 7, 2019

Change Summary

Add a mypy plugin with a number of features; see the added docs for more details.

Other notable improvements for pydantic developers:

  • The mypy tests have been reorganized to be easier to work on
    • Eventually, it will probably make sense to better organize individual tests, but
      this seemed like the most practical solution for now
  • Thanks to the reorganization, the mypy tests now run after all others (I think this is preferable)
  • The mypy tests and codebase check now run much faster on repeated execution because they no longer invalidate each other's caches
    • The tests now make use of separate cache directories by plugin settings, all contained as subfolders of the previously used .mypy_cache folder.
    • Because they are subfolders of .mypy_cache, the same make clean works to clean up all of the cache directories

Related issue number

Closes #460

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • change description file added to changes/,
    see changes/README.md for details
    on the format
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Aug 7, 2019

In principle this looks great, but as you say let's defer it until 1.1 - it shouldn't cause any backwards incompatibility issues I assume?

@dmontagu dmontagu force-pushed the dmontagu:mypy-plugin branch from 977c4d0 to 04982a7 Aug 7, 2019
setup.cfg Outdated Show resolved Hide resolved
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Aug 7, 2019

You can always just turn the plugin off (by not incluiding it in mypy.ini/setup.cfg), so it is strictly opt-in. So I don't think there are backwards compatibility issues.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Aug 7, 2019

For what it's worth, if anyone wants this in the interim, you can just copy/paste the contents of the pydantic/mypy.py file into your own project and add it to the mypy plugins in your mypy.ini/setup.cfg and it should just work. (Doesn't require any changes to pydantic.)

It would be nice to be able to distribute it with the library though.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #722 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #722    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          16     17     +1     
  Lines        2800   3142   +342     
  Branches      543    615    +72     
======================================
+ Hits         2800   3142   +342
Impacted Files Coverage Δ
pydantic/mypy.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb2a302...7bf2595. Read the comment docs.

David Montague added 2 commits Aug 7, 2019
@dmontagu dmontagu force-pushed the dmontagu:mypy-plugin branch from f8d513d to 0cda68e Oct 25, 2019
David Montague
@dmontagu dmontagu removed the deferred label Oct 25, 2019
David Montague
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 25, 2019

After some painful edge case exploration and mypy debugging, I've gotten the test coverage of the plugin itself up to 100% (modulo two minor no-covers related to import edge cases, where I've just copied the logic from the official dataclasses plugin).

Still needs:

  • docs
  • support for type checking the construct method

I think it might make sense to (eventually) support other config settings and/or finer-grained strictness controls, but I'm more inclined to just release this and see what features people actually request.

Edit: Actually, I'm going to hold off on adding support for construct until I have a chance to work on python/mypy#7301; otherwise we'd have to copy a lot of code right out of mypy.

Edit 2: It turned out to be easier than I expected to get this working. PR opened for the mypy issue, but I was able to drop a single function into place that we can remove once the PR is merged.

David Montague added 3 commits Oct 26, 2019
David Montague added 5 commits Oct 27, 2019
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 27, 2019

@samuelcolvin I think this is basically done! I added docs and updated the description at the top with some more detail.

I think the docs pretty clearly cover everything the plugin adds.

The tests are a little haphazard organized, basically just dumped into the big plugin-success.py and plugin-fail.py files; we can fix that eventually but I personally think this is good enough for a first release. I'm happy to put more effort into cleaning it up though if you want that to happen before a merge.

Special note: these changes should make running the mypy tests and mypy check faster on subsequent runs due to better caching (even though there are now more tests).

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 27, 2019

Also, @koxudaxi if you have a chance to review I think you might find it interesting and/or have some inputs.

David Montague
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Oct 27, 2019

I'm away from my computer but will look on Tuesday if not before.

Are we caching the mypy cache directory (sorry can't see on my phone)?

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 27, 2019

@samuelcolvin Sounds good, no rush.

I’m not sure what you mean by “are we caching the mypy cache directory” — could you clarify?

I put the test caches inside the default one (used during checks). The structure of the caching directories is such that this doesn’t cause any conflicts.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Oct 27, 2019

Sorry, I mean there's a .mypy_cache or simpler directory. Have we added that to the list of cached directories in Travis?

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 27, 2019

Yeah, the directory is .mypy_cache. I didn’t know you could cache directories in Travis. Actually I don’t know much about Travis in general. I’m not at my computer now either but can try to figure it out when I can if you’d like.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Oct 27, 2019

Well, might make things faster. Don't worry if it's a pain.

docs/mypy_plugin.md Outdated Show resolved Hide resolved
docs/mypy_plugin.md Outdated Show resolved Hide resolved
docs/mypy_plugin.md Outdated Show resolved Hide resolved
David Montague
Copy link
Owner

samuelcolvin left a comment

In general this all looks great. Must have been a massive effort!

More of my comments are questions or trivial corrections.

docs/mypy_plugin.md Show resolved Hide resolved
docs/mypy_plugin.md Outdated Show resolved Hide resolved
docs/mypy_plugin.md Outdated Show resolved Hide resolved
docs/mypy_plugin.md Outdated Show resolved Hide resolved
docs/mypy_plugin.md Show resolved Hide resolved
pydantic/mypy.py Show resolved Hide resolved
pydantic/mypy.py Show resolved Hide resolved
pydantic/mypy.py Show resolved Hide resolved
pydantic/mypy.py Show resolved Hide resolved

cases = [
('mypy-plugin.ini', 'plugin_success.py', None),
('mypy-plugin.ini', 'plugin_fail.py', 'plugin-fail.txt'),

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Oct 29, 2019

Owner

are you sure these are really easier as separate files? Surely easier to have the expected output right here with the test even if some are very verbose?

This comment has been minimized.

Copy link
@dmontagu

dmontagu Oct 30, 2019

Author Collaborator

It's extremely annoying to iterate on it without a mechanism for generating the expected output. By putting it in a file, I can set GENERATE=True above and it will write the file with the actual output, and I can look at the git diffs to see if things are as desired.

I initially tried having the results in-line here, but the problem is that a one line changes all the line numbers. And the line numbers actually do matter, so I think we shouldn't just strip them from the expected output. But then every time I tweak a line I had to manually compare all the output lines and fix the line numbers. Then I'd make another minor change and have to do it all again.

I think there might be better ways to manage mypy plugin tests (might be good to look at how the sqlalchemy or django plugins do this). But I thought they might ultimately rely on calling mypy on a separate file for each test, and I was specifically trying to minimize the amount of time spent executing mypy tests during normal test runs, and I think there is a substantial amount of overhead for each file analyzed. I tried to get all the tests into as few files as possible to keep the tests running fast.

@dmontagu dmontagu force-pushed the dmontagu:mypy-plugin branch from 9e53321 to 8873ea0 Oct 30, 2019
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 30, 2019

@samuelcolvin I think I've incorporated all your feedback on the non-docs files, or at least responded above where I had concerns.

I renamed the plugin config settings so they could all be False by default (but basically have the same meaning).

I'm going to wait until we reach consensus on the non-docs stuff before updating the docs.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 30, 2019

It looks like this is what django-stubs uses.

Here's a blog article discussing: https://sobolevn.me/2019/08/testing-mypy-types

It looks like a massive improvement over what is used in this PR, but 1) I'm not sure how fast it would run, and 2) I'm not sure how maintained it is.

We could also just run the mypy plugin tests separately, the only downside would be the added complexity of checking for 100% coverage.

tests/mypy/test_mypy.py Outdated Show resolved Hide resolved
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Oct 30, 2019

Looking good.

Apart from the docs and a few pretty trivial potential fixes, I think this is more or less ready.

David Montague added 2 commits Oct 31, 2019
David Montague
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 31, 2019

I tried using dataclasses but it caused the cythonized builds to fail due to the __init__ method not getting created properly.

Maybe there's a workaround, but since it's not really giving much benefit here I figured hand-writing the init function was fine.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 31, 2019

Also, I believe I have now addressed all your concerns with regard to the docs. Maybe give it another once over, but I think it's good to go now.

@samuelcolvin samuelcolvin merged commit 0c18619 into samuelcolvin:master Oct 31, 2019
10 checks passed
10 checks passed
Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to cb2a302
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20191031.3 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Oct 31, 2019

this is awesome.

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.