Conversation
|
Handled errors reported from
Here are some important observations to be aware of and to consider more carefully: These errors refer to This function is wrapped in another function via the ~ While I tried to avoid using Note ~ Added I was unsure of how to address this without rewriting the function, so I decided to ~ I was unsure of how to handle this one, so I used ~ I was unsure of how to handle these without mucking up the imports, so I set the type to I was unsure of how to handle this without mucking up the imports, so I used ~ I was unsure if we should use a cast on these, so I used ~ I was unsure of how to fix this, so I used ~ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2599 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7962 7970 +8
Branches 1101 1101
=========================================
+ Hits 7962 7970 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| def create_app() -> falcon.asgi.App: | ||
| def create_app() -> falcon.asgi.App: # type: ignore[type-arg] |
There was a problem hiding this comment.
We shouldn't need the ignore here, we can just parametrize as intended: -> falcon.asgi.App[Request, Response].
There was a problem hiding this comment.
Done (marking for my own reference).
| # that case. | ||
| try: | ||
| return self.scope['root_path'] | ||
| return '{}'.format(self.scope['root_path']) |
There was a problem hiding this comment.
These are affecting performance for no good reason in a hot code path.
If in doubt, we simply need to ignore, or use a more advanced scope typing where scope[root_path']` is typed as string.
Edit: we can probably just copy these or implement something similar: https://github.com/django/asgiref/blob/main/asgiref/typing.py#L66.
There was a problem hiding this comment.
How would you feel about using ignore at the moment, and marking a better long-term solution as a new issue?
This way, the broader PR can be pushed through, then we can give this specific item a proper, focused consideration?
There was a problem hiding this comment.
That works for me. We can create a follow-up issue, and comment these lines with TODO(0xMattB): (explain what will be improved <...>).
| raise errors.PayloadTypeError('Missing TEXT (0x01) payload') | ||
|
|
||
| return text | ||
| return '{}'.format(text) |
There was a problem hiding this comment.
Again, undo these in a hot code path.
There was a problem hiding this comment.
Done (marking for my own reference).
| raise errors.PayloadTypeError('Missing BINARY (0x02) payload') | ||
|
|
||
| return data | ||
| return bytes(data) |
There was a problem hiding this comment.
Done (marking for my own reference).
| def status_code(self) -> int: | ||
| """HTTP status code normalized from :attr:`status`.""" | ||
| return http_status_to_code(self.status) | ||
| return int(http_status_to_code(self.status)) |
There was a problem hiding this comment.
These shouldn't be needed either. Does http_status_to_code always return an int?
There was a problem hiding this comment.
It appears so from the code, but I believe something with the decorator is throwing this off.
falcon/http_status.py:70: error: Returning Any from function declared to return "int" [no-any-return]
See first observation in my above post.
Edit: Or maybe it's too ambiguous for mypy --strict, I'm honestly not exactly sure.
There was a problem hiding this comment.
Ah, right, it's coming from these LRU cache decorators... I'll take a look tomorrow... err later today 💤
|
Thanks a lot for this effort so far, it looks great overall! 💯 |
vytas7
left a comment
vytas7
left a comment
There was a problem hiding this comment.
I took a quick look at the things LRU, it could potentially be improved slightly.
| # the nocover pragma here. | ||
| def _lru_cache_nop(maxsize: int) -> Callable[[Callable], Callable]: # pragma: nocover | ||
| def decorator(func: Callable) -> Callable: | ||
| def _lru_cache_nop( |
There was a problem hiding this comment.
Probably we could use ParamSpec to improve typing of this?
I still had to use one cast(...), because it seems lru_cache is, itself, not fully typing-friendly.
But that is done once here, not in the places where the function is called.
For me, the below snippet passes with --strict:
import functools
from typing import cast, Callable, ParamSpec, TypeVar
_P = ParamSpec('_P')
_T = TypeVar('_T')
def my_cache(func: Callable[_P, _T]) -> Callable[_P, _T]:
if func.__name__.startswith('fast'):
return func
cache = functools.lru_cache(maxsize=8)
return cast(Callable[_P, _T], cache(func))
@my_cache
def parse_int(number: str) -> int:
if number == 'zero':
return 0
if number == 'one':
return 1
if number == 'two':
return 2
if number == 'three':
return 3
return int(number)
def test() -> None:
assert parse_int('two') == 2
assert parse_int('3') == 3
if __name__ == '__main__':
test()
|
Update 01/23/26: I had saved the list of original errors, so I was able to methodically address the proposed changes in neat groups. Per comment, restored to original code, and added Per comment, restored to original code, and added Per comment, restored to original code, and added ~ While analyzing this, I found that the I was able to get Any thoughts? |
|
Thanks a ton for your extensive work on this so far @0xMattB! We already have the same situation across different CPython versions too. Perhaps unintuitive, but I found by trial and error that you can add I know it's ugly, but maybe it could do as a workaround? |
|
@vytas7 No problem!
Pretty much, but the latest errors were not from the |
CaselIT
left a comment
CaselIT
left a comment
There was a problem hiding this comment.
Looks nice, but please avoid code changes in a typing related PR.
The code change may make sense but they should go to a separate PR so that they can be tracked better
|
In retrospect, maybe it was a somewhat vague goal to support Edit: the meaning of
|
seems a lot more effort for not much gain. this pr is not too big |
Yes, I edited my comment. I meant it might be good to break out a couple of hardest ones into separate PRs, if needed. |
|
Hi again @0xMattB, just checking if you're still working on this? |
Add the remaining `--strict` options to `[tool.mypy]` (`check_untyped_defs`, `disallow_any_generics`, `implicit_reexport = false`, `warn_redundant_casts`, `warn_return_any`) so the strict checklist lives in config rather than depending on the `--strict` CLI flag. Parametrize the `App` annotations in the testing module with `[Any, Any]` (required by `disallow_any_generics`), since these clients accept any ASGI/WSGI app — including Falcon apps with custom Request/Response subclasses. Leave a NOTE pointing at making the test clients generic as future work (facilitated by TypeVar defaults on Python 3.13+). Also pass `--no-check-untyped-defs` to `mypy tests` in tox, since the suite is deliberately left untyped (`--allow-untyped-defs`) and checking the bodies of those fixtures surfaces noise that the project's typing strategy doesn't intend to address here.
Revert changes that were unrelated to making `mypy --strict` pass, per @CaselIT's review comments: * `falcon/hooks.py`: restore the `decorate_on_request` wrapping block (and the `cast(_R, ...)` returns) in both `_before` and `_after` that had been deleted. Setting `falcon.hooks.decorate_on_request` had become a no-op. * `falcon/testing/helpers.py`: drop the unnecessary `'{}'.format(text)` / `bytes(data)` wraps in `receive_text` / `receive_data`; silence the underlying `no-any-return` (originating from `AsgiEvent = Mapping[str, Any]`) with a `# type: ignore` matching the style used elsewhere in this branch. * `falcon/util/__init__.py`: remove `'sys'` from `__all__`. Use the `import sys as sys` explicit re-export idiom so that `from falcon.util import sys` (in `falcon/__init__.py`) still works under `implicit_reexport = false` without exposing a stdlib module name through `__all__`. * `falcon/app.py`: drop the underscored TypeVars `_ReqT` / `_RespT` from `__all__`; move their import in `falcon/inspect.py` to come from their real home `falcon._typing`. Align the resulting `__all__` with the `falcon.asgi.app` shape (`App`, `Request`, `Response`).
|
I have addressed most of the concerns with "AI" + myself, but I see some |
|
|
||
| __all__ = ( | ||
| 'App', | ||
| 'Request', |
There was a problem hiding this comment.
Why do we need to officially re-export Request and Response from this app module? 🤔
Co-authored-by: Federico Caselli <cfederico87@gmail.com>
|
|
||
| from http import cookies as http_cookies | ||
| import sys | ||
| import sys as sys # re-export for ``from falcon.util import sys`` consumers |
There was a problem hiding this comment.
There should be no such "consumers"... I'll take a look at this.
|
|
||
| __all__ = ( | ||
| 'App', | ||
| 'Request', |
4 participants
Summary of Changes
Updated code to pass
mypy --stricttype checking.Related Issues
Issue #2504
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. Reading our contribution guide at least once 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.
tox.docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtox -e towncrier, and inspectdocs/_build/html/changes/in the browser 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.