Skip to content

Mypy improvement #677

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

Closed
wants to merge 3 commits into from
Closed

Conversation

Secrus
Copy link
Collaborator

@Secrus Secrus commented Nov 23, 2022

Pull Request Check List

  • Added tests for changed code. Not needed
  • Updated documentation for changed code. No changes require documenting

Mypy 0.990 added show_error_codes as enabled by default and our # type: ignore comments had wrong codes which caused a failure in #674.

Second commit moves mypy usage away from pre-commit, since when it's being run from pre-commit, it might miss some typing issues coming from dependencies. See python-poetry/cleo#254 (comment)



if sys.version_info < (3, 8):
from typing_extensions import Literal
Copy link
Contributor

@bryanforbes bryanforbes Nov 28, 2022

Choose a reason for hiding this comment

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

Aliasing things like Literal is not supported in pyright. I'll add a comment where Literal is being used to show how it should be imported to prevent typing_extensions from becoming a runtime dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are only supporting mypy check in our pipelines right now and I stick to how mypy works (never worked with pyright, but it sounds like it's not compatible with mypy).

Copy link
Contributor

Choose a reason for hiding this comment

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

pyright and mypy work pretty hard to be compatible with one another. pyright does an early scan for some special forms to speed things up, and the Literal import is one of those. This is one of the few differences (aside from pyright supporting more PEPs) between pyright and mypy. The other thing to take into account is that pyright is the type checker that is included by default with Visual Studio Code's Python extension.


if sys.version_info < (3, 8):
from typing_extensions import Literal
from typing_extensions import SupportsIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes typing_extensions a runtime dependency. Is that desired?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not 😆. I will need to rework this PR anyway since typing fails depending on the version of Python it's run on.

@@ -41,7 +41,7 @@
from pendulum.utils._compat import PY38

if TYPE_CHECKING:
from typing import Literal
from pendulum.utils._compat import Literal
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be changed to from typing_extensions import Literal so that pyright is supported

@@ -19,7 +19,7 @@
from pendulum.mixins.default import FormattableMixin

if TYPE_CHECKING:
from typing import Literal
from pendulum.utils._compat import Literal
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be changed to from typing_extensions import Literal so that pyright is supported

@Secrus
Copy link
Collaborator Author

Secrus commented May 10, 2023

Closing in favor of #680

@Secrus Secrus closed this May 10, 2023
@Secrus Secrus deleted the mypy-improvement branch May 10, 2023 16:24
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