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): remove notimplemented as type hints as not valid #6854

Merged

Conversation

binste
Copy link
Contributor

@binste binste commented Aug 12, 2023

NotImplemented is an instance and the actual type is NotImplementedType. The mypy errors which this fixes look like this:

...
ibis/expr/types/numeric.py:711: error: Variable "builtins.NotImplemented" is not valid as a type  [valid-type]
ibis/expr/types/numeric.py:711: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
...

As mentioned in #6844, I can create a few more of these smaller PRs or I can bundle more changes into a single PR. Whatever works best for you.

Edit: NotImplementedType was introduced in Python 3.10 so tests fail for 3.9. I'll look into it.

@binste binste marked this pull request as draft August 12, 2023 14:49
@binste binste marked this pull request as ready for review August 12, 2023 14:50
@binste
Copy link
Contributor Author

binste commented Aug 13, 2023

In Python < 3.10, NotImplemented cannot be typed at all as mypy does not accept type(NotImplemented) as a type hint. Apparently it is in general unclear if and how to type hint NotImplemented, see python/mypy#363.

Based on some testing, this SO thread, and looking into other libraries such as pandas-stubs, my understanding is that newer versions of mypy work well if NotImplementedType is not provided as a return type but instead only the type of the object that is returned if the operation was successful.

@binste binste force-pushed the replace_notimplemented_with_type branch from c2fa860 to 1d1176d Compare August 13, 2023 08:06
@binste binste changed the title fix(type hints): In type hints replace NotImplemented with NotImplementedType fix(type hints): Remove NotImplemented as type hints as not valid Aug 13, 2023
@binste binste force-pushed the replace_notimplemented_with_type branch from 1d1176d to e2b818d Compare August 13, 2023 08:35
@cpcloud cpcloud added this to the 7.0 milestone Aug 13, 2023
@cpcloud cpcloud added bug Incorrect behavior inside of ibis ecosystem External projects or activities labels Aug 13, 2023
@binste binste changed the title fix(type hints): Remove NotImplemented as type hints as not valid fix(type hints): remove notimplemented as type hints as not valid Aug 13, 2023
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you :)

@cpcloud cpcloud merged commit 57ea7a1 into ibis-project:master Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis ecosystem External projects or activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants