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

Add support for Dj40, drop Py36 and Dj31 #1039

Merged
merged 9 commits into from Dec 19, 2021
Merged

Add support for Dj40, drop Py36 and Dj31 #1039

merged 9 commits into from Dec 19, 2021

Conversation

Andrew-Chen-Wang
Copy link
Member

@Andrew-Chen-Wang Andrew-Chen-Wang commented Dec 14, 2021

Fixes #1037

Description of the Change

Add support for Dj40, drop Py36 and Dj31

Dj31 and Py36 will be EOL end of this December of 2021. I do not want to hold back any PRs that need to support old versions of stuff, thus I have dropped those two.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@Andrew-Chen-Wang
Copy link
Member Author

Note: do not merge this as of yet. I'll need to run the quick migration.

@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #1039 (fd891d7) into master (d25cb27) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1039   +/-   ##
=======================================
  Coverage   96.64%   96.64%           
=======================================
  Files          31       31           
  Lines        1756     1756           
=======================================
  Hits         1697     1697           
  Misses         59       59           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d25cb27...fd891d7. Read the comment docs.

@Andrew-Chen-Wang
Copy link
Member Author

@n2ygk In the settings, you'll need to remove the required check for Python 3.6 due to the dropping of Python 3.6 in this PR. Thanks!

@Andrew-Chen-Wang
Copy link
Member Author

I believe there is a bug in Django which is mentioned in #1037. I'm not sure how to resolve it, but it seems like an unintended bug in Django itself, not our end. I've filed a bug (which will probably get closed wontfix) https://code.djangoproject.com/ticket/33366

@auvipy I will be taking finals for next 3 days, so please take over. Cheers :)

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

ok will check what remains

@auvipy
Copy link
Contributor

auvipy commented Dec 15, 2021

I think python 3.10 is not backported in django 3.2?

@auvipy
Copy link
Contributor

auvipy commented Dec 15, 2021

build (3.6) Expected — Waiting for status to be reported what is this build?

@Andrew-Chen-Wang
Copy link
Member Author

I removed Py36. Authors in settings can require certain builds to be successful before PR merge. Authors should disable this. If they don't then we can add Python 3.6 back in to circumvent.

@auvipy
Copy link
Contributor

auvipy commented Dec 15, 2021

I am not really sure why python 3.10 is failing

@Andrew-Chen-Wang
Copy link
Member Author

Please do not merge until the migration file is properly fixed.

@auvipy
Copy link
Contributor

auvipy commented Dec 15, 2021

we actually cant merge this due to required check. also the migration issue is not actual issue. thats due to django new settings/features as far as I know.

@Andrew-Chen-Wang
Copy link
Member Author

Andrew-Chen-Wang commented Dec 15, 2021

Yes, however, like Django 3.2, users kept creating migration files inside the package itself since people were using virtual envs. This is unintended behavior and can cause huge issues down the road when adding new migration files as migration file IDs will be duplicated due to an accidental dev-side migration.

TL;DR if we don't fix the migration file issue, people's production servers will crash.

@Andrew-Chen-Wang
Copy link
Member Author

@auvipy Python 3.10 failing due to main branch of Django. Somehow sqlite3.OperationalError: Cannot add a UNIQUE column this error is now popping up. Must mean either a bug or something new that we're not aware of.

@n2ygk @jezdez In the settings, you'll need to remove the required check for Python 3.6 due to the dropping of Python 3.6 in this PR. Thanks!

@auvipy
Copy link
Contributor

auvipy commented Dec 16, 2021

as I said this seems to be a regression in django https://github.com/django/django/pull/15205/files

@auvipy
Copy link
Contributor

auvipy commented Dec 16, 2021

Also I don't think it's a blocker for release. we can drop this after a new release

@Andrew-Chen-Wang
Copy link
Member Author

Andrew-Chen-Wang commented Dec 16, 2021

Sounds good! Also Django 3.2.9 added support for Py310

@auvipy
Copy link
Contributor

auvipy commented Dec 16, 2021

not a problem, we can do 1.6.1 soon :D

@jezdez
Copy link
Member

jezdez commented Dec 16, 2021

@n2ygk @jezdez In the settings, you'll need to remove the required check for Python 3.6 due to the dropping of Python 3.6 in this PR. Thanks!

Done that.

@jezdez
Copy link
Member

jezdez commented Dec 16, 2021

Also I don't think it's a blocker for release. we can drop this after a new release

FWIW dropping support for a Django or Python version shouldn't be done in a patch version, I'd suggest to do a 1.7.0 with this change applied.

Copy link
Member Author

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

@jezdez I think this should be merged though with a change in setup.py to disallow Django 4.0.0 but allow 4.0.1. That way it'd be included in this new minor release as it seems like current maintainers are fairly busy.

We can just delete the current git tag and re tag since not released on pypi yet

tox.ini Outdated Show resolved Hide resolved
@n2ygk n2ygk requested a review from jezdez December 19, 2021 14:30
@n2ygk n2ygk changed the title Add support for Dj40, drop Py36 and Dj31 WIP: Add support for Dj40, drop Py36 and Dj31 Dec 19, 2021
n2ygk and others added 3 commits December 19, 2021 09:41
* Installation should block 4.0.0 in my opinion to avoid anyone's production sit
e from going down due to this package. This is only the case if someone runs mak
emigrations on their production server (for any bad reason).
* Updated docs to reflect the changes in this PR
* As of Dec 19, 2021, Django 4.0.1 has not released changes to fix a regression.It is a user-end regression that does not affect usability, but it does affect user's belief that they need to create a migration. In @Andrew-Chen-Wang's past experience, that has led to production errors and an emergency migration of one of his past packages...
@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

@Andrew-Chen-Wang oops you and I crossed commits in the mail... I think the tox should be what I had if my understanding is correct:

-    dj40: Django>=4.0.1,<4.1
+    dj40: Django>=4.0.0,<4.1

@Andrew-Chen-Wang
Copy link
Member Author

Nope! Django 4.0.1 isn't released yet, but I do want to test 4.0. Please read my explanation above 😅 (sorry edited it awhile ago).

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

Nope! Django 4.0.1 isn't released yet, but I do want to test 4.0. Please read my explanation above 😅 (sorry edited it awhile ago).

D'oh!

Looks like everything but py310 passed the tests.

@Andrew-Chen-Wang
Copy link
Member Author

Andrew-Chen-Wang commented Dec 19, 2021

Resolved. Tests should be passing.

Hmm that's why auvipy removed it due to Django main failing on Python 3.10. I personally don't understand what's going on 😅, but what I do know now is that it's also failing on Python 3.9 and others, but Too successfully ignored those ones but didn't properly ignore Python 3.10. I'll get tox to ignore the errors then.

Also, do you mind requiring Python 3.10 in the settings? Thanks!

@Andrew-Chen-Wang
Copy link
Member Author

This should be the last PR, so it'll require deleting the 1.6.0 git tag release and recreating it to include this PR. Also enabling Python 3.10 requirement in CI in the GitHub settings. Finally, you'll be able to make the Jazzband release. Thanks for a great library!

@Andrew-Chen-Wang Andrew-Chen-Wang changed the title WIP: Add support for Dj40, drop Py36 and Dj31 Add support for Dj40, drop Py36 and Dj31 Dec 19, 2021
@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

Looks like a failure in 3.9 now so will need to investigate that.

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

Looks like a failure in 3.9 now so will need to investigate that.

Hmm. ran tox locally and 3.9 was OK:

  docs: commands succeeded
  py37-dj22: commands succeeded
  py38-dj22: commands succeeded
  py39-dj22: commands succeeded
  py37-dj32: commands succeeded
  py38-dj32: commands succeeded
  py39-dj32: commands succeeded
  py310-dj32: commands succeeded
  py38-dj40: commands succeeded
  py39-dj40: commands succeeded
  py310-dj40: commands succeeded
  py38-djmain: ignored failed command
  py39-djmain: ignored failed command
  py310-djmain: ignored failed command

(flake8 failed locally due to an old build directory laying around.)

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

I took the liberty of using this PR to update the CHANGELOG to be hopefully more user-friendly. We don't need to list every single PR and issue, only those that have an impact on a user considering upgrading.

@n2ygk n2ygk removed the request for review from jezdez December 19, 2021 21:13
@n2ygk n2ygk merged commit 6aeb1b2 into master Dec 19, 2021
@n2ygk n2ygk mentioned this pull request Dec 21, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Django 4.0 compatibility
4 participants