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

Add stubs for WebOb #9874

Merged
merged 21 commits into from
Jun 18, 2023
Merged

Add stubs for WebOb #9874

merged 21 commits into from
Jun 18, 2023

Conversation

Daverball
Copy link
Contributor

I've already created some more simple private stubs for WebOb, so I thought I'd try to clean them up and contribute them to the community. WebOb makes very heavy use of descriptors, which I've tried to fully leverage, but I will have to test if all the overloads actually work correctly. It might be worth going back to something more simplistic again which replaces some of the descriptors which just plain attributes, even if WebOb internally would be more generous what it accepts as an input for the assignment.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

pyrightconfig.stricter.json Outdated Show resolved Hide resolved
stubs/WebOb/METADATA.toml Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

stubs/WebOb/webob/multidict.pyi Outdated Show resolved Hide resolved
stubs/WebOb/webob/request.pyi Outdated Show resolved Hide resolved
stubs/WebOb/webob/request.pyi Outdated Show resolved Hide resolved
stubs/WebOb/webob/static.pyi Show resolved Hide resolved
stubs/WebOb/webob/cookies.pyi Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 14, 2023

Is this PR ready for proper review? If so, please take it out of draft :)

The annotations certainly don't need to be complete for this to be merged; we encourage incremental submissions of annotations! It makes it much easier for us to review.

@Daverball
Copy link
Contributor Author

I'd like to play around with the annotations in a couple of different code bases first to see if there are any particularly rough edges or stuff that doesn't resolve as intended. I won't really have time to do that until later this week though.

@AlexWaygood
Copy link
Member

Take your time, there's no rush! Was just clarifying the status :)

Since mypy doesn't support asymmetric properties yet, I've had to switch
to a custom descriptor implementation for those properties.
@github-actions

This comment has been minimized.

@Daverball
Copy link
Contributor Author

I did some testing in various codebases and in a sandbox with reveal_type mostly with the Request/Response classes, since that is really the part that matters most. There were some issues with asymmetric properties that I had to solve by adding custom descriptors.

All in all I'm satisfied from a library users' perspective now, so this should be ready for a more general review.

@Daverball Daverball marked this pull request as ready for review March 18, 2023 16:22
The header property setters previously all accepted `bytes` as an input
for the header, but upstream enforces the use of `str`, so it's better
to be strict, so people's code won't pass type checking if it will break
in the next version of WebOb.
@github-actions

This comment has been minimized.

@Daverball Daverball requested a review from AlexWaygood March 21, 2023 16:28
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Also fixes small mistake in `Cookie.load` and `Cookie.__init__`
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

stubs/WebOb/@tests/stubtest_allowlist.txt Outdated Show resolved Hide resolved
stubs/WebOb/@tests/stubtest_allowlist.txt Outdated Show resolved Hide resolved
Comment on lines +116 to +123
@overload
def __set__(self, __obj: Any, __value: str | None) -> None: ...
@overload
def __set__(self, __obj: Any, __value: AcceptNoHeader | AcceptValidHeader | AcceptInvalidHeader) -> None: ...
@overload
def __set__(self, __obj: Any, __value: SupportsItems[str, float | tuple[float, str]]) -> None: ...
@overload
def __set__(self, __obj: Any, __value: _ListOrTuple[str | tuple[str, float, str] | list[Any]]) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like this needs overloads, you can just use a union as the argument type for __value.

Copy link
Contributor Author

@Daverball Daverball Jun 8, 2023

Choose a reason for hiding this comment

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

You are completely correct, however in this case I think a bunch of overloads reads more nicely than a very large type union. I've seen overloads used like this in other places in typeshed and liked how it looks compared to a very large type union where after maybe the fourth or fifth type in the union you just stop reading, especially once the types get as complicated as the last two overloads.

So here it's separated neatly into the plain string version of the header (as per the HTTP spec), then the specialized objects for representing the header from WebOb, then the Mapping like way to represent this data, and then the sequence like way to represent it.

You could make a case for combining the first two overloads, but I think the other two should stay distinct. I've also debated whether or not to add the final overload at all, because it isn't completely type safe and kind of confusing to read, it just happens to be one of the supported and documented ways to pass this data in.

Fixes a couple of typos in the `stubtest_allowlist.txt` comments

Co-authored-by: Jelle Zijlstra <[email protected]>
@github-actions

This comment has been minimized.

Implements bare minimum on `webob.dec.wsgify` to make the decorator work
@github-actions

This comment has been minimized.

@Daverball Daverball requested a review from JelleZijlstra June 14, 2023 06:08
Fixes annotation of `dicts` and `copy` on `NestedMultiDict`
@Daverball Daverball requested a review from JelleZijlstra June 15, 2023 07:05
@github-actions

This comment has been minimized.

@Daverball Daverball requested a review from JelleZijlstra June 16, 2023 06:07
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

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