-
Notifications
You must be signed in to change notification settings - Fork 143
Make aiohttp
and requests
optional dependencies
#177
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
base: main
Are you sure you want to change the base?
Conversation
As discussed in the issue and other PR, this is a breaking change for web service users. I'd prefer either a solution that allows you to opt-out of the dependencies rather than opt-in or one that breaks the package into several packages where you could depend on the parts that you need with |
Yes, in semver terms this would be a major version bump. (In fact, the reason why I looked into this was that we have a downstream application that has pinned GeoIP to a much older version because the new version would pull in aiohttp and other things we don't want.) However, practically, chances are that web service users will already have
Unfortunately that's not possible with Python package extras.
In my view, it would be significantly more packaging and infra work for little profit. However, this practically does that already: the core package is |
6ac3f9c
to
0eebbbd
Compare
Rebased on 5.0.1. |
46cce6f
to
e014f68
Compare
Closes maxmind#104 (supersedes it) Fixes maxmind#101
Rebased. Looking at the changelog for 5.0.0, this could easily be just another "BREAKING" change for 6.0.0? |
Thanks for the PR! Looking at it, I think the approach still has the concerns @oschwald mentioned, in that I think this could break things for existing users as they would need to install new packages in situations where they are not already installing I understand the current situation isn't ideal. And there may not be a great alternative with the current packaging system. However I think the status quo is preferable until we have a solution that is less disruptive. |
In my opinion, asking users to change a For me and the company I work for, this is a supply chain vettability and possible security issue.
While installing the library from this PR's HEAD (and in fact, this is what we currently do in production):
|
I understand the concern, but we'd prefer to be conservative about potential breakage. The gain here doesn't seem big enough to warrant that. |
This PR makes
aiohttp
andrequests
, only required forgeoip2.webservice
, optional dependencies.They can be installed separately (many projects will likely have
requests
anyway), or using the extras provided, i.e.to use the version range specified by requirements.
The Tox configuration is adjusted so tests are run both with and without the libraries installed.
This fixes #101 (makes it possible to install without the webservice bits).
This closes #104 (supersedes it to work with the current packaging and testing infrastructure).