-
-
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?
Changes from all commits
04cfe20
00bf86d
f718dfa
ee835af
ca661b2
f5dd973
abf02a2
92dc07c
9409845
d5300e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
from pkg_resources import get_distribution, DistributionNotFound | ||
from importlib.metadata import version, PackageNotFoundError | ||
|
||
try: | ||
__version__ = get_distribution("django-newsletter").version | ||
except DistributionNotFound: | ||
__version__ = version("django-newsletter") | ||
except PackageNotFoundError: | ||
# package is not installed | ||
__version__ = None |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
# Generated by Django 5.0.6 on 2024-05-08 15:28 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
dependencies = [ | ||
("newsletter", "0008_longer_subscription_name"), | ||
] | ||
|
||
operations = [ | ||
migrations.AlterField( | ||
model_name="article", | ||
name="id", | ||
field=models.BigAutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="attachment", | ||
name="id", | ||
field=models.BigAutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="message", | ||
name="id", | ||
field=models.BigAutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="newsletter", | ||
name="id", | ||
field=models.BigAutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="submission", | ||
name="id", | ||
field=models.BigAutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
migrations.AlterField( | ||
model_name="subscription", | ||
name="id", | ||
field=models.BigAutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
] |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
from django.test import TestCase | ||
from importlib.metadata import version, PackageNotFoundError | ||
from newsletter import __version__ | ||
|
||
class TestNewsletterInit(TestCase): | ||
|
||
def test_version(self): | ||
try: | ||
expected_version = version("django-newsletter") | ||
except PackageNotFoundError: | ||
expected_version = None | ||
self.assertEqual(__version__, expected_version) | ||
|
||
def test_not_existing_version(self): | ||
try: | ||
expected_version = version("django-not-existing-newsletter") | ||
except PackageNotFoundError: | ||
expected_version = None | ||
self.assertIsNone(expected_version) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ def test_validate_email_nouser_noerror(self): | |
def test_validate_email_nouser_error(self): | ||
""" Test validate_email_nouser where error is raised. """ | ||
User = get_user_model() | ||
password = User.objects.make_random_password() | ||
password = 'testpassword123' | ||
user = User.objects.create_user( | ||
'john', '[email protected]', password) | ||
user.save() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better use a |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -582,7 +582,7 @@ def test_subscribe_request_post_existinguser_email(self): | |
""" Post the subscription form without email shoud fail. """ | ||
|
||
User = get_user_model() | ||
password = User.objects.make_random_password() | ||
password = 'testpassword123' | ||
user = User.objects.create_user( | ||
'john', '[email protected]', password) | ||
user.save() | ||
|
@@ -781,7 +781,7 @@ def test_user_update(self): | |
""" | ||
|
||
User = get_user_model() | ||
password = User.objects.make_random_password() | ||
password = 'testpassword123' | ||
user = User.objects.create_user( | ||
'john', '[email protected]', password) | ||
user.save() | ||
|
@@ -1055,7 +1055,7 @@ def test_update_request_post_existinguser_email(self): | |
""" Post the update form without email shoud fail. """ | ||
|
||
User = get_user_model() | ||
password = User.objects.make_random_password() | ||
password = 'testpassword123' | ||
user = User.objects.create_user( | ||
'john', '[email protected]', password) | ||
user.save() | ||
|
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.