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: initial typing of the public API #248

Merged
merged 7 commits into from
Aug 28, 2023

Conversation

multani
Copy link
Contributor

@multani multani commented May 30, 2023

This adds type annotations for the 2 main decorators of the library.

Closes: #190

I'm also carrying over a small stub file for the event-based functions, which would be now available for everybody.

This adds type annotations for the 2 main decorators of the library.

Closes: GoogleCloudPlatform#190
Comment on lines 61 to 68
```python
import flask
import functions_framework

@functions_framework.http
def hello(request):
def hello(request: flask.Request) -> flask.typing.ResponseReturnValue:
return "Hello world!"
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to best show this to users:

  • On one hand, I think it would be great if developers don't have to search for a long time the type definitions of the functions
  • On the other hand, I really like these short examples in the README file

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Including these type annotations for the sake of illustration in this developer focused README.md seems like a reasonable doc update, some of the essence of the simplicity of this sample is preserved on the GCP docs in https://cloud.google.com/functions/docs/samples/functions-helloworld-get#functions_helloworld_get-python.

setup.py Outdated
Comment on lines 49 to 50
package_data={"functions_framework": ["py.typed"]},
zip_safe=False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Good find, but I'm now seeing that this recommendation has been removed. python/mypy#8802 (comment). We might be safe to remove explicitly setting zip_safe. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I removed the flag 👍

Comment on lines +49 to +50
CloudEventFunction = Callable[[CloudEvent], None]
HTTPFunction = Callable[[flask.Request], flask.typing.ResponseReturnValue]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The signature of the HTTP function is a bit verbose :/

Also, if the type checking fails, the error message is ... interesting :)

$ cat main.py
import functions_framework
import flask


@functions_framework.http
def hello(request: flask.Request) -> flask.typing.ResponseReturnValue:
    return False
$ mypy --strict main.py 
main.py:7: error: Incompatible return value type (got "bool", expected "Union[Union[Response, str, bytes, List[Any], Mapping[str, Any], Iterator[str], Iterator[bytes]], Tuple[Union[Response, str, bytes, List[Any], Mapping[str, Any], Iterator[str], Iterator[bytes]], Union[Headers, Mapping[str, Union[str, List[str], Tuple[str, ...]]], Sequence[Tuple[str, Union[str, List[str], Tuple[str, ...]]]]]], Tuple[Union[Response, str, bytes, List[Any], Mapping[str, Any], Iterator[str], Iterator[bytes]], int], Tuple[Union[Response, str, bytes, List[Any], Mapping[str, Any], Iterator[str], Iterator[bytes]], int, Union[Headers, Mapping[str, Union[str, List[str], Tuple[str, ...]]], Sequence[Tuple[str, Union[str, List[str], Tuple[str, ...]]]]]], Callable[[Dict[str, Any], StartResponse], Iterable[bytes]]]")  [return-value]
Found 1 error in 1 file (checked 1 source file)

I reckon it's more a Flask issue, would you have an idea how to make that better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is return False something that we'd expect to work? Testing with the latest version of ff I think an error is issued when attempting to return a bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember exactly why I returned False in this example, I think the idea was just to return a non-valid type to see what the type checker would show in this case.

So basically, if a developer returns a non-valid type from a @functions_framework.http handler, this will be the error show to the developer.

@multani multani changed the title Initial typing of the public API feat: initial typing of the public API May 30, 2023
@garethgeorge
Copy link
Contributor

Thanks @multani for contributing this pull request! Sorry for the delay in getting a review out for these changes. This is a contribution we'd be happy to merge and your work looks like a great step towards adding typing for functions framework python.

Copy link
Contributor

@garethgeorge garethgeorge left a comment

Choose a reason for hiding this comment

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

Requesting one change with respect to zip_safe=False, otherwise this generally looks good to me and can be merged. Thanks for the contribution!

@garethgeorge
Copy link
Contributor

Thanks! Would you mind adding one more commit with the output of

black src tests setup.py conftest.py --exclude tests/test_functions/background_load_error/main.py

to appease the linter.

You may need to install the black code formatter if you don't already have it.

@multani
Copy link
Contributor Author

multani commented Aug 15, 2023

@garethgeorge I commited the change from black!

@garethgeorge
Copy link
Contributor

Hi @multani , thanks! It looks like there are still some failing lints (import order I believe) would you mind taking a look?

@garethgeorge garethgeorge merged commit 45aed53 into GoogleCloudPlatform:main Aug 28, 2023
42 checks passed
@multani multani deleted the initial-typing branch August 29, 2023 01:52
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.

Untyped functions_framework.http
3 participants