Allow installing aiodns and cchardet as extras #3412
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3412 +/- ##
=======================================
Coverage 97.94% 97.94%
=======================================
Files 44 44
Lines 8511 8511
Branches 1382 1382
=======================================
Hits 8336 8336
Misses 74 74
Partials 101 101Continue to review full report at Codecov.
|
|
Thanks! |
| @@ -109,6 +109,11 @@ def read(f): | |||
| 'pytest-xdist', | |||
| ] | |||
|
|
|||
| extras_require = { | |||
| 'cchardet': ['cchardet'], | |||
| 'aiodns': ['aiodns'], | |||
There was a problem hiding this comment.
I'd go for smth like dns-speedup and charset-speedup to make extras represent feature flags, not end-dependencies. This also allows to abstract extra names and make those deps easily replaceable.
There was a problem hiding this comment.
Sorry, looks like I've pressed 'merge' too fast.
The chance of libraries replacement is very low.
I doubt if we'll do it at all.
But I like speedup requirement name.
What if we add 'speedup: ['aiodns', 'cchardet']` line to requirements for quick installation of all available accellerators?
P.S.
I've missed 'brotli', it is another optional dependency.
There was a problem hiding this comment.
@asvetlov are there viable cases when one would want to install aiodns but not cchardet or vice versa? If no, speedup(s?) sounds cool.
There was a problem hiding this comment.
For example, user wants to use aiodns but cchardet installation faild on his system.
Or aiodns doesn't work well for some reason, e.g. it doesn't respect all OS configuration files.
There was a problem hiding this comment.
Well, there's speedups in plural I saw used in other distributions. We could use that for "everything".
As for "it's not likely to change", while it's likely true I'd like to point out that there will probably be other dependencies and I'd like to see extra names being consistent as it's a public API, so changing names would be difficult.
I don't want to end up with a bunch of extras named using different random strategies. And I think extras should name features, not implementations.
Bottom line: we might want to have separate extra deps + one "wholesale" extra.
There was a problem hiding this comment.
I like it.
The only thing we need to think about is naming.
cchardet is definitely a speedup.
aiodns is not. I can boost the execution time but the main point is using nonblocking async DNS API.
brotli is a feature, basically a support for another comression method (implemented partially now).
What are the best names for these features? Suggestions?
There was a problem hiding this comment.
Taking into account the conversation I've reverted the PR.
Let's make an agreement about exposed names first and only after it implement them.
The issue priority is low but I love to add changes only once without future renaming.
There was a problem hiding this comment.
charset-guessing-speedup, async-dns and brotli-compression?
Users will be able to install
aiodnsandcchardetusingpip install aiohttp[aiodns,cchardet]Related to #2397