search

Azure / azure-functions-python-library

Azure Functions Python SDK
MIT License
151 stars 63 forks source link

Add type hints to `HttpResponse` #94

Closed michelole closed 2 years ago

michelole commented 3 years ago

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 commented 2 years ago

@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)
ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

michelole commented 2 years ago

@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.

michelole commented 2 years ago

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

michelole commented 2 years ago

@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.

vrdmr commented 2 years ago

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.