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

Drop support for Python 3.8, added support for Django 5.1 #1421

Merged
merged 3 commits into from
Nov 24, 2024

Conversation

tim-schilling
Copy link
Contributor

@tim-schilling tim-schilling commented Nov 19, 2024

Description

  • Drop support for Python 3.8
  • Added support for Django 5.1
  • Updated pre-commit versions and applied formatters

Related Issue

Resolves #1388.

Motivation and Context

Python 3.8 has reached EoL

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Is dropping a python version a breaking change?

Checklist:

  • I have run the pre-commit run command to format and lint.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@tim-schilling
Copy link
Contributor Author

@ddabble alright, I'm done for the day. I think I created the PRs in reverse order, but 🤷 . We'll get there.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.96%. Comparing base (4a8756a) to head (ca9ff34).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1421      +/-   ##
==========================================
+ Coverage   96.81%   96.96%   +0.14%     
==========================================
  Files          24       24              
  Lines        1446     1448       +2     
  Branches      236      236              
==========================================
+ Hits         1400     1404       +4     
+ Misses         25       24       -1     
+ Partials       21       20       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@anyidea
Copy link

anyidea commented Nov 20, 2024

#1388 i think this issue need to be resolved

@tim-schilling
Copy link
Contributor Author

@anyidea would you be interested in doing the follow-up PR to handle that deprecation?

@ulgens
Copy link
Member

ulgens commented Nov 21, 2024

@tim-schilling If this PR is not ready to merge, Is it okay if we split it into multiple parts? pre-commit updates, 3.8 drop and 5.1 support can be handled separately, which I believe would lead to faster merges.

@tim-schilling
Copy link
Contributor Author

@ulgens I think in either case, the PR needs to be reviewed which is the blocker.

@ddabble
Copy link
Member

ddabble commented Nov 24, 2024

If this PR is not ready to merge, Is it okay if we split it into multiple parts? pre-commit updates, 3.8 drop and 5.1 support can be handled separately, which I believe would lead to faster merges.

I agree that splitting PRs like this in general lowers the bar of reviewing them, with regards to the time required to do so properly 👍 Though I think the size of this PR is absolutely manageable, so it's ok :)

@ddabble
Copy link
Member

ddabble commented Nov 24, 2024

@tim-schilling Would you mind if I rebased the commits? Both to add a few things that I think should be added (I'll request your review in any case 🙂), and to reorganize the commits to have one for dropping Python 3.8 and one for adding Django 5.1.

@tim-schilling
Copy link
Contributor Author

Sounds good to me!

Copy link
Member

@ddabble ddabble left a comment

Choose a reason for hiding this comment

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

The main changes that I added:

  • Updated the version table in index.rst
  • Removed some unused typing imports
  • Merged the dj{50,51} and djmain envlist entries in tox.ini

Great work! 😊

Comment on lines +903 to +910
cache_name = (
# DEV: Remove this when support for Django 5.0 has been dropped
self.field.remote_field.get_cache_name()
if django.VERSION < (5, 1)
else self.field.remote_field.cache_name
)
try:
return self.instance._prefetched_objects_cache[
self.field.remote_field.get_cache_name()
]
return self.instance._prefetched_objects_cache[cache_name]
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the cache_name property was added in Django 5.1, so I corrected the version numbers 🙂

Also, the tests didn't fail because of the except (AttributeError, KeyError): below, so I moved the cache_name variable out of the try block, to make the except only catch errors with the _prefetched_objects_cache[cache_name] line (self.field.remote_field should always have a cache_name/get_cache_name() value, to my understanding).

Comment on lines -34 to -35
# DEV: uncomment this when the `pyproject-fmt` pre-commit hook stops removing it
#"Programming Language :: Python :: 3.13",
Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks :)

@ddabble
Copy link
Member

ddabble commented Nov 24, 2024

@tim-schilling When you think the PR is ready, let's do a merge commit, since the commits have been cleaned up 🙂

Also, thanks @ulgens and @anyidea, for your input!

@tim-schilling tim-schilling merged commit 98462de into jazzband:master Nov 24, 2024
20 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.

Request for Django 5.1 Compatibility
4 participants