-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add incorrect/missing types to Werkzeug wrappers #969
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
Conversation
third_party/2/werkzeug/wrappers.pyi
Outdated
status = ... # type: str | ||
direct_passthrough = ... # type: bool | ||
response = ... # type: Iterable[str] | ||
def __init__(self, response: Union[Iterable[str], str]=None, status=Union[basestring, int], headers: Union[Headers, Mapping[basestring, basestring], Sequence[Tuple[basestring, basestring]]]=None, mimetype: basestring=None, content_type: basestring=None, direct_passthrough: bool=False): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must have -> None
(see the Travis failure). I guess it didn't fail before because the function didn't have any type annotations.
third_party/2/werkzeug/wrappers.pyi
Outdated
dict_storage_class = ... # type: Any | ||
form_data_parser_class = ... # type: Any | ||
trusted_hosts = ... # type: Any | ||
charset = ... # type: basestring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing things as basestring
can lead to issues because basestring
doesn't have any methods defined on it, so people will always have to do an isinstance
check on it before they can do anything with this value that mypy will approve of. I think the normal practice for this kind of thing is to type the attribute as Any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JelleZijlstra How about AnyStr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That leads to similar issues (at least under mypy); in fact mypy doesn't seem to allow me to type a class attribute this way. See #871 for some discussion on how to handle this sort of case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JelleZijlstra Okay, I will change attributes and return values to be typed as one of str
or unicode
depending on its typical uses. In case of arguments, I will stick with basestring
.
third_party/3/werkzeug/wrappers.pyi
Outdated
status = ... # type: str | ||
direct_passthrough = ... # type: bool | ||
response = ... # type: Iterable[bytes] | ||
def __init__(self, response: Union[Iterable[bytes], bytes]=None, status=Union[str, int], headers: Union[Headers, Mapping[str, str], Sequence[Tuple[str, str]]]=None, mimetype: str=None, content_type: str=None, direct_passthrough: bool=False): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs -> None
3ac979d
to
b21dd8c
Compare
@JelleZijlstra I adjusted the patch as you advised. 😄 |
Thanks! I haven't compared this with werkzeug documentation or code but don't see further issues in the stubs. |
Thanks! I'm making a leap of faith here based on Jelle's recommendation. If something is still wrong here we can iterate in a new PR. |
Fixed incorrectly typing, and added missing typing of
werkzeug.wrappers.BaseRequest
/BaseResponse
attributes. It isn't complete, but covers commonly used attributes/methods.