jazzband / django-downloadview

Serve files with Django.
https://django-downloadview.readthedocs.io
Other
364 stars 58 forks source link

Update compatibility for Django 4.0 #188

Closed tari closed 2 years ago

tari commented 2 years ago

The only documented compatibility change is removing use of force_text (which was deprecated in Django 3.0) in favor of force_str (which has existed since before Django 1.11). There are also some changes in middleware because Django's MiddlewareMixin started requiring a get_response argument to the constructor.

Fixes #187.

codecov[bot] commented 2 years ago

Codecov Report

Merging #188 (e9fbb74) into master (0578729) will decrease coverage by 0.16%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   85.69%   85.52%   -0.17%     
==========================================
  Files          47       72      +25     
  Lines         608     1313     +705     
==========================================
+ Hits          521     1123     +602     
- Misses         87      190     +103     
Impacted Files Coverage Δ
demo/demoproject/settings.py 100.00% <ø> (+100.00%) :arrow_up:
setup.py 0.00% <ø> (ø)
django_downloadview/io.py 57.44% <100.00%> (ø)
django_downloadview/middlewares.py 81.73% <100.00%> (ø)
django_downloadview/views/virtual.py 42.85% <0.00%> (-57.15%) :arrow_down:
django_downloadview/shortcuts.py 50.00% <0.00%> (-50.00%) :arrow_down:
django_downloadview/nginx/settings.py 76.78% <0.00%> (-19.65%) :arrow_down:
django_downloadview/views/base.py 87.50% <0.00%> (-12.50%) :arrow_down:
django_downloadview/views/path.py 92.85% <0.00%> (-7.15%) :arrow_down:
django_downloadview/views/object.py 90.62% <0.00%> (-3.13%) :arrow_down:
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0578729...e9fbb74. Read the comment docs.

Archmonger commented 2 years ago

@tari Please add Django 4.0 as a CI test case.

tari commented 2 years ago

Added (as well as Python 3.10), but the tox configuration seems to duplicate parts of the test matrix and cause spurious failures; there seem to a few real failures as well that I have yet to investigate.

Archmonger commented 2 years ago

Only Django 3.2+ should support Python 3.10, so exclusions can be made for the rest.

tari commented 2 years ago

I fixed the middleware issues in c286c0e0b01f0ffe5b03123d7134543895a3cd0f, but it's a little bit of a hack since it's just hiding the problem of instantiating a middleware without any get_response function; doing that became an error rather than simply deprecated in Django 4.0.

Breaking the "try each delegate middleware" logic out of DownloadDispatcherMiddleware so it can be used without creating something that looks like a middleware but isn't might be a nicer solution, but I opted for the more expedient hack for now since that also seems to be part of the public API (though I didn't see mention of it in the documentation).


There's a remaining issue that's not Django 4.0-specific in that nose isn't compatible with Python 3.10 and is basically abandoned upstream, so testing probably needs to be migrated to use something else.

Archmonger commented 2 years ago

I'm personally not the project lead for this repo, but since they haven't voiced on this PR I'll provide my input.

Migrating away from Nose can be done in a separate PR, but tests need to be passed on this PR before merging. Which means this PR can focus on Django 4.0 compatibility but exclude Python 3.10. Then a separate issue and PR should be created for 3.10 compatibility (which requires replacing nose).

Additionally, my opinion is this PR should have a proper non-hacky solution for Django 4.0.

Natim commented 2 years ago

this PR can focus on Django 4.0 compatibility but exclude Python 3.10

Agreed

tari commented 2 years ago

Understood- I'll work out a better solution to the middlewares, and leave Python 3.10 out of the mix for now.

tari commented 2 years ago

I've rebased this on top of #189 for Python 3.10 support, and made some more considered changes to the middleware that I'm happy with now.

Archmonger commented 2 years ago

@Natim Just a reminder that this PR needs merging. Package is currently broken with Django 4+. I've confirmed the changes look good, but am leaving the final say to you.