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

Update Python & Django supported versions + migrate to ruff #551

Merged
merged 6 commits into from
Mar 30, 2024

Conversation

samiashi
Copy link

@samiashi samiashi commented Mar 29, 2024

Update Python supported versions - https://devguide.python.org/versions/

  • Drop support for Python 3.6 & 3.7
  • Add support for Python 3.11 & 3.12

Update Django supported versions - https://www.djangoproject.com/download/#supported-versions

  • Drop support for Django 2.2, 3.0 & 3.1
  • Add support for Django 4.2 & 5.0

Migrate to ruff - https://github.com/astral-sh/ruff

  • Replace flake8 & black with ruff
  • Format files with ruff

Miscellaneous

  • Fixed markdown violations

@samiashi samiashi force-pushed the bump-python-django branch 5 times, most recently from 3568c99 to 3d189b6 Compare March 29, 2024 11:38
@samiashi samiashi changed the title Update Python & Django supported versions Update Python & Django supported versions + migrate to ruff Mar 29, 2024
@samiashi samiashi force-pushed the bump-python-django branch 3 times, most recently from 263fd12 to 8a20f52 Compare March 29, 2024 13:11
Copy link
Collaborator

@PacificGilly PacificGilly left a comment

Choose a reason for hiding this comment

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

Amazing PR, and thanks for grouping these nicely in all the commits. Looks like you still need to rebase first though. Approving to trigger the CI to check this builds properly

tox.ini Outdated Show resolved Hide resolved
@@ -36,7 +37,7 @@ exclude = ["tests", "docs"]

[tool.poetry.dependencies]
python = ">=3.8"
django = ">=4.2,<5.0"
django = ">=4.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Further changes are needed in 5.1 of Django to maintain support - #534

Suggested change
django = ">=4.2"
django = ">=4.2,<5.1"

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, if we do this, then people won't even bother installing the app beyond this version, nor will they be able to tolerate or fix minor issues in unsupported versions.

The decision to have an upper bound is contentious I admit, if this package is absolutely broken on Django 5.1 then maybe it's a good idea, but otherwise, maybe we should leave it open and endeavour to resolve issues as they arise

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah good point. It's not totally broken in 5.1. This seems to only impact fieldsets, so likely still a widespread issue.

@@ -42,7 +42,7 @@ def test_logout(admin_client):
"""
url = reverse("admin:logout")

response = admin_client.get(url)
response = admin_client.post(url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting this wasn't already done, as I thought we fixed the logout issue already

Copy link
Author

@samiashi samiashi Mar 30, 2024

Choose a reason for hiding this comment

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

The logout via GET was deprecated in Django 4.1 hence why it wasn't caught earlier
https://docs.djangoproject.com/en/4.2/releases/4.1/#log-out-via-get

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think it might have been missed in #544

tox.ini Outdated Show resolved Hide resolved
@PacificGilly
Copy link
Collaborator

@farridav the 3.11 build is failing
https://github.com/farridav/django-jazzmin/actions/runs/8481330654/job/23245748961?pr=551

  coveralls.exception.CoverallsException: Not on TravisCI. You have to provide either repo_token in .coveralls.yml or set the COVERALLS_REPO_TOKEN env var.

I'm not sure why it can't find the COVERALLS_REPO_TOKEN env var as your PRs work without issue. Without seeing the settings, I'm guessing this token is also working with your account maybe?
https://github.com/samiashi/django-jazzmin/blob/8a20f52fed70ee8de7a80ae7c2a9d093f3a09830/.github/workflows/build.yml#L18

Copy link
Owner

@farridav farridav left a comment

Choose a reason for hiding this comment

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

Some great changes here, and will help us with maintenance.

.gitignore Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
jazzmin/__init__.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@@ -36,7 +37,7 @@ exclude = ["tests", "docs"]

[tool.poetry.dependencies]
python = ">=3.8"
django = ">=4.2,<5.0"
django = ">=4.2"
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, if we do this, then people won't even bother installing the app beyond this version, nor will they be able to tolerate or fix minor issues in unsupported versions.

The decision to have an upper bound is contentious I admit, if this package is absolutely broken on Django 5.1 then maybe it's a good idea, but otherwise, maybe we should leave it open and endeavour to resolve issues as they arise

@farridav
Copy link
Owner

Yeah it's odd, probably some sort of a permissions issue, I will see if I can make sense of it and get the coveralls token working properly

@farridav
Copy link
Owner

Right, lets get this moving.. a massive step in the right direction here ..

Next steps will be to make the entire codebase properly typed, start making use of pydantic (or at least data classes) for configuration.

And get some better test coverage :)

@farridav
Copy link
Owner

Can we get a reabase on this ? Then we can get it merged ...

@samiashi
Copy link
Author

Can we get a reabase on this ? Then we can get it merged ...

@farridav Should be rebased with master now

@farridav farridav merged commit 4150ac7 into farridav:master Mar 30, 2024
4 of 5 checks passed
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.

3 participants