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

IAgents should return IResponses #443

Merged
merged 6 commits into from
Oct 28, 2021
Merged

IAgents should return IResponses #443

merged 6 commits into from
Oct 28, 2021

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Oct 27, 2021

In #439 I tried to use Response rather than IResponse in some annotations,
to avoid having to
write a type stub to cover twisted's iweb interface.

There were other complications down the line from this, because I was now contravening the zope interfaces I was claiming to adhere to. It was easier to change my mind, use stubgen, and polish up the result by hand. A lot of the work had been done already in annotating the implementation classes.

David Robertson added 2 commits October 27, 2021 20:51
I tried to use `Response` rather than `IResponse` to avoid having to
write a stub for the latter. There were other complications down the
line from this. It was easier to change my mind, use `stubgen` and
polish up the `iweb` stub.

See also #439 (comment)
@DMRobertson DMRobertson mentioned this pull request Oct 27, 2021
@DMRobertson DMRobertson marked this pull request as ready for review October 27, 2021 20:06
@DMRobertson DMRobertson requested a review from a team as a code owner October 27, 2021 20:06
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

looks pretty good, just a couple tiny things

@@ -61,7 +60,7 @@ class FileBodyProducer:
def resumeProducing(self) -> None: ...

def readBody(response: IResponse) -> Deferred[bytes]: ...

@implementer(IResponse) # type: ignore[misc]
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this need to be ignored? Is it just because the stub doesn't reproduce all the method signatures verbatim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly it. Laziness on my part, really.

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'll add a comment to explain this.

def setHeader(k: AnyStr, v: AnyStr) -> None: ...
def redirect(url: AnyStr) -> None: ...
# returns CACHED or False. CACHED is a string constant but we treat it as
# a binary object.
Copy link
Contributor

Choose a reason for hiding this comment

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

'binary' object? hrm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be "opaque", thanks. Don't stub when you're tired, kids.

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

thanks!

@DMRobertson DMRobertson merged commit 48d2e7f into main Oct 28, 2021
@DMRobertson DMRobertson deleted the dmr/mypy/stub-iweb branch October 28, 2021 10:08
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.

2 participants