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

Adding tests and support for Django 4.2 - 5x and removing EOL versions of python and django #394

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

sebastian-muthwill
Copy link
Member

@sebastian-muthwill sebastian-muthwill commented Dec 10, 2024

Update python and django versions

  • dropped tests for Python <3.7
  • dropped tests for Django <4.2
  • added tests for Python 3.11, 3.12, 3.13
  • added tests for Django >=4.2

This PR addresses the following issues:

aclark4life and others added 3 commits May 7, 2024 20:58
- dropped tests for Python <3.7
- dropped tests for Django <4.2
- added tests for Python 3.11, 3.12, 3.13
- added tests for Django >=4.2

as discussed in #392
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.51%. Comparing base (c9d7bf2) to head (9409845).

Files with missing lines Patch % Lines
newsletter/__init__.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage   86.02%   86.51%   +0.49%     
==========================================
  Files          16       16              
  Lines        1302     1313      +11     
  Branches      138      136       -2     
==========================================
+ Hits         1120     1136      +16     
+ Misses        135      131       -4     
+ Partials       47       46       -1     

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

@sebastian-muthwill
Copy link
Member Author

Created an issue to get the changes released: jazzband/help#383

Copy link
Contributor

@newearthmartin newearthmartin left a comment

Choose a reason for hiding this comment

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

Let's not leave Django versions out unless we have a good reason.
Let's keep LooseVersion if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with LooseVersion? It's better than manually parsing the version number. Please leave like it was unless there is some good reason.

(Also it's better to just remove code instead of leaving it commented)

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH this part I was already in the Django-5 branch I based my changes on and I did not checked that.
From what I see this is a patch in order to support Django versions <1.9.
It would depend here on the general decision if such old versions need to be supported or not.

Copy link
Contributor

@newearthmartin newearthmartin Dec 13, 2024

Choose a reason for hiding this comment

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

It's not a good practice to leave commented lines. Please remove them.

And also there is no point in removing LooseVersion and then manually parsing the version. You can just revert that.

If we are dropping Django < 1.9 , then these lines should all be gone. Otherwise they should remain as they were before, they were more clear than the new ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better use a TEST_PASSWORD variable if we are going to repeat it all over

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing support for those Django versions? What's the reason? Is there something incompatible?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is maybe more of a "philosophical" question.
I try to follow in all my projects the release cycle of Python and Django and migrate the code base always at least to the latest LTS version.
In general it becomes more and more challenging to support EOL versions because it requires patches for compatibility purpose and this effects the maintainability (like here: https://github.com/jazzband/django-newsletter/pull/394/files/d5300e560ad14dda0c625290236e00d7b98d86b7#diff-4d097d8e2ddb447f443dc2a92699225f61c2f7370d077988d93c83cc572d71f9)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of dropping support for older versions that require these patches. In that case we need to remove the patches, too. But that will not take us up to 4.2. We can support more versions.

Remember that this is not "your" or "my" project. I also keep my projects always on the latest versions, but this library has been around for many years and there may be many projects using older Django versions. We don't want to force them to upgrade unnecessarily.

@sebastian-muthwill
Copy link
Member Author

sebastian-muthwill commented Dec 12, 2024

I just saw that @dokterbob already released v1.0 to PyPi 🥳
May we have your opinion here as well. I think we should at least merge this PR to master as well to keep master and PyPi version in sync.
After that we can discuss if older versions should be supported or not.

@newearthmartin
Copy link
Contributor

Before merging please address the code quality comments I made. Don't leave commented lines and remove the patches for older Django versions.

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.

pkg_resources is deprecated in python 3.12 Django 4.1 support?
3 participants