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
base: master
Are you sure you want to change the base?
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 62 62
Lines 6871 6930 +59
Branches 1098 1098
=========================================
+ Hits 6871 6930 +59 ☔ 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
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.