Modernize support for Django 5.2 and 6.0#14
Conversation
| | 3.14 | 6.0, 5.2 | | ||
| +----------------+--------------------+ | ||
| | 3.13 | 6.0, 5.2 | | ||
| +----------------+--------------------+ | ||
| | 3.10 | 4.2, 4.1, 4.0, 3.2 | | ||
| | 3.12 | 6.0, 5.2 | | ||
| +----------------+--------------------+ | ||
| | 3.9 | 4.2, 4.1, 4.0, 3.2 | | ||
| | 3.11 | 5.2 | | ||
| +----------------+--------------------+ | ||
| | 3.8 | 4.2, 4.1, 4.0, 3.2 | | ||
| | 3.10 | 5.2 | |
There was a problem hiding this comment.
Using the Django documentation as reference: What Python version can I use with Django?
On this PR creation these are the references:
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 |
There was a problem hiding this comment.
We have a test that checks the output of the node command. Although some systems already come with a version of Node installed, I decided to be more conservative and ensure that we have Node installed.
| try: | ||
| from django.urls import get_resolver | ||
| except ImportError: | ||
| from django.core.urlresolvers import get_resolver | ||
|
|
||
| REQUIRES_SYSTEM_CHECKS = [False] | ||
|
|
There was a problem hiding this comment.
Since we will no longer support very old versions, this and other similar fallbacks have been removed.
| js2py = None | ||
| # Js2Py relies on Python bytecode details that changed in Python 3.13. | ||
| if sys.version_info < (3, 13): | ||
| try: | ||
| import js2py | ||
| except (ImportError, RuntimeError): | ||
| js2py = None |
There was a problem hiding this comment.
You can see more details about the issue here. There's a fork that apparently solves the problem, but I think it would add unnecessary overhead to the library at the moment.
| if sys.version < '3': | ||
| text_type = unicode # NOQA | ||
| else: | ||
| text_type = str |
There was a problem hiding this comment.
Fallbacks for older versions of Python have also been removed.
pyanderson
requested review from
Copilot and
fjsj
and removed request for
Copilot
| 'context_processors': [ | ||
| 'django.template.context_processors.request', | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Is this now necessary for the lib to work? If so, is it documented?
There was a problem hiding this comment.
No, this is only needed by the test setup, not by the library. But I can move it to an override decorator.
There was a problem hiding this comment.
Fine to keep here, maybe add a comment saying this is for tests only
|
|
||
| self.assertEqual(node_jseval(module), url(node_jseval)) | ||
| self.assertEqual(js2py_jseval('(function () {{ {} }}())'.format(script)), url(js2py_jseval)) | ||
| if js2py is not None: |
There was a problem hiding this comment.
ideally the test should test with and without js2py (if it's optional)
There was a problem hiding this comment.
Agree. The current tox/GitHub Actions matrix already exercises both paths: with js2py on Python 3.10-3.12 and without it on 3.13+. Do you recommend any changes?
| inner_urlresolver = urlresolvers.get_ns_resolver( | ||
| inner_ns_path, | ||
| inner_urlresolver, | ||
| tuple(getattr(urlresolver.pattern, 'converters', {}).items()), |
There was a problem hiding this comment.
is this getattr necessary? is converters added by the lib or by a dep?
There was a problem hiding this comment.
No, it's not necessary. I only kept it as a defensive compatibility measure, but I looked at the Django API, and it's really not needed, so I'll remove it.
|
|
||
| Django JS Reverse is a small django app that makes url handling of | ||
| `named urls <https://docs.djangoproject.com/en/dev/topics/http/urls/#naming-url-patterns>`__ in javascript easy and non-annoying.. | ||
| `named urls <https://docs.djangoproject.com/en/6.0/topics/http/urls/#naming-url-patterns>`__ in javascript easy and non-annoying.. |
There was a problem hiding this comment.
I think stable it's better, gonna change it.
fjsj
left a comment
fjsj
left a comment
There was a problem hiding this comment.
Looks good overall, I left some questions
fjsj
left a comment
fjsj
left a comment
There was a problem hiding this comment.
LGTM, I think the new release should be a major one to avoid breaking old Django installs.
2 participants
Summary
This PR modernizes
django-js-reversefor currently supported Django releases.What changed
js2pylimitation on Python 3.13+How to test
python -m pip install tox.python -m tox -e py313-django52.python -m tox -e py313-django60.python -m tox -e lint.python -m tox -e prepare_npm.python -m tox -e release.Notes
js2pyis skipped on Python 3.13+ because it depends on Python bytecode details that changed in Python 3.13.