-
Notifications
You must be signed in to change notification settings - Fork 387
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
docs: kick start the process of adding type definitions #684
base: master
Are you sure you want to change the base?
docs: kick start the process of adding type definitions #684
Conversation
967405d
to
3a9cfdb
Compare
a482057
to
bcc5602
Compare
Hi @a-ungurianu, Awesome that you started that work!! Full disclosure, I haven't reviewed all the changes but one thing I wanted to mention is trying to minimize imports at runtime. In most of my projects, i am able to only import from typing import TYPE_CHECKING
# ... other imports
if TYPE_CHECKING: # pragma: nocover
from typing import...
from typing_extensions import...
T = TypeVar(...) I have seen numerous other projects doing that as well. What do you think? |
bcc5602
to
723d8a8
Compare
I think I agree. I've just been adding any new modules inside the |
16f1829
to
755c416
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #684 +/- ##
==========================================
- Coverage 96.81% 96.19% -0.62%
==========================================
Files 27 27
Lines 3549 3628 +79
==========================================
+ Hits 3436 3490 +54
- Misses 113 138 +25 ☔ View full report in Codecov by Sentry. |
1ee35e3
to
9faa0ca
Compare
fc13d08
to
5c7e1d6
Compare
5c7e1d6
to
8a231b4
Compare
4298ad8
to
9efd2ac
Compare
9efd2ac
to
e994541
Compare
This is ready for a proper review now. The history log in the PR is a bit of a mess for this due to me fighting with Hound and with making sure the stuff used from |
@a-ungurianu any chance this would get merged? |
I'm considering declaring bankruptcy on this PR and re-openning so that it looks a bit more approachable for review. Also, if there's anything in the structure that makes it hard to review, lemme know and I can amend |
Would it be possible to exclude the |
Beginnings of #647
Why is this needed?
Python type annotations are a great way of documenting the APIs, as well as improving the developer experience when using this library.
I find the actual type checking to be an added benefit than a necessity.
Proposed Changes
Does this PR introduce any breaking change?