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

Django 4.x #126

Merged
merged 29 commits into from
Aug 11, 2022
Merged

Django 4.x #126

merged 29 commits into from
Aug 11, 2022

Conversation

mpasternak
Copy link
Contributor

@mpasternak mpasternak commented Aug 7, 2022

Fixes #122.

@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #126 (fa72b27) into master (8e8b76f) will decrease coverage by 0.19%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   75.24%   75.04%   -0.20%     
==========================================
  Files          13       13              
  Lines         513      521       +8     
  Branches       79       78       -1     
==========================================
+ Hits          386      391       +5     
- Misses         95       99       +4     
+ Partials       32       31       -1     
Impacted Files Coverage Δ
dbtemplates/conf.py 64.44% <0.00%> (ø)
dbtemplates/test_cases.py 99.00% <ø> (ø)
dbtemplates/admin.py 48.31% <38.46%> (-0.50%) ⬇️
dbtemplates/models.py 88.23% <50.00%> (-3.44%) ⬇️
dbtemplates/management/commands/sync_templates.py 69.33% <72.72%> (-0.94%) ⬇️
dbtemplates/apps.py 100.00% <100.00%> (ø)
dbtemplates/loader.py 75.55% <100.00%> (ø)
dbtemplates/utils/cache.py 88.88% <100.00%> (+8.24%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mpasternak mpasternak requested a review from jezdez August 7, 2022 22:50
@mpasternak mpasternak self-assigned this Aug 8, 2022
@mpasternak mpasternak requested a review from hugovk August 8, 2022 22:22
@@ -1,6 +1,13 @@
Changelog
=========

v4.1 (2022-06-15, iplweb)
Copy link
Member

Choose a reason for hiding this comment

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

What's iplweb?

Suggested change
v4.1 (2022-06-15, iplweb)
v4.1 (2022-06-15)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's iplweb ! Changelog updated. I'll be happy to remove my fork as soon as we'll be able to develop dbtemplates via Jazzband.

Copy link
Member

Choose a reason for hiding this comment

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

I see you got added to the project in #121 (comment) 👍

Are there any remaining blockers to develop via Jazzband? (Other than this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a few forked projects because they were not maintained and instead of renaming them I decided to keep the "-iplweb" suffix. In some cases, like in https://pypi.org/project/django-password-policies-iplweb/ they became pretty popular.

As I had no contact with Jazzband team for some time and I really needed dbtemplates functionality for my app, I did the same thing with it. You can see the remnants of my changes, probably I should have cleaned that up before committing.

If you will find me a worthy partner, I suppose I'll bring some more projects to Jazzband with me in the future.

@mpasternak mpasternak requested a review from hugovk August 9, 2022 15:43
Co-authored-by: Hugo van Kemenade <[email protected]>
Copy link
Member

@hugovk hugovk 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 update tox.ini to something like this to make sure we're testing all the versions:

diff --git a/tox.ini b/tox.ini
index 16d6e9c..6d8861c 100644
--- a/tox.ini
+++ b/tox.ini
@@ -4,8 +4,8 @@ usedevelop = True
 minversion = 1.8
 envlist =
     flake8
-    py3{7,8,9}-dj22
     py3{7,8,9,10,11}-dj32
+    py3{8,9,10,11}-dj{40,41,main}
 
 [gh-actions]
 python =

Co-authored-by: Hugo van Kemenade <[email protected]>
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks, almost there!

Comment on lines 12 to 14
if (django.VERSION[0] >= 3 and django.VERSION[1] >= 2) or django.VERSION[
0
] >= 4: # noqa
Copy link
Member

Choose a reason for hiding this comment

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

(django.VERSION[0] >= 3 and django.VERSION[1] >= 2) is true for 3.2+ and also 4.2+ and 5.2+ etc.

Not a problem in this case, but it's good practice to compare to tuples instead (and we can skip the second part of the OR):

Suggested change
if (django.VERSION[0] >= 3 and django.VERSION[1] >= 2) or django.VERSION[
0
] >= 4: # noqa
if (django.VERSION >= (3, 2):

But we only support Django 3.2+ so we can remove this whole check and simplify the function.

It's a compatibility wrapper, so we can probably remove the whole function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Django 3.2 is the lowest supported version I decided I'll remove it.


* Moved test runner to GitHub Actions:

http://github.com/jazzband/django-dbtemplates/actions

* Django 4.x support
Copy link
Member

Choose a reason for hiding this comment

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

Let's also mention dropping Django < 3.2.

@mpasternak
Copy link
Contributor Author

Thanks, almost there!

I love it! Having a supportive reviewer who takes care of the details is great, I hope I can learn from you in the future too!

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

@mpasternak mpasternak merged commit 13bacac into jazzband:master Aug 11, 2022
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.

Does not work with django 4
2 participants