-
Notifications
You must be signed in to change notification settings - Fork 792
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
fix(type): make some decorator utility functions type-safe and add some type annotations #1785
Conversation
…me type annotations
Hello @yetone! Thanks for opening this PR.
Do see the Hitchhiker's guide to code style |
from urllib.parse import urlparse, uses_netloc, uses_params, uses_relative | ||
|
||
from google.protobuf.message import Message | ||
from mypy.typeshed.stdlib.contextlib import _GeneratorContextManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means adding mypy into our library dependency. I would rather to reserve all typeshed
import under TYPE_CHECKING
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to add the relevant dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries since its only a type call i just added to the files. I think we shouldn't add mypy to library dependency anw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I don't know if its a good practice to include mypy into library dependency. I don't see a lot of other library doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to do it, I'll research how other libraries are doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I will fix the CI related in a separate PR |
We should revisit sometimes once we decide whether to go to 3.10 or 3.9 as default |
…me type annotations (bentoml#1785)
Description
Some internal decorator utility functions are not type-safe now, for example:
before:
after:
I fixed the error above in this PR, but some decorators like
cached_contextmanager
andcatch_exceptions
are still not type-safe, this problem that relies on this issue: python/mypy#8645Motivation and Context
How Has This Been Tested?
Checklist:
./dev/format.sh
and./dev/lint.sh
script have passed(instructions).