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

Remove type comment in favor of PEP 526 for variable annotation #2538

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

merelcht
Copy link
Member

Description

Addresses #2078

Development notes

The problem found in #2078 was caused by the following: PyCQA/pyflakes#747 (comment) Essentially, flake8 doesn't support type checking in comments anymore with the # type: ... syntax. Instead it now looks for the PEP 526 syntax for variable annotation: https://peps.python.org/pep-0526/

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@merelcht merelcht requested a review from idanov as a code owner April 25, 2023 15:51
@merelcht merelcht self-assigned this Apr 25, 2023
@merelcht merelcht linked an issue Apr 25, 2023 that may be closed by this pull request
@merelcht merelcht requested review from astrojuanlu and noklam and removed request for idanov April 25, 2023 15:52
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks @merelcht! I was always a fan of type: comments, I guess Python is solidly heading towards types as annotations...

Some questions:

  • What's the role of flake8 in typing? Does it replace MyPy, or it only checks that the annotations are correct?
  • May I suggest using import typing as t? (Silly personal preference, but I think it's nice to avoid polluting the global namespace with typing stuff, and instead I prefer to do t.Any, t.TypedDict and such)
  • May I suggest adding from __future__ import annotations to leverage PEP 585 and hence be able to do list[str] instead of t.List[str]? (Available from Python 3.7)
  • Where it makes sense for this PR (without having to overhaul the whole codebase) may I suggest adding from __future__ import annotations to leverage PEP 604 and hence use str | None instead of t.Optional[str] and str | bytes instead of t.Union[str, bytes]?

@merelcht
Copy link
Member Author

Some questions:

  • What's the role of flake8 in typing? Does it replace MyPy, or it only checks that the annotations are correct?

I think we've always had flake8 on Kedro, and never fully leveraged MyPy to its full abilities, see #2156. For this specific issue flake8 was checking if all imports are being used in the code and before this change it was failing any imports that were only used in type: comments.

  • May I suggest using import typing as t? (Silly personal preference, but I think it's nice to avoid polluting the global namespace with typing stuff, and instead I prefer to do t.Any, t.TypedDict and such)

Is this common practice? I'm not sure I like it 😄 haha! Perhaps we can make a follow up task with several PEP update suggestions and just vote as a team?

  • May I suggest adding from __future__ import annotations to leverage PEP 585 and hence be able to do list[str] instead of t.List[str]? (Available from Python 3.7)

This one I do like!

  • Where it makes sense for this PR (without having to overhaul the whole codebase) may I suggest adding from __future__ import annotations to leverage PEP 604 and hence use str | None instead of t.Optional[str] and str | bytes instead of t.Union[str, bytes]?

Is this one also available from Python 3.7?

@astrojuanlu
Copy link
Member

I see, thanks! Okay with keeping from typing import *, I had to try 😸

About PEP 604, I'm not sure, it's not clearly written in the PEP 🤔

@merelcht
Copy link
Member Author

I'll create a new PR to leverage PEP 585.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

LGTM!

@merelcht merelcht requested a review from deepyaman April 26, 2023 13:57
@noklam noklam changed the title Use PEP 526 for variable annotation Remove type comment in favor of PEP 526 for variable annotation Apr 26, 2023
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@merelcht merelcht merged commit 9aa75b1 into main Apr 26, 2023
@merelcht merelcht deleted the mypy-issue branch April 26, 2023 16:23
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.

Follow up and investigate the Linting Error
3 participants