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

10270: add annotation to twisted.web.http.Request.getHeader #1669

Merged
merged 5 commits into from
Feb 6, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Oct 21, 2021

Scope and purpose

Another drive-by contribution. Similar to #1668.

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10270
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #1669 from DMRobertson:annotate-getHeader

Author: DMRobertson
Reviewer: <github_username>, <github_username_if_more_reviewers>
Fixes: ticket:10270

Long description providing a summary of these changes.
(as long as you wish)

@DMRobertson DMRobertson changed the title Add annotation to twisted.web.http.Request.getHeader 10270: add annotation to twisted.web.http.Request.getHeader Oct 21, 2021
@@ -1051,7 +1051,7 @@ def unregisterProducer(self):

# The following is the public interface that people should be
# writing to.
def getHeader(self, key):
def getHeader(self, key: AnyStr) -> Optional[AnyStr]:
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 this might need overloads to successfully capture that the type of the output matches the type of the input. (A str key causes a Optional[str] return value.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AnyStr is a TypeVar, so I think this should be fine as is. See https://docs.python.org/3/library/typing.html?highlight=anystr#typing.AnyStr (and note the same is done in the definition of getRawHeaders().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I hadn't realized AnyStr didn't allow mixing. Seems perfect then! 👍

@DMRobertson DMRobertson marked this pull request as ready for review October 21, 2021 15:12
DMRobertson pushed a commit to matrix-org/sydent that referenced this pull request Oct 21, 2021
Adding the stub for `getHeader` means that mypy isn't looking at
twisted's implementation to deduce what types instance attributes have.
So let's add those to the stub.

I don't want to maintain a stub out-of-tree forever, but I've submitted
the `getHeader` annotation upstream in
twisted/twisted#1669 .
@twm twm merged commit ebb2d36 into twisted:trunk Feb 6, 2022
@twm
Copy link
Contributor

twm commented Feb 6, 2022

Thank you all!

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.

5 participants