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

Add type to more util modules #1587

Merged
merged 2 commits into from
Mar 29, 2024
Merged

Conversation

savarin
Copy link
Contributor

@savarin savarin commented Mar 24, 2024

Describe your changes

This PR adds mypy types to more _utils modules, as per #1372.

modal/_utils/hash_utils.py
modal/_utils/mount_utils.py
modal/_utils/rand_pb_testing.py
modal/_utils/shell_utils.py


def generator(rand):
return rand.choice(enum_values)
generator = lambda rand: rand.choice(enum_values) # noqa: E731
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@freider I added a ruff ignore here to avoid having a def, which requires a function type signature and conflicts vs line 64.

@savarin savarin force-pushed the include-types-4 branch 2 times, most recently from 7ad5e06 to f0aaa9b Compare March 28, 2024 02:11
@savarin
Copy link
Contributor Author

savarin commented Mar 28, 2024

@freider Let me know if you have comments, thanks!

Copy link
Contributor

@mwaskom mwaskom left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -12,6 +12,7 @@ def _update(hashers, data: Union[bytes, IO[bytes]]):
for hasher in hashers:
hasher.update(data)
else:
assert isinstance(data, IO)
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd that this is necessary!

Copy link
Contributor

Choose a reason for hiding this comment

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

It turns out to be microsoft/pyright#5697

Also this broke our CI once it landed which is weird because it looks like it passed on the branch 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mwaskom My bad for delay, had been deep in interview-land 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I got to learn some interesting stuff about typing edge cases :D

@mwaskom mwaskom merged commit 310ae91 into modal-labs:main Mar 29, 2024
3 checks passed
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