Add type hints for ._app #359
Conversation
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
=======================================
Coverage 98.35% 98.36%
=======================================
Files 46 46
Lines 3892 3906 +14
Branches 256 256
=======================================
+ Hits 3828 3842 +14
Misses 46 46
Partials 18 18
Continue to review full report at Codecov.
|
|
@wsanchez It looks there aren't changes here anymore? |
|
@twm Well that's surprising… |
|
@twm: fixed |
…ce they are useful for client type signatures.
There was a problem hiding this comment.
Thanks for the additional types!
- On the
KleinRequestthing, please just file a separate issue to follow up on that so we don't forget; we really don't need to block on it. - On the
castthing, I do feel like anywhere we have atype: ignoreor acastwe should really include a comment explaining in English why we know it's safe, but don't feel like you have to block on the refactoring; just add the comment in both spots or add one comment pointing at the other if refactoring them is at all tricky.
Thanks for continuing to improve Klein's type discipline!
| @@ -20,6 +31,9 @@ | |||
|
|
|||
| __all__ = ( | |||
| "Klein", | |||
| "KleinErrorHandler", | |||
There was a problem hiding this comment.
Great to have some sensible public names for these!
src/klein/_app.py
Outdated
| ): | ||
| # type: (...) -> Text | ||
| assert self.mapper is not None | ||
| return cast( |
There was a problem hiding this comment.
Why do we know for sure that this cast is valid?
There was a problem hiding this comment.
Ideally, Werkzeug will add types in the future.
In the meantime, I guess we hope the docs showing strings in all examples means it's returning strings all the time
There was a problem hiding this comment.
Oh wait is this a cast to narrow? I thought it was a cast away from some other type, not Any
There was a problem hiding this comment.
If it's just to narrow I'd say you could stkip it, since the -> Text will fix it anyway?
Type hints for
._app.py