Skip to content

Add type signature for a WSGI Application to wsgiref. #725

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

Closed
wants to merge 3 commits into from

Conversation

rowillia
Copy link
Contributor

This type is something core to Python and is useful when typing web applications,
but doesn't actually exist in the stdlib anywhere. I put this in wsgiref, but I am
open to suggestions as for a better place.

This type is something core to Python and is useful when typing web applications,
but doesn't actually exist in the stdlib anywhere.  I put this in wsgiref, but I am
open to suggestions as for a better place.
@rowillia rowillia force-pushed the add_wsgi_application_stub branch from ce54f29 to 81431d2 Compare November 30, 2016 18:53
Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

I'm undecided here. At the same time, I see the need for specifying the application's (callable's) signature, and at the same time a more kosher place for it would be wsgiref/simple_server.pyi.

@@ -0,0 +1,26 @@
# Type declaration for a WSGI Function in Python 2
#
# This function actually exist, but we need a central place to define the type
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean something along the lines of "WSGIFunction doesn't exist in wsgiref/init.py, it's a type provided for type checking purposes."

exc_info = Tuple[Optional[Type[BaseException]],
Optional[BaseException],
Optional[TracebackType]]
WSGIFunction = Callable[[Dict[Union[unicode, str], Union[unicode, str]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't typing.AnyStr work better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ambv no, AnyStr is a TypeVar and I don't want this function to be generic with respect to a single string type - https://github.com/python/typing/blob/master/python2/typing.py#L560 . It would be perfectly valid to have something like

environ = {
  u'hello': 'world'
}
result = app(environ, ....

@@ -0,0 +1,26 @@
# Type declaration for a WSGI Function in Python 3
#
# This function actually exist, but we need a central place to define the type
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto about the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK

exc_info = Tuple[Optional[Type[BaseException]],
Optional[BaseException],
Optional[TracebackType]]
WSGIFunction = Callable[[Dict[Union[unicode, str], Union[unicode, str]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this should rather be called WSGICallable as it can very well be a class, as documented in the comment in this example:
https://docs.python.org/3/library/wsgiref.html#examples

Or maybe WSGIApplication which it's called in most frameworks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM, will fix.

@rowillia
Copy link
Contributor Author

@ambv Fixed up the names. I'd somewhat prefer to leave it under wsgiref as opposed to simple_server as it's core to WSGI, not just the simple_server and the ergonomics of:

if TYPE_CHECKING:
    from wsgiref.simple_server import WSGIApplication

Feels less authoritative than:

if TYPE_CHECKING:
    from wsgiref import WSGIApplication

@ambv
Copy link
Contributor

ambv commented Nov 30, 2016

Ssssh, I'm just trying to talk you into providing stubs for more files ;-)

@ambv
Copy link
Contributor

ambv commented Nov 30, 2016

@gvanrossum This looks reasonable to me, do you have an opinion?

@gvanrossum
Copy link
Member

I'm not against having this alias in the wsgiref stubs per se, but:

  • exc_info probably should be renamed to _exc_info to clarify it is not part of the interface (it's a tuple type that's used anonymously in many places).
  • The __init__.py is currently empty; I think it should stay empty, and the type alias(es) should be in their own submodule. That's how the wsgiref package is structured.

@rowillia
Copy link
Contributor Author

rowillia commented Dec 1, 2016

@gvanrossum moved things around. LMK what you think!

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Just various editorial nits. Also, do you have real-world code that uses this yet? It would be nice if you tested with that, to make sure that it's actually reasonable to write a function that conforms to the signature.

# from typing import TYPE_CHECKING
#
# if TYPE_CHECKING:
# from wsgiref import WSGIFunction
Copy link
Member

Choose a reason for hiding this comment

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

Change to from wsgiref.types import ...

# wsgiref/typing.py doesn't exist and neither does WSGIApplication, it's a type
# provided for type checking purposes.
#
# To correctly use this type stub, utilize the `TYPE_CHECKING` flag in
Copy link
Member

Choose a reason for hiding this comment

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

utilize -> use

@@ -0,0 +1,26 @@
# Type declaration for a WSGI Function in Python 2
#
# wsgiref/typing.py doesn't exist and neither does WSGIApplication, it's a type
Copy link
Member

Choose a reason for hiding this comment

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

typing -> types

#
# if TYPE_CHECKING:
# from wsgiref import WSGIFunction
#
Copy link
Member

Choose a reason for hiding this comment

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

In the PY3 version (but not in the PY2 version) you should also tell users to use "forward reference" notation when using this (except in # type: comments). An example would be good.

WSGIApplication = Callable[[Dict[Union[unicode, str], Union[unicode, str]],
Union[
Callable[[Union[unicode, str], List[Tuple[Union[unicode, str], Union[unicode, str]]]], Callable[[Union[unicode, str]], None]],
Callable[[Union[unicode, str], List[Tuple[Union[unicode, str], Union[unicode, str]]], _exc_info], Callable[[Union[unicode, str]], None]]
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty unreadable. Maybe you can clear it up a bit using some more type aliases? (An added benefit might be that there are fewer differences between PY2 and PY3.)

@gvanrossum
Copy link
Member

@rowillia Do you have time to update this? (I couldn't get GitHub to let me edit the files else I would have done it myself. :-)

@rowillia
Copy link
Contributor Author

rowillia commented Dec 20, 2016

@gvanrossum Looking into it now, thanks for the feedback.

@ambv ambv force-pushed the master branch 10 times, most recently from 63ba0ab to 7853c26 Compare December 23, 2016 00:12
@gvanrossum
Copy link
Member

@rowillia Ping? Do you have any time to update? Or should I just close this?

@gvanrossum
Copy link
Member

Superseded by #825.

@gvanrossum gvanrossum closed this Jan 13, 2017
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.

3 participants