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

Use Iterable hint instead of Sequence #4125

Merged

Conversation

stj
Copy link
Contributor

@stj stj commented Oct 1, 2019

Given that the FrozenList type hint is Union[List[_T], Iterable[_T]]
the Application middleware type should not restrict to Sequence and
support all Iterable types.

What do these changes do?

In my application I pass a generator type as the middleware argument to create a web.Application. mypy complains that it is not a Sequence type as hinted in the class. I feel the Sequence hint is
to restrictive and an Iterable is the better choice.

Are there changes in behavior for the user?

No.

Related issue number

None

Checklist

  • I think the code is well written
  • [-] Unit tests for the changes exist
  • [-] Documentation reflects the changes
  • [-] If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • [-] Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Given that the `FrozenList` type hint is `Union[List[_T], Iterable[_T]]`
the Application middleware type should not restrict to `Sequence` and
support all `Iterable` types.
@stj stj requested a review from asvetlov as a code owner October 1, 2019 22:55
@codecov-io
Copy link

codecov-io commented Oct 1, 2019

Codecov Report

Merging #4125 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4125   +/-   ##
=======================================
  Coverage   97.33%   97.33%           
=======================================
  Files          43       43           
  Lines        8849     8849           
  Branches     1381     1381           
=======================================
  Hits         8613     8613           
  Misses        114      114           
  Partials      122      122
Impacted Files Coverage Δ
aiohttp/web_app.py 96.01% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1a1a53...cd50afa. Read the comment docs.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Makes sense.
Thank you

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Please add ./CHANGES/4125.feature file.

@stj stj force-pushed the change_application_middleware_typehint branch from 72e3142 to cd50afa Compare October 2, 2019 15:47
@stj stj requested a review from webknjaz as a code owner October 2, 2019 15:47
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 2, 2019
@stj
Copy link
Contributor Author

stj commented Oct 2, 2019

Done.

@asvetlov asvetlov merged commit 8475395 into aio-libs:master Oct 2, 2019
@asvetlov
Copy link
Member

asvetlov commented Oct 2, 2019

Thanks!

asvetlov pushed a commit that referenced this pull request Oct 2, 2019
Given that the `FrozenList` type hint is `Union[List[_T], Iterable[_T]]`
the Application middleware type should not restrict to `Sequence` and
support all `Iterable` types..
(cherry picked from commit 8475395)

Co-authored-by: Stefan T <66305+stj@users.noreply.github.com>
asvetlov added a commit that referenced this pull request Oct 3, 2019
Given that the `FrozenList` type hint is `Union[List[_T], Iterable[_T]]`
the Application middleware type should not restrict to `Sequence` and
support all `Iterable` types..
(cherry picked from commit 8475395)

Co-authored-by: Stefan T <66305+stj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants