-
-
Notifications
You must be signed in to change notification settings - Fork 948
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
feat: type more falcon modules #2171
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2171 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 63 63
Lines 6849 6889 +40
Branches 1260 1260
=========================================
+ Hits 6849 6889 +40 ☔ View full report in Codecov by Sentry. |
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.
Great work.
- "from future annotation" is missing from a few modules
- in general I'm not a fan of the type alias union that include None, like RawHeaders for example, since it's a bit strange to read
headers: RawHeaders = None
. That said it's my preference, as they are it's also ok - i'm not convinced we need the new modules that were added, it seems that we could just use the existing typing one. But I think it's better to wait the input of @vytas7 for this one
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.
thanks for the update, I've repried to the open messages.
I'll try taking a better look locally to se if we can avoid some of the circular imports
Thanks @copalco for the update, I'll try taking a look by the weekend! |
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.
Thanks for the updates. In general this looks mostly ok.
While it works on definition when using future annotation, I'm not sure if it has unintended consequences at runtime for older python
I've pushed an update to avoid the imports in the functions, using |
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.
Looks great, it's a massive chunk of work (bar a couple of nitpicks)!
I'm just not 100% convinced by these new base classes inside typing.py
🤔 Do we really need these? It would be easier to get this review merged if it did not introduce any changes to the codebase's behaviour/performance (which in turn needs more extensive testing), just annotations.
Or maybe we could split these into a separate PR?
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.
Thanks for your help on this! Retrofitting type hints is a lot of work and we really appreciate it. I still need to finish reviewing several of the modules, but wanted to share my comments so far to get you unblocked.
@copalco let us know if you plan on continuing this or if we should take this over the finishing lie. In any case thanks a lot for the effort on this! |
Sure I can continue. Also I don't mind if you will just propose the actual change as a comment. |
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.
thanks for updating it
a27ea3a
to
b2b94d8
Compare
# Conflicts: # falcon/errors.py # falcon/hooks.py # falcon/inspect.py # falcon/middleware.py # falcon/response.py
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.
Thanks, this looks like a really great start toward better typing! 💯
The only part that looks slightly inconsistent, or at least I don't understand why it needs to be done this way, is typing of media Handlers
in request.py
, response.py
, and media/
itself.
Will look |
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 now
Summary of Changes
Replace this text with a high-level summary of the changes included in this PR.
Related Issues
Please reference here any issue #'s that are relevant to this PR, or simply enter "N/A" if this PR does not relate to any existing issues.
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
docs/
.docs/
.versionadded
,versionchanged
, ordeprecated
directives.docs/_newsfragments/
, with the file name format{issue_number}.{fragment_type}.rst
. (Runtowncrier --draft
to ensure it renders correctly.)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.