-
Notifications
You must be signed in to change notification settings - Fork 592
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
fix(type hints): improve type hints in ibis.common #6867
fix(type hints): improve type hints in ibis.common #6867
Conversation
Thanks for pulling out into a separate PR! I need to play with it locally before I can answer the questions. In the meantime, can you please rebase? |
1e4d4d6
to
7685b03
Compare
Rebased on master. |
@@ -755,6 +789,7 @@ class TypeOf(Slotted, Pattern): | |||
"""Pattern that matches a value that is of a given type.""" | |||
|
|||
__slots__ = ("type",) | |||
type: type |
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.
We will probably need to rename these (in a follow-up) properties due to:
ibis/common/patterns.py:1226: error: Variable "ibis.common.patterns.SequenceOf.type" is not valid as a type [valid-type]
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.
Before: Found 85 errors in 20 files (checked 27 source files)
After: Found 50 errors in 15 files (checked 27 source files)
Solid improvement already!
There are a couple of nits left, could you please squash the commits afterwards?
60a9e67
to
6d698c8
Compare
Thanks for the inputs and great to see the progress! Implemented all requested changes, rebased again on master and squashed commits down to one. What mypy command did you run that only gave you 50 errors? I get
with |
6d698c8
to
1aa1e20
Compare
I installed the stub packages, so my number is probably lower due to those. |
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.
Great! Thanks @binste!
Hmm seems like we have a different setup. If I install sub packages, the errors explode to 987 errors. I followed the instructions for conda on Mac at https://ibis-project.org/community/contribute/01_environment/#support-matrix and then ran |
As suggested by @kszucs in #6862 (comment), I copied all type hint changes from #6852 into this PR. I also fixed a few more mypy errors but this PR does not yet fix all in
ibis.common
. I think I'm not familiar enough with Ibis to suggest changes for the remaining ones but happy to brainstorm and review!Some but not all outstanding points:
Annotable
createAttributes without a default cannot follow attributes with one
errors. See bug: Incomplete type hints #6844 (comment)MapSet
does not take any input arguments but methods such as__and__
pass arguments to class (self.__class__(intersection)
) which giveserror: Too many arguments for "MapSet" [call-arg]
errors. ShouldMapSet
be an abstract base class with abstract methods and/or define__init__
?