-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
base: master
Are you sure you want to change the base?
Conversation
- 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
Codecov ReportAttention: Patch coverage is
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. |
Created an issue to get the changes released: jazzband/help#383 |
There was a problem hiding this 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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
I just saw that @dokterbob already released v1.0 to PyPi 🥳 |
Before merging please address the code quality comments I made. Don't leave commented lines and remove the patches for older Django versions. |
Update python and django versions
This PR addresses the following issues: