- Notifications
You must be signed in to change notification settings - Fork145
Removing unused dependency, adding optional dependencies and loosening requests version#104
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ement and making aiohttp and requests optional installs
Although the extra dependencies do make some sense, this would likely be a breaking change for many existing web service users. I don't know if this is easily resolvable with Setuptools. Perhaps at some point, it would make sense to break this up into several packages but it would be nice to that in such a way that doesn't increase maintenance burden. I believe |
sebix left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also possible without checkingsys.modules.
| importaiohttp | ||
| importaiohttp.http | ||
| exceptImportError: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| pass | |
| aiohttp=None |
| importipaddress | ||
| importjson | ||
| importsys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| import sys |
| importrequests | ||
| importrequests.utils | ||
| exceptImportError: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| pass | |
| requests=None |
| ) | ||
| # If neither requests or aiohttp is installed then inform user how to | ||
| # install them | ||
| if"aiohttp"notinsys.modulesand"requests"notinsys.modules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if"aiohttp"notinsys.modulesand"requests"notinsys.modules: | |
| ifnotrequestsandnotaiohttp: |
| ) | ||
| if"aiohttp"insys.modules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if"aiohttp"insys.modules: | |
| ifaiohttp: |
| f"GeoIP2-Python-Client/{geoip2.__version__}{aiohttp.http.SERVER_SOFTWARE}" | ||
| ) | ||
| if"requests"insys.modules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if"requests"insys.modules: | |
| ifrequests: |
sebix commentedJul 13, 2021
The extras-feature of setuptools is already a very easy way to "split" the requirements.
requests even comes with a built-in version of urllib3, so that won't be effective.
Although distributions ship "older" versions of requests, they patch the security issues in the library. Requiring very current versions, causes an upgrade of requests during the installation of geoip2, possible causing compatibility issues. Could you insteadrecommend a higher version (e.g. using an optional |
akx commentedAug 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This would be lovely to merge. I was considering bumping EDIT: See#177 for a modernized version of this PR for the current packaging. |
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Closesmaxmind#104 (supersedes it)Fixesmaxmind#101
Uh oh!
There was an error while loading.Please reload this page.
Linked to Issue#101. I noticed when installing the library that the dependency tree is quite deep. I have to install to a non-internet connected environment so I have to download all of the .whls manually which highlighted the issue.
I saw some opportunities for making the installation a bit leaner.
Have removed unused dependency
urllib3.Have made
aiohttpandrequestsoptional installs. User gets an error message when trying to import the libraries withoutrequestsoraiohttpinstalledSome figures on the size of the install for different configurations
This would be a big change to how this library is installed so this would need some maintainer feedback on this level of change.
Also I noticed a very specific version of
requestshas been picked>=2.24.0which is from June 2020 onwards. I'm not sure this package needs such a specific version looking the history of therequestspackage (https://github.com/psf/requests/blob/master/HISTORY.md). And it'll require the majority of people who use it who are already usingrequeststo need to uninstall their existing version and install 2.24.0.Since the minimum supported version of Python for this library is Python 3.6, and the earliest version of
requeststhat supports Python 3.6 is2.14.0I've suggested updating this dependency torequests>=2.14.0,<3.0.0. I've tested withrequests 2.14.0and all tests are passing with identical results.