Skip to content

Add 'speedups' extra dependencies group #3434

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 7, 2018
Merged

Add 'speedups' extra dependencies group #3434

merged 1 commit into from
Dec 7, 2018

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Dec 6, 2018

Resolves #2397

What do these changes do?

Add extra deps so that it'd possible to install them along with the main project as pip install aiohttp[speedups]

Are there changes in behavior for the user?

Enable users to use pip install aiohttp[speedups]

Related issue number

#2397

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."

@webknjaz webknjaz requested review from asvetlov and kxepal December 6, 2018 13:53
@webknjaz webknjaz mentioned this pull request Dec 6, 2018
Closed
@kxepal
Copy link
Member

kxepal commented Dec 6, 2018

s/speedups/faster/ - that's nicer and could be sold with easy. Also, why there is no uvloop?

@webknjaz
Copy link
Member Author

webknjaz commented Dec 6, 2018

I don't want faster, because it's a name of group of deps, not an action. It's the same as when you call a function with action/verb but you call a var/class as an object.

@webknjaz
Copy link
Member Author

webknjaz commented Dec 7, 2018

@asvetlov so what about uvloop?

@asvetlov
Copy link
Member

asvetlov commented Dec 7, 2018

Just library installation doesn't make sense, uvloop policy should be initialized by asyncio.set_event_loop_policy().

I don't see a place for the call in aiohttp itself.

@webknjaz
Copy link
Member Author

webknjaz commented Dec 7, 2018

So I think the idea is to check whether it's present and do it on init.

@asvetlov
Copy link
Member

asvetlov commented Dec 7, 2018

What do you mean by on init?
I don't see any 100% reliable scenario.
A user always can do weird things, e.g. calling asyncio API before aiohttp import.
If aiohttp will change the default loop policy on import (which is a side effect BTW) previously opened connection will just hang etc.

@webknjaz
Copy link
Member Author

webknjaz commented Dec 7, 2018

True. So no uvloop in extras then.

@webknjaz webknjaz merged commit e3c1224 into master Dec 7, 2018
@asvetlov asvetlov deleted the packaging/extra-deps branch December 7, 2018 16:40
@lock
Copy link

lock bot commented Dec 7, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Dec 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@asvetlov asvetlov asvetlov approved these changes

@kxepal kxepal Awaiting requested review from kxepal

Assignees

@asvetlov asvetlov

@webknjaz webknjaz

Labels
enhancement outdated question StackOverflow
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Design good distribution extras support
4 participants
@webknjaz @kxepal @asvetlov @aio-libs-bot