Skip to content

MNT: Upgrade mypy to version 1.0.0#297

Merged
adrinjalali merged 2 commits intoskops-dev:mainfrom
BenjaminBossan:MNT-upgrade-mypy-1.0.0
Feb 28, 2023
Merged

MNT: Upgrade mypy to version 1.0.0#297
adrinjalali merged 2 commits intoskops-dev:mainfrom
BenjaminBossan:MNT-upgrade-mypy-1.0.0

Conversation

@BenjaminBossan
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

ready for review @skops-dev/maintainers

Copy link
Copy Markdown
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 thread skops/card/_model_card.py
Comment on lines +23 to +26
if sys.version_info >= (3, 11):
from typing import Self
else:
from typing_extensions import Self
Copy link
Copy Markdown
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
Copy Markdown
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