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

Upgrade to latest mypy #618

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Upgrade to latest mypy #618

merged 3 commits into from
Dec 20, 2022

Conversation

achidlow
Copy link
Contributor

PEP-484 has changed to recommend against implicit optionals based on default arguments, and mypy has accordingly flipped the default in a recent release.

ref: https://peps.python.org/pep-0484/#union-types

PEP-484 has changed to recommend against implicit optionals based on default arguments, and mypy has accordingly flipped the default in a recent release.

ref: https://peps.python.org/pep-0484/#union-types
@CLAassistant
Copy link

CLAassistant commented Dec 19, 2022

CLA assistant check
All committers have signed the CLA.

@barnjamin
Copy link
Contributor

@tzaffi I recall seeing a recent commit from you switching the thing | None type hint to Optional[thing], was there a specific reason for that?

@tzaffi
Copy link
Contributor

tzaffi commented Dec 19, 2022

@tzaffi I recall seeing a recent commit from you switching the thing | None type hint to Optional[thing], was there a specific reason for that?

These are semantically equivalent. I think we ought to come to an agreement about what style we ought to adopt going forward -so we don't need to spend any mental calories on it. I propose:

  • use Optional[TheType] instead of TheType | None
  • Use TheFirstType | TheSecondType instead of Union[TheFirstType, TheSecondType]

Why? I don't really have a good reason. I could be easily swayed to another viewpoint. But we should adopt some style.

@@ -2,7 +2,7 @@ black==22.3.0
flake8==5.0.4
flake8-tidy-imports==4.6.0
graviton@git+https://github.com/algorand/[email protected]
mypy==0.950
mypy==0.991
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@barnjamin
Copy link
Contributor

@achidlow noted that the use of Union/Optional are required if the type must be quoted

@tzaffi
Copy link
Contributor

tzaffi commented Dec 19, 2022

@achidlow noted that the use of Union/Optional are required if the type must be quoted

That's an argument for not using | at all.

mypy.ini Show resolved Hide resolved
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Thanks for upgrading mypy!

I had a small nit regarding the Makefile.

Also, I won't hold up the PR on account of this, but I created #619 and suggested that we disallow | for annotations going forward.

@achidlow
Copy link
Contributor Author

@tzaffi pending the outcome of #619, I've at least made this PR internally consistent in the use of | over Optional. If you'd prefer it the other way around for now I'm happy to flip that for all the methods changed in this PR.

mypy:
mypy --show-error-codes $(MYPY)
mypy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why drop the --show-error-codes arg? It's been handy on occasion.

Copy link
Contributor Author

@achidlow achidlow Dec 19, 2022

Choose a reason for hiding this comment

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

Not sure if this used to be flipped in a previous mypy version, but that's the default and assumed. You have to disable it with --hide-error-codes

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@tzaffi tzaffi merged commit 6f28661 into algorand:master Dec 20, 2022
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.

4 participants