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

Mypy Typing Migration and Some typo fixes #299

Merged
merged 17 commits into from
Jan 8, 2025

Conversation

emersodb
Copy link
Collaborator

@emersodb emersodb commented Nov 28, 2024

PR Type

Other

Short Description

Note: This PR touches a lot of files. However, it does not change any of the actual library functionality.

There are likely too many files for anyone to review. I'd say spot review and to get the gist of what's being changed, but we can probably rely on the tests and mypy here to confirm that everything is working fine.

The goal of this PR is two fold. First, it is to move our library away from using the from typing import ... to the built in types and collections abcs that now underpin the modern mypy types that can be used in Python 3.10 and above. See some of the documentation here

For example, this means that we're using list[str], tuple[int, int], tuple[int, ...], dict[str, int], type[C] as built-in types (no need to import them anymore) and Iterable[int], Sequence[bool], Mapping[str, int], Callable[[...], ...] from collections.abc (as recommended by mypy now).

We are also moving to the new Optional and Union specification style:
Optional[typing_stuff] -> typing_stuff | None
Union[typing1, typing2] -> typing1 | typing2
Optional[Union[typing1, typing2]] -> typing1 | typing2 | None

Finally, I also corrected a number of typos throughout the library.

Tests Added

No tests have been added. However, there is a new script that is run by precommit that checks whether any of the legacy mypy types are being imported. This will ensure that we don't regress to the older styles in any of our files.

@emersodb emersodb marked this pull request as ready for review November 28, 2024 23:08
Copy link
Collaborator

@scarere scarere left a comment

Choose a reason for hiding this comment

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

I think its safe for me to just trust that you did all these typing changes correctly. As an additional suggestion, is there a way to get mypy to check whether any of the deprecated types were used? Would be useful to add this to the precommit checks to prevent someone from having to go through all these files manually again. Alternatively, we could just update the contributing readme with a note to not use deprecated types from the typing module and to prefer | over typing.Union and typing.Optional . Actually it would be cool if we could check for the use of Union or Optional in the precommit checks as well. Even cooler if we could add an autofix for it and the other deprecated types.

@emersodb
Copy link
Collaborator Author

emersodb commented Dec 3, 2024

I think its safe for me to just trust that you did all these typing changes correctly. As an additional suggestion, is there a way to get mypy to check whether any of the deprecated types were used? Would be useful to add this to the precommit checks to prevent someone from having to go through all these files manually again. Alternatively, we could just update the contributing readme with a note to not use deprecated types from the typing module and to prefer | over typing.Union and typing.Optional . Actually it would be cool if we could check for the use of Union or Optional in the precommit checks as well. Even cooler if we could add an autofix for it and the other deprecated types.

@scarere: I've created a custom script to identify when we are regressing to the legacy types of mypy. This will force an update to the new style upon PR creation or running pre-commit locally. Autofixing this will be a little trickier. So we'll go with this more simple script approach first.

Copy link
Collaborator

@scarere scarere left a comment

Choose a reason for hiding this comment

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

Couple more nitpicky comments but pretty cool that we can catch these in the pre-commits

mypy_disallow_legacy_types.py Show resolved Hide resolved
mypy_disallow_legacy_types.py Show resolved Hide resolved
@@ -46,6 +46,14 @@ repos:
- id: nbqa-flake8
- id: nbqa-mypy

- repo: local
hooks:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool :)

Copy link
Collaborator

@jewelltaylor jewelltaylor 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! Glad you added the pre-commit check because I am so used to using types from Typing I would definitely forget sometimes

Base automatically changed from dbe/some_server_side_checkpointer_consolidation to main January 8, 2025 16:34
@emersodb emersodb merged commit 8efdb31 into main Jan 8, 2025
6 checks passed
@emersodb emersodb deleted the dbe/migrate_some_mypy_typing branch January 8, 2025 17:42
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