Skip to content
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: introduce type hints (MVP) #1947

Merged
merged 24 commits into from
Jul 8, 2023
Merged

Conversation

vytas7
Copy link
Member

@vytas7 vytas7 commented Aug 6, 2021

Closes #1350

This is only to get the ball rolling though, with an emphasis on the most commonly used, publicly documented functionality.
We'll have to expand typing later in follow-up PRs. One of such PRs is already in Draft: #1944.

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #1947 (f32943c) into master (1e34bf4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1947   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           62        62           
  Lines         6806      6829   +23     
  Branches      1098      1098           
=========================================
+ Hits          6806      6829   +23     
Impacted Files Coverage Δ
falcon/asgi/multipart.py 100.00% <ø> (ø)
falcon/media/multipart.py 100.00% <ø> (ø)
falcon/app.py 100.00% <100.00%> (ø)
falcon/app_helpers.py 100.00% <100.00%> (ø)
falcon/asgi/app.py 100.00% <100.00%> (ø)
falcon/errors.py 100.00% <100.00%> (ø)
falcon/media/base.py 100.00% <100.00%> (ø)
falcon/request.py 100.00% <100.00%> (ø)
falcon/response.py 100.00% <100.00%> (ø)
falcon/routing/converters.py 100.00% <100.00%> (ø)
... and 4 more

@vytas7 vytas7 changed the title chore: prepare for shipping type hints [DRAFT] feat: add type hints [DRAFT] Aug 21, 2022
@vytas7 vytas7 changed the title feat: add type hints [DRAFT] feat: introduce type hints [DRAFT] Nov 6, 2022
@vytas7 vytas7 changed the title feat: introduce type hints [DRAFT] feat: introduce type hints (MVP) Mar 11, 2023
@vytas7 vytas7 marked this pull request as ready for review March 11, 2023 10:01
@vytas7 vytas7 requested review from kgriffs and CaselIT and removed request for kgriffs March 11, 2023 10:02
@vytas7 vytas7 requested a review from kgriffs March 11, 2023 10:02
CaselIT
CaselIT previously approved these changes Mar 13, 2023
Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks mostly ok to me. I've left a suggestion and replied to a TODO

falcon/app.py Show resolved Hide resolved
falcon/app.py Show resolved Hide resolved
falcon/typing.py Show resolved Hide resolved
falcon/util/misc.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
tox.ini Show resolved Hide resolved
Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! This patch is a solid improvement.

falcon/app.py Outdated Show resolved Hide resolved
falcon/app.py Outdated Show resolved Hide resolved
falcon/app_helpers.py Outdated Show resolved Hide resolved
falcon/app_helpers.py Outdated Show resolved Hide resolved
falcon/media/multipart.py Show resolved Hide resolved
falcon/media/base.py Show resolved Hide resolved
Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just https://github.com/falconry/falcon/pull/1947/files#r1145637935 otherwise LGTM regarding my previous comments.

CaselIT
CaselIT previously approved these changes Jul 1, 2023
Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the generic were not added like in list, dict, tuple, etc.

I guess we can to a pass on them later

falcon/app_helpers.py Show resolved Hide resolved
falcon/app_helpers.py Show resolved Hide resolved
@vytas7
Copy link
Member Author

vytas7 commented Jul 1, 2023

I haven't had the time to finish this yet @CaselIT @kgriffs... As discussed on Gitter, I'll remove the generics and try to type as much as possible using plain Request and Response.

@CaselIT
Copy link
Member

CaselIT commented Jul 1, 2023

I haven't had the time to finish this yet @CaselIT @kgriffs... As discussed on Gitter, I'll remove the generics and try to type as much as possible using plain Request and Response.

I was referring to generic only on builtin containers such as list, dict, iterable, tuple, etc

@vytas7
Copy link
Member Author

vytas7 commented Jul 1, 2023

@CaselIT See my other comment, I'm not convinced there is any difference in elaborating that way with [Any].

See, for instance:
test.py

from typing import Iterable


def print_all(iterable: Iterable, header: str = '== Items ==') -> None:
    print(header)
    for index, item in enumerate(iterable):
        print(f'{index+1}: {item!r}')


def main() -> None:
    print_all('ABC', header='=====')
    print_all(None)
    print_all(header=3)


if __name__ == '__main__':
    main()
$ mypy test.py 
test.py:12: error: Argument 1 to "print_all" has incompatible type "None"; expected "Iterable[Any]"  [arg-type]
test.py:13: error: Missing positional argument "iterable" in call to "print_all"  [call-arg]
test.py:13: error: Argument "header" to "print_all" has incompatible type "int"; expected "str"  [arg-type]

Judging from the error messages, Iterable and Iterable[Any] are simply synonymous.

@vytas7 vytas7 requested review from kgriffs and CaselIT July 2, 2023 20:35
@vytas7 vytas7 merged commit 574a0d0 into falconry:master Jul 8, 2023
35 checks passed
@kgriffs
Copy link
Member

kgriffs commented Jul 20, 2023

🥳

@dkbarn
Copy link

dkbarn commented Apr 16, 2024

How certain are we that the changes made here actually have the intended effect? I'm noticing that the py.typed file was never added as package data, it was only added in the MANIFEST.in. When I install the latest version of falcon (3.1.3), there is no py.typed file present in the install dir. This means mypy continues to complain when importing falcon:

error: Skipping analyzing "falcon": module is installed, but missing library stubs or py.typed marker  [import-untyped]

@vytas7
Copy link
Member Author

vytas7 commented Apr 16, 2024

Hi @dkbarn! We're still working on improved typing, this was just a start. But the main reason you are not even seeing any py.typed is that the changes haven't been incorporated into any stable release yet. This and other typing improvements will be shipped as part of 4.0.0.

@dkbarn
Copy link

dkbarn commented Apr 16, 2024

Sounds good, thanks for the explanation @vytas7 !

@vytas7
Copy link
Member Author

vytas7 commented Apr 16, 2024

Thanks for understanding, I know our velocity has been less than stellar...

For the time being, you can also give the latest development snapshot a shot, and see if it works better with Mypy:

pip install git+https://github.com/falconry/falcon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add type information - mypy support
4 participants