-
Notifications
You must be signed in to change notification settings - Fork 768
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
Fix test Client headers for Django 4.2 #1465
Conversation
since it could be quite heavy
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.
Thanks for quickly adding this @kiendang! Looks good. just one point for quick discussion
graphene_django/utils/utils.py
Outdated
|
||
|
||
def __django_version(): | ||
from pkg_resources import get_distribution |
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.
There's a warning at the top here https://setuptools.pypa.io/en/latest/pkg_resources.html saying
Use of pkg_resources is deprecated in favor of importlib.resources, importlib.metadata and their backports (importlib_resources, importlib_metadata). Some useful APIs are also provided by packaging (e.g. requirements and version parsing). Users should refrain from new usage of pkg_resources and should work to port to importlib-based solutions.
Instead of parse_version()
, it looks like we could use https://packaging.pypa.io/en/stable/version.html (from packaging.version import parse
). And for getting the currently-installed django version string, it looks like import django; django.get_version()
would work (or alternatively importlib.metadata.version
seems to be the substitute for get_distribution("...").parsed_version
).
Unsure how important this deprecation warning is, but seems like it may be worthwhile to avoid the new code depending on this, as they suggest.
Another alternative could be something like this (if we want to avoid packaging
for instance):
import django
_DJANGO_VERSION_AT_LEAST_4_2 = django.VERSION[0] > 4 or (django.VERSION[0] >= 4 and django.VERSION[1] >= 2)
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.
@sjdemartini @superlevure Thanks for the review. I know about pkg_resources
being deprecated in favor of importlib...
modules. However went for pkg_resources.get_distribution("...").parsed_version
here since we need to compare 2 semvers while importlib.metadata.version
only returns the version string, unparsed.
django.VERSION
looks like exactly what we need which I was not aware of. Will convert to that. Thanks for pointing that out!
graphene_django/utils/utils.py
Outdated
|
||
|
||
def __django_version(): | ||
from pkg_resources import get_distribution |
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.
The usage of pkg_resources
is deprecated: https://setuptools.pypa.io/en/latest/pkg_resources.html
How about using:
from django import VERSION
print(VERSION)
# (4, 1, 11, 'final', 0)
instead?
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.
Looks like @sjdemartini beat me to it
Fix #1464