Skip to content
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

Replace calls to methods removed in Django v4 #1275

Merged
merged 8 commits into from
Feb 7, 2022

Conversation

MisterGlass
Copy link
Contributor

If I'm running the tests correctly this seems to be all that's needed to resolve #1274

@MisterGlass
Copy link
Contributor Author

I wasn't running the tests correctly. I ran them manually and found a few more easy changes.

I could use help adding it to the tox config & github actions properly.

@MisterGlass MisterGlass force-pushed the django-v4-compatibility branch 6 times, most recently from 5e33ecd to d22a5db Compare December 2, 2021 23:33
@MisterGlass
Copy link
Contributor Author

I updated the tox config & github actions, you can see the various versions tested here: https://github.com/MisterGlass/graphene-django/actions/runs/1533043305

@tim-schilling
Copy link
Contributor

@MisterGlass Shouldn't this be against main rather than the v2 branch? The latest version of graphene-django is 3.X

@MisterGlass
Copy link
Contributor Author

MisterGlass commented Dec 7, 2021

@MisterGlass Shouldn't this be against main rather than the v2 branch? The latest version of graphene-django is 3.X

The latest release on pypi is 2.15.0. When I started this PR I noticed the 3.0 beta hadn't been updated in a year, so I focused on a 2.x revision.

I'm not sure what state the 3.0 work is in (I'm a new contributor here) but I'd be happy to help make similar changes to it.

@tim-schilling
Copy link
Contributor

Ah, I did not see that. You can ignore my comment then.

@MisterGlass
Copy link
Contributor Author

MisterGlass commented Dec 7, 2021

edit oops

@MisterGlass MisterGlass closed this Dec 7, 2021
@MisterGlass MisterGlass reopened this Dec 7, 2021
@ulgens
Copy link
Collaborator

ulgens commented Dec 8, 2021

@MisterGlass Shouldn't this be against main rather than the v2 branch? The latest version of graphene-django is 3.X

I agree with this. With the previous Django - Python version update PR, the majority agreed it's too big of an update and it should be part of 3.0. Considering how long it took to release 3.0, i don't think it was the best decision but it's done. At this point, the 2.x version is legacy and I wouldn't expect any new version on that.

About the 3.0 release, I pinged people on Slack a couple of days ago but no one responded yet. Still, I think it's better to contribute to the main branch.

@MisterGlass
Copy link
Contributor Author

MisterGlass commented Dec 8, 2021

Django v4 is now officially released. This library is now the only thing preventing me (and I'm sure others) from upgrading.

The changes I made are minimal, aside from the test configuration I changed 11 lines, replacing deprecated functions with their new equivalents. I really can't see how 11 lines is too big of an update.

There is no mention in the documentation that I can find saying 2.x is legacy & there will be no updates. Was I supposed to be running the beta this whole time? If 2.x is legacy and 3.x is beta, then there is no current maintained version of graphene-django and I need to fork or migrate ASAP.

edit I really truly would be happy to contribute similar changes to 3.x, I just think the project should do this 2.x release so users can upgrade without waiting for 3.x to be finished.

@ulgens
Copy link
Collaborator

ulgens commented Dec 8, 2021

If 2.x is legacy and 3.x is beta, then there is no current maintained version of graphene-django and I need to fork or migrate ASAP.

I don't want to discourage anyone from contributing but I'd be lying if I say this is not the case.

@tim-schilling
Copy link
Contributor

@ulgens, what's blocking us from doing a dual release of 2.x and 3.x?

@ulgens
Copy link
Collaborator

ulgens commented Dec 9, 2021

@ulgens, what's blocking us from doing a dual release of 2.x and 3.x?

I'd say nothing is stopping us, but also I'm not in a place to call it.

@tim-schilling tim-schilling mentioned this pull request Dec 9, 2021
7 tasks
Copy link
Collaborator

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not abandon our commitment to keep shipping updates to v3, but we also have a commitment to keep up with Django releases. This PR should be accepted once it introduces a version bump (ie 2.16.0) to reflect the drop in support for Django v1. See graphene_django/__init__.py for the source of the version number.

We will commit to porting this PR to v3, but I won't be able to get to it right now. Please do us a favor and submit another PR for the main branch, even if it is broken.

@tim-schilling
Copy link
Contributor

tim-schilling commented Dec 10, 2021

We will commit to porting this PR to v3, but I won't be able to get to it right now. Please do us a favor and submit another PR for the main branch, even if it is broken.

This should be done with #1281, though I do need to investigate something regarding force_text.

Edit, looks like 1281 does handle the force_text -> force_str. Seems good.

@caiocarrara
Copy link

@zbyte64 sorry for the inconvenient ping. This is just a friendly reminder about this PR. Since your request for changes, MisterGlass updated the version. It would be great to have your approval here.

@jyotendra
Copy link

@zbyte64 can you plz review the changes and approve the merge. Seems like it's blocking issue#1284 since long.

@insomniac34
Copy link

Bump to get this merged in :)

@ulgens
Copy link
Collaborator

ulgens commented Jan 24, 2022

Hey everyone, thanks for keeping this alive. I have the permission to merge it if we think it's ready to be merged, but I'd like to have the test matrix cleaned first (removing old versions and simplifying it) If anyone is up for that, please go on. Otherwise, I'll try to handle it as soon as I found some spare time.

@MisterGlass
Copy link
Contributor Author

I'm happy to remove/simplify whatever you think is appropriate for this release, I had originally tried to keep as much compatibility testing as possible.

@jeremystretch
Copy link

jeremystretch commented Feb 7, 2022

Hi @ulgens! Is there a specific subset of the matrix you had in mind? Here's what it looks like currently in the PR:

Django 2.2 3.0 3.1 3.2 4.0
Python 3.6 x x x x
Python 3.7 x x x x
Python 3.8 x x x x x
Python 3.9 x x x x x
Python 3.10 x x

IMO there's a strong argument for dropping Python 3.6 support as it's been officially deprecated. It might also be reasonable to omit Django 2.2 if we need to further simplify, although that is technically still supported until April.

@ulgens
Copy link
Collaborator

ulgens commented Feb 7, 2022

I'd recommend removing 3.6 but keeping 2.2 for now.

@ulgens
Copy link
Collaborator

ulgens commented Feb 7, 2022

In any case, i'll just go and merge this. It took so long and I feel responsible for everyone. We can fix whatever left, after the merge.

@ulgens ulgens self-assigned this Feb 7, 2022
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
graphene_django/__init__.py Outdated Show resolved Hide resolved
ulgens and others added 4 commits February 7, 2022 17:08
Co-authored-by: Jeremy Stretch <jstretch@ns1.com>
Co-authored-by: Jeremy Stretch <jstretch@ns1.com>
Co-authored-by: Jeremy Stretch <jstretch@ns1.com>
@ulgens ulgens dismissed zbyte64’s stale review February 7, 2022 14:15

This PR is not ready to release as 2.16, but i'm merging it because it blocks the pipeline.

@ulgens ulgens self-requested a review February 7, 2022 14:16
@ulgens ulgens merged commit a7a8b3d into graphql-python:v2 Feb 7, 2022
@nextmat
Copy link

nextmat commented Feb 8, 2022

Thanks for getting this merged! Just to clarify, what is the likely release plan for this?

@ulgens
Copy link
Collaborator

ulgens commented Feb 8, 2022

Thanks for getting this merged! Just to clarify, what is the likely release plan for this?

I can't really there is one, but I plan to do some more cleaning under #1298 first.

keithhackbarth pushed a commit to keithhackbarth/graphene-django that referenced this pull request Mar 29, 2022
* Replace calls to deprecated methods

* Fix test config & Replace additional methods removed in django 4.0

* Update tox for official Django 4 release

* 2.16.0

* Revert version update

* Remove duplicate entry

Co-authored-by: Jeremy Stretch <jstretch@ns1.com>

* Limit max Django version

Co-authored-by: Jeremy Stretch <jstretch@ns1.com>

* Remove Python 3.5 (deprecated) from tox

Co-authored-by: Jeremy Stretch <jstretch@ns1.com>

Co-authored-by: Ülgen Sarıkavak <ulgens@users.noreply.github.com>
Co-authored-by: Jeremy Stretch <jstretch@ns1.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants