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

Add type hints to HttpResponse #94

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Add type hints to HttpResponse #94

merged 4 commits into from
Mar 9, 2022

Conversation

michelole
Copy link
Contributor

@michelole michelole commented Aug 19, 2021

It seems HttpResponse is the only class in this file that does not contain type hints.

This generates errors when client code is checked using mypy in strict mode:

error: Call to untyped function "HttpResponse" in typed context

Add type hints following the docstring.

@vrdmr
Copy link
Member

vrdmr commented Sep 24, 2021

@tonybaloney - I think this change will need a corresponding change in _http_asgi.py to fix the type annotations for the headers in to_func_response.

=================================== FAILURES ===================================
__________________________ TestCodeQuality.test_mypy ___________________________
Traceback (most recent call last):
  File "/home/runner/work/azure-functions-python-library/azure-functions-python-library/tests/test_code_quality.py", line 32, in test_mypy
    f'mypy validation failed:\n{output}') from None
AssertionError: mypy validation failed:
azure/functions/_http_asgi.py:73: error: Argument "headers" to "HttpResponse" has incompatible type "Union[Headers, Dict[Any, Any]]"; expected "Optional[Mapping[str, str]]"
Found 1 error in 1 file (checked 45 source files)

@vrdmr vrdmr added the Blocked label Sep 24, 2021
@ghost
Copy link

ghost commented Oct 7, 2021

CLA assistant check
All CLA requirements met.

@michelole
Copy link
Contributor Author

@vrdmr @tonybaloney I have ignored the error for the moment following similar code in _http_wsgi.py.

A proper solution would be to make a consistent usage of the Headers class in HttpResponse, thus deprecating HttpResponseHeaders and BaseHeaders, which are only used there and do not seem to support duplicate keys. I believe that should be tackled in a different issue/PR though.

@vrdmr vrdmr requested a review from gavin-aguiar as a code owner December 7, 2021 00:01
@michelole
Copy link
Contributor Author

@vrdmr @gavin-aguiar Any updates here? Since the mypy error was fixed, could we remove the "Blocked" label?

@vrdmr vrdmr removed the Blocked label Jan 31, 2022
@michelole
Copy link
Contributor Author

@AnatoliB @anirudhgarg @gavin-aguiar Could you have a look on this PR as required reviewers?

Ps.: @vrdmr It seems tests are failing/cancelling across all builds and are not related to this PR only.

Copy link
Member

@vrdmr vrdmr left a comment

Choose a reason for hiding this comment

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

LGTM.

@vrdmr vrdmr requested a review from YunchuWang as a code owner March 9, 2022 07:01
@vrdmr vrdmr merged commit c6ae418 into Azure:dev Mar 9, 2022
@vrdmr
Copy link
Member

vrdmr commented Mar 9, 2022

Thanks a lot, @michelole for the contribution, and apologies for the delay.

I'll fix the CI issues with our workflow caused due to a PR coming from a fork. Thanks again.

@michelole michelole deleted the patch-1 branch March 9, 2022 09:47
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.

2 participants