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

Make NAV run on Django 3.2 #2313

Closed
wants to merge 16 commits into from
Closed

Make NAV run on Django 3.2 #2313

wants to merge 16 commits into from

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Oct 14, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 14, 2021

Test results

       6 files         6 suites   8m 8s ⏱️
2 794 tests 2 698 ✔️   96 💤 0
5 588 runs  5 396 ✔️ 192 💤 0

Results for commit 0eda8c5.

♻️ This comment has been updated with latest results.

hmpf added 15 commits October 22, 2021 07:39
In order for the tests to run at all, six must be working. Better to
do a global change that temporarily fixes the problem everywhere and
then get rid of six, then having to reanalyse all places that uses six
while the tests are not working.

As long as we support Django 2.2, which is the last to support Python
2.7, we need something like six. When we no longer support 2.2, we can
get rid of six *and all other workrounds for running on Python 2.7 and
Python 3 at the same time* in a "stop supporting python 2.7"-branch.
NAV depends on DRF older than 3.10, which don't support newer
Django's and Python's.

* Use drf 3.8's "action" instead *_route
* Rename drf base_name to basename to support drf > 3.9
Django 2.2 has *_text, Django 3.2 has both, Django 4.0 has
*_str. Gets rid of a warning.
Newest crispy-forms might not work with Django 2.2 so we have to wait.
.. as in: supported since Django 2.2. It might be that repr()-ing any
Response is also safe now and that the test-function is no longer
needed.
Gets rid of the following warnings:

RemovedInDjango40Warning:
The {% ifequal %} template tag is deprecated in favor of {% if %}.

RemovedInDjango40Warning:
The {% ifnotequal %} template tag is deprecated in favor of {% if %}.
Gets rid of warning:

RemovedInDjango40Warning:
django.conf.urls.url() is deprecated in favor of django.urls.re_path().
Gets rid of warning:

models.TimePeriod: (models.W042) Auto-created primary key used when not
defining a primary key type, by default 'django.db.models.AutoField'.
        HINT: Configure the DEFAULT_AUTO_FIELD setting or the
        NavModelsConfig.default_auto_field attribute to point to
        a subclass of AutoField, e.g. 'django.db.models.BigAutoField'.
Gets rid of exception on Django 3.2:

AttributeError: module 'django.forms.forms' has no attribute 'pretty_name'
Also Import lru_cache from seddable place
@hmpf
Copy link
Contributor Author

hmpf commented Oct 22, 2021

This PR will be split into multiple smaller PRs for ease of review.

@hmpf
Copy link
Contributor Author

hmpf commented Oct 22, 2021

First PR to merge: #2315. Does not need Django 3.2 to run but prevents exceptions when later testing on 3.2.

@hmpf
Copy link
Contributor Author

hmpf commented Oct 22, 2021

Second PR, can be merged independently from the first: #2316. Upgrade dependencies that need changes in our code.

@hmpf
Copy link
Contributor Author

hmpf commented Oct 22, 2021

Third PR, depends on the two previous PRs: #2317. Upgrades the actual code, and runs the tests on 3.2 in addition to 2.2.

@hmpf
Copy link
Contributor Author

hmpf commented Oct 22, 2021

Fourth and last PR, cutting down on warnings: #2318. Depends on #2317.

@hmpf
Copy link
Contributor Author

hmpf commented Oct 25, 2021

Note how judicious (ab)use of git rebase has split and reordered the commits. In order for this to be possible at all (to trigger warnings and exceptions) the tests had to run on 3.2, which means #2317 came first. Later I realized #2315 and #2316 were useful even without running on Django 3.2 (since testing on 3.2 in addition to 2.2 takes longer) so they were moved in front.

@lunkwill42
Copy link
Member

Given that all the split PRs have been reviewed and merged, I assume this combined one can be closed now. Thank you!

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.

2 participants