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 annotations and py.typed to conform with PEP561. Add type checking to tox #40

Merged
merged 3 commits into from
May 31, 2020

Conversation

keiclone
Copy link
Contributor

@keiclone keiclone commented May 26, 2020

*Issue number of the reported bug or feature request: #24

Describe your changes
made attrs-strict PEP561 compatible by adding annotations and py.typed. This removes the missing import error from mypy when attempting to run static type checking on packages using attrs-strict

Testing performed
Was able to successfully run static type checking in a test project after installing attrs-strict from my branch.

when testing on a separate library:

# test.py
@attr.s
class FooClass(object):
    list_of_numbers = attr.ib(validator=type_validator(), type=List[int])


fc = FooClass([1, 2, 3, "four"])
# before changes
$ pipenv install attrs-strict
Installing git+https://github.com/bloomberg/attrs-strict
...
$ mypy test.py
error: Cannot find implementation or library stub for module named 'attrs_strict'
note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
error: List item 3 has incompatible type "str"; expected "int"

# after changes
$ pipenv install git+https://github.com/keiclone/attrs-strict#egg=attrs-strict
Installing git+https://github.com/keiclone/attrs-strict#egg=attrs-strict
...
$ mypy test.py
error: List item 3 has incompatible type "str"; expected "int"

Additional context
could use some extra attention on how this was added to tox to make sure it was done correctly

@keiclone keiclone force-pushed the types-merge-candidate branch from 60543ae to fd0f1d3 Compare May 26, 2020 08:04
@keiclone keiclone changed the title Add type annotations, stubs and py.typed. Add type checking to tox Add annotations and py.typed to conform with PEP561. Add type checking to tox May 26, 2020
@keiclone keiclone force-pushed the types-merge-candidate branch from fd0f1d3 to db18fd3 Compare May 26, 2020 08:08
@keiclone
Copy link
Contributor Author

@gaborbernat sorry for opening a new PR, I borked my rebase process and figured it'd be easier/less risky to start over.

One thing to note is that it seems mypy seems to have some strange behavior when we do things like compare typing.Type[Any] to typing.Union, specifically a "Nonoverlapping comparison". Seems like an edge case that we can ignore for now.

I think I addressed all the comments in #31. Could you please take a look?

@keiclone keiclone force-pushed the types-merge-candidate branch from db18fd3 to cd821f4 Compare May 26, 2020 08:13
MANIFEST.in Outdated Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
attrs_strict/_type_validation.py Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@keiclone keiclone force-pushed the types-merge-candidate branch 3 times, most recently from a7d9b7d to 777692c Compare May 29, 2020 19:15
keiclone and others added 3 commits May 31, 2020 09:28
Signed-off-by: Wilfred Wong <[email protected]>
Signed-off-by: Bernat Gabor <[email protected]>
Signed-off-by: Bernat Gabor <[email protected]>
Signed-off-by: Bernat Gabor <[email protected]>
@gaborbernat gaborbernat force-pushed the types-merge-candidate branch from 777692c to 118f266 Compare May 31, 2020 09:23
@gaborbernat gaborbernat self-requested a review May 31, 2020 09:27
@gaborbernat gaborbernat requested a review from erikseulean May 31, 2020 09:27
base_type = _get_base_type(expected_type)

if base_type is None or base_type == typing.Any:
if base_type == typing.Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the none check dropped here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure to be fair; I assume given the type checker does not complain it's not needed? @keiclone knows probably better

Copy link
Contributor

@erikseulean erikseulean left a comment

Choose a reason for hiding this comment

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

Looks good, just the comment about the None check.

@keiclone
Copy link
Contributor Author

The None check was effectively moved up to line 69. _get_base_type only takes in and returns typing.Type types.

Copy link
Contributor

@erikseulean erikseulean left a comment

Choose a reason for hiding this comment

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

Thanks for the effort put into this @keiclone . Much appreciated.

@erikseulean erikseulean merged commit 79bfc60 into bloomberg:master May 31, 2020
@gaborbernat
Copy link
Contributor

@erikseulean want to do a new release?

@erikseulean
Copy link
Contributor

Yes, feel free to trigger a new one.

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