-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add stubs for WebTest #14541
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
base: main
Are you sure you want to change the base?
Add stubs for WebTest #14541
Conversation
This comment has been minimized.
This comment has been minimized.
Thanks for contributing! I couldn't find any prior discussion about adding type hints directly to the package, but it does seem like preferred option, so I think it's worth opening an issue to suggest this idea to WebTest maintainers. |
Considering my PR to add my WebOb stubs as inline type hints to WebOb has been open for half a year without so much as a peep from a maintainer and them both being Pylons projects I decided not to bother spending the extra amount of time required to add inline hints, and I needed the stubs for something I was working on anyways, so it was quicker to just get it done. So since I already had them I decided to just contribute them. That being said, WebTest does have a different maintainer, which appears to be ever so slightly more active, so I'm willing to try: Pylons/webtest#271 |
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.
Considering that there has been no response upstream, it makes sense to me to add this typeshed.
from webob.request import BaseRequest | ||
from webtest.response import TestResponse | ||
|
||
_Files: TypeAlias = Sequence[tuple[str, str] | tuple[str, str, bytes]] |
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.
Considering that Sequence
is not a protocol, using a real protocol (from collections.abc
or _typeshed
) would be better.
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.
I considered using Collection
, the minimum requirement appears to be Iterable & Sized
at first glance, but since the docs and error message say list
I wanted to be a little more conservative, in case some implementation details change, without making it annoying to pass in lists that contain a type that's compatible with tuple[str, str] | tuple[str, str, bytes]
.
relative_to: str | None = None, | ||
use_unicode: bool = True, | ||
cookiejar: CookieJar | None = None, | ||
parser_features: Sequence[str] | str | None = None, |
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.
See above. More instances below.
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 matches what beautifulsoup4 expects for the features
parameter, so I think it should stay like this.
stubs/WebTest/webtest/app.pyi
Outdated
params: Mapping[str, str] | str | None = None, | ||
headers: Mapping[str, str] | None = None, | ||
extra_environ: Mapping[str, Any] | None = None, |
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.
Same is true for Mapping
(with more instances below).
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.
Actually I just noticed Mapping
is too broad for extra_environ
, since the implementation does del extra_environ[key]
. I probably should just change this to _typeshed.wsgi.WSGIEnvironment
, like I initially had planned to, the documentation explicitly says dict
as well.
For params
I probably should use _QueryType
from urllib.parse
.
headers
is another one that is too narrow currently, if xhr=True
it needs to be able to be able support update
called with a dict
, so at best we could reduce that SupportsItems & SupportsUpdate
, but I'm not sure whether it's worth the extra effort, it might be easier to just switch to the documented dict
or MutableMapping
.
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.
I take it back params
needs to be different because of utils.encode_params
which does isinstance(params, (list, tuple))
and also does list(params.items()
if params
has items
. So it needs to be some complex combination of SupportsItems
list
and tuple
.
encode_multipart
which is used if there are any file uploads, has slightly different requirements once again which poses some hard restrictions on what the value type can be, which isn't great for list
, so I might opt for using Sequence
after all.
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.
And it looks like extra_environ
only needs to be dict
in the constructor after all, everywhere else everything that works with dict.update(extra_environ)
is fine, as long as it is also Sized
.
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
+ discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
- discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
+ discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
- discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
|
A small testing framework for WSGI applications based around WebOb.
Source: https://github.com/Pylons/webtest/blob/3.0.6/webtest/__init__.py
Docs: https://docs.pylonsproject.org/projects/webtest/en/latest/
webtest.forms.Form
turned out a little more lax than I would've liked. But making it more strict would've probably caused quite a bit of churn in test code, like not being able to useCheckbox.checked
without first doing anisinstance
check, or either having to do the same forSelect.select
/MultipleSelect.select_multiple
or having to switch toForm.select
/Form.select_multiple
.I also debated for a while on whether or not to make
TestApp
generic, but ultimately it seemed worth it, since if you have a test fixture that returns aTestApp
, you don't necessarily want to have to specify another fixture in order to get at theWSGIApplication
that's being tested with the correct type information. Giving theTypeVar
a default ofWSGIApplication
should end up making it behave more or less the same as the non-generic version, if you don't care about being able to access the application throughTestApp.app
.