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 support for dataclasses #966

Merged

Conversation

@koxudaxi
Copy link
Contributor

koxudaxi commented Nov 5, 2019

Change Summary

The PR supports dataclasses on mypy plugin

Related issue number

#957

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 5, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #966   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          19     19           
  Lines        3287   3293    +6     
  Branches      650    651    +1     
=====================================
+ Hits         3287   3293    +6
Impacted Files Coverage Δ
pydantic/dataclasses.py 100% <100%> (ø) ⬆️
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 caa270a...ff1f630. Read the comment docs.

pydantic/dataclasses.py Outdated Show resolved Hide resolved
pydantic/dataclasses.py Outdated Show resolved Hide resolved
pydantic/mypy.py Show resolved Hide resolved
tests/mypy/outputs/plugin-success.txt Outdated Show resolved Hide resolved
koxudaxi added 2 commits Nov 7, 2019
- change name of an expected file
- update documents
Copy link
Owner

samuelcolvin left a comment

we should also add a ! warning to usage/dataclasses.md saying that as of v1.2 the mypy plugin must be installed to type check pydantic dataclasses.

pydantic/mypy.py Show resolved Hide resolved
tests/mypy/modules/fail4.py 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 Outdated Show resolved Hide resolved
koxudaxi and others added 5 commits Nov 7, 2019
- update documents
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@koxudaxi

This comment has been minimized.

Copy link
Contributor Author

koxudaxi commented Nov 7, 2019

@dmontagu
Would you help me?
I get an unexpected result from the mypy test.
This problem is added empty lines on error out.
Also, The problem is happend only first time after make clean.

>           assert actual_out == expected_out, actual_out
E           AssertionError: 119: error: Unexpected keyword argument "name" for "AddProject"  [call-arg]
E             
E             119: error: Unexpected keyword argument "slug" for "AddProject"  [call-arg]
E             
E             119: error: Unexpected keyword argument "description" for "AddProject"  [call-arg]
E           assert '119: error: ..."  [call-arg]' == '119: error: ..."  [call-arg]'
E               119: error: Unexpected keyword argument "name" for "AddProject"  [call-arg]
E             - 
E               119: error: Unexpected keyword argument "slug" for "AddProject"  [call-arg]
E             - 
E               119: error: Unexpected keyword argument "description" for "AddProject"  [call-arg]

tests/mypy/test_mypy.py:61: AssertionError

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Nov 7, 2019

Huh, that's very weird. If it's just whitespace differences, it seems like it's probably an issue with mypy?

If the issue is just extra whitespace, I would just apply actual_output = re.sub(r'\n\s*\n', r'\n', actual_output) or whatever actually works to remove the blank lines (didn't check if that statement actually works).

@koxudaxi

This comment has been minimized.

Copy link
Contributor Author

koxudaxi commented Nov 8, 2019

@dmontagu

Thank you for your advice.
I tested this command. but, I did not look extra whitespaces.

mypy --config-file tests/mypy/configs/mypy-default.ini  --show-error-codes tests/mypy/modules/success.py

I use this method to resolve the error. actual_output = re.sub(r'\n\s*\n', r'\n', actual_output)

Copy link
Owner

samuelcolvin left a comment

Good except, as per the names success.py and plugin_success.py should pass and therefore have no output.

tests/mypy/modules/success.py Outdated Show resolved Hide resolved
tests/mypy/outputs/plugin-success.txt Outdated Show resolved Hide resolved
Copy link
Owner

samuelcolvin left a comment

otherwise LGTM

docs/usage/dataclasses.md Outdated Show resolved Hide resolved
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 9, 2019

@dmontagu are you happy with this?

koxudaxi and others added 2 commits Nov 9, 2019
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@samuelcolvin samuelcolvin requested a review from dmontagu Nov 13, 2019
@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Nov 13, 2019

@koxudaxi It looks to me like the received config isn't currently used by mypy, it's just allowed to be passed without raising an error -- is that right?

@samuelcolvin This looks good to me as a first release ensuring compatibility with the config argument. Eventually it would be nice to have the config values affect the type checking where relevant (I guess I'm not totally sure it affects anything mypy would detect anyway), but I don't think that needs to block a release.

Looks good to me!

@@ -22,6 +22,9 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
def __validate__(cls, v: Any) -> 'DataclassType':
pass

def __call__(self, *args: Any, **kwargs: Any) -> 'DataclassType':

This comment has been minimized.

Copy link
@dmontagu

dmontagu Nov 13, 2019

Collaborator

Should this use a TypeVar?

This comment has been minimized.

Copy link
@koxudaxi

koxudaxi Nov 14, 2019

Author Contributor

Did you expect this?

    def __call__(self: T, *args: Any, **kwargs: Any) -> T:
        pass

Also, Did you want to replace other -> 'DataclassType' like __init__ ?

This comment has been minimized.

Copy link
@dmontagu

dmontagu Nov 14, 2019

Collaborator

Yeah, that's what I meant, I think it would be better (assuming that is the right signature, which I think it is?).

I don't think the __init__ signature needs to change, but maybe the _validate_dataclass one should.

(Any other places where 'DataclassType' is currently used probably should; I believe this is what is done for factory classmethods for BaseModel -- I don't see why it shouldn't be that way for the dataclasses.)

Also, I would recommend using something like DataclassT as the name of the TypeVar, to make it slightly more obvious what's happening, and give it bound=DataclassType. (The TypeVar for BaseModel in pydantic.main is called Model; I think that's nice.)

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 14, 2019

As far as I can tell, we're just waiting for a tweak to the signature of __call__. Is that correct?

If not, let me know and I'll merge.

@koxudaxi

This comment has been minimized.

Copy link
Contributor Author

koxudaxi commented Nov 14, 2019

@samuelcolvin
Sorry, I have pushed it which is changed by @dmontagu's review

@samuelcolvin samuelcolvin merged commit 33b3dc1 into samuelcolvin:master Nov 14, 2019
11 checks passed
11 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 caa270a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20191114.3 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
samuelcolvin.pydantic (Job Python38) Job Python38 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 14, 2019

Woop woop. Thank you very much.

@koxudaxi koxudaxi deleted the koxudaxi:mypy_plugin_support_for_dataclasses branch Nov 14, 2019
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.