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

fix(type hints): improve type hints in ibis.common #6867

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented Aug 14, 2023

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:

  • Subclasses of Annotable create Attributes 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 gives error: Too many arguments for "MapSet" [call-arg] errors. Should MapSet be an abstract base class with abstract methods and/or define __init__?

@kszucs
Copy link
Member

kszucs commented Aug 14, 2023

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?

ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/typing.py Outdated Show resolved Hide resolved
@binste binste force-pushed the improve_type_hints_in_common branch from 1e4d4d6 to 7685b03 Compare August 15, 2023 06:00
@binste
Copy link
Contributor Author

binste commented Aug 15, 2023

Rebased on master.

ibis/common/annotations.py Outdated Show resolved Hide resolved
ibis/common/annotations.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
@@ -755,6 +789,7 @@ class TypeOf(Slotted, Pattern):
"""Pattern that matches a value that is of a given type."""

__slots__ = ("type",)
type: type
Copy link
Member

@kszucs kszucs Aug 15, 2023

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]

ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
ibis/common/patterns.py Outdated Show resolved Hide resolved
Copy link
Member

@kszucs kszucs left a 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?

@binste binste force-pushed the improve_type_hints_in_common branch from 60a9e67 to 6d698c8 Compare August 15, 2023 18:54
@binste
Copy link
Contributor Author

binste commented Aug 15, 2023

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


Found 140 errors in 30 files (checked 27 source files)

with mypy ibis/common --ignore-missing-imports but I'm also missing a few stub packages.

@binste binste force-pushed the improve_type_hints_in_common branch from 6d698c8 to 1aa1e20 Compare August 15, 2023 18:57
@kszucs
Copy link
Member

kszucs commented Aug 16, 2023

I installed the stub packages, so my number is probably lower due to those.

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

Great! Thanks @binste!

@kszucs kszucs merged commit ff00347 into ibis-project:master Aug 16, 2023
87 checks passed
@binste
Copy link
Contributor Author

binste commented Aug 17, 2023

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 mypy --install-types.

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.

2 participants