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

MNT: Upgrade mypy to version 1.0.0 #297

Merged
merged 2 commits into from
Feb 28, 2023

Conversation

BenjaminBossan
Copy link
Collaborator

The upgrade worked out of the box. I made a small change, however, to adjust the signatures of Card to return the Self type, since this is now correctly recognized by mypy.

Below is an example to illustrate why it's useful:

class Card1:
    def add(self) -> Card1:
        return self

class MyCard1(Card1):
    pass

reveal_type(MyCard1().add())  # returns Card1 but should be MyCard1

class Card2:
    def add(self) -> Self:
        return self

class MyCard2(Card2):
    pass

reveal_type(MyCard2().add())  # returns MyCard2

Another advantage of upgrading is that mypy 1.0.0 should be faster. More in this post.

Note that pre-commit config has to be updated for this change as well.

The upgrade worked out of the box. I made a small change, however, to
adjust the signatures of Card to return the Self type, since this is now
supported by mypy.

Below is an example to illustrate why it's useful:

class Card1:
    def add(self) -> Card1:
        return self

class MyCard1(Card1):
    pass

reveal_type(MyCard1().add())  # returns Card1 but should be MyCard1

class Card2:
    def add(self) -> Self:
        return self

class MyCard2(Card2):
    pass

reveal_type(MyCard2().add())  # returns MyCard2

Note that pre-commit config has to be updated for this change as well.
@BenjaminBossan
Copy link
Collaborator Author

ready for review @skops-dev/maintainers

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

Comment on lines +23 to +26
if sys.version_info >= (3, 11):
from typing import Self
else:
from typing_extensions import Self
Copy link
Member

Choose a reason for hiding this comment

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

is this another instance where mypy doesn't like it to be in fixes.py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is indeed annoying.

@adrinjalali adrinjalali merged commit 6c7240c into skops-dev:main Feb 28, 2023
@BenjaminBossan BenjaminBossan deleted the MNT-upgrade-mypy-1.0.0 branch February 28, 2023 12:52
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