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

Fix deprecation warnings #5680

Merged
merged 43 commits into from
May 1, 2020
Merged

Fix deprecation warnings #5680

merged 43 commits into from
May 1, 2020

Conversation

cwdavies
Copy link
Contributor

@cwdavies cwdavies commented Apr 22, 2020

When tox is run with python -Wd there are a multitude of deprecation warnings logged
The following changes are made to fix many of these deprecations:

  1. Change assertRaisesRegexp to assertRaisesRegex
  2. Replace assertRegexMatch with assertRegex
  3. Change assertEquals to assertEqual
  4. The warn method is deprecated, use warning instead
  5. The modelcluster.tags module has been moved to modelcluster.contrib.taggit
  6. The unescape method is deprecated and will be removed in 3.5, use html.unescape() instead.
  7. The imp module is deprecated in favour of importlib
  8. Change to is_safe_url used in login_with_lockout in v1\view\__init__.py for Django 2.2
  9. Change to _post_feedback in v1\tests\handlers\blocks\test_feedback.py to fix:
    TypeError: Cannot encode None as POST data

Additions

  • handle_404_error in cfgov/urls.py
  • test handle_404_error in cfgov/tests/test_urls.py
  • test_handle_404_error_case_insensitive_redirect in cfgov/tests/test_urls.py

Removals

  • Django 1.10 imports

Testing

Update tox.ini to add -Wd option to python command line as follows:
python -b -Wd -m coverage run --source='.' manage.py test {posargs}

  1. tox -e unittest-current
  2. tox -e unittest-future

Notes

  • There are far too many deprecation warnings to fix all of them in this Pull Request

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the CFPB development guidelines
  • Passes all existing automated tests
  • Reviewers requested with the Reviewers tool ➡️

@willbarton
Copy link
Member

@cwdavies could you look at the test coverage failures? It's probably only failing because you touched those files and they don't have full coverage, not because of changes you made specifically.

But I think it'd be good to add coverage as we find places like that.

@cwdavies
Copy link
Contributor Author

cwdavies commented Apr 27, 2020

@cwdavies could you look at the test coverage failures? It's probably only failing because you touched those files and they don't have full coverage, not because of changes you made specifically.

But I think it'd be good to add coverage as we find places like that.

This file is concerning which I will attempt to address:
cfgov/v1/management/commands/add_images.py (0.0%): Missing lines 5,42
cfgov/cfgov/urls.py (83.3%): Missing lines 624

@willbarton willbarton mentioned this pull request Apr 30, 2020
11 tasks
cfgov/cfgov/tests/test_urls.py Outdated Show resolved Hide resolved
cfgov/cfgov/urls.py Outdated Show resolved Hide resolved
@cwdavies
Copy link
Contributor Author

https://docs.djangoproject.com/en/stable/ref/contrib/redirects/

Django comes with an optional redirects application. It lets you store redirects in a database and handles the redirecting for you. It uses the HTTP response status code 301 Moved Permanently by default.

https://docs.djangoproject.com/en/stable/ref/request-response/#django.http.HttpResponsePermanentRedirect

class HttpResponsePermanentRedirect¶
Like HttpResponseRedirect, but it returns a permanent redirect (HTTP status code 301) instead of a “found” redirect (status code 302).

cwdavies and others added 2 commits May 1, 2020 11:40
This change asserts that the URL gets redirected to lower-case from mixed case, but because the lowercase URL doesn't necessarily exist, it does not fetch the resulting response.
@willbarton
Copy link
Member

@cwdavies I figured out the right incantation and pushed a change to the tests to use assertRedirects. I think this is ready to go!

@cwdavies cwdavies merged commit d4db2dd into master May 1, 2020
@cwdavies cwdavies deleted the fix-deprecation-warnings branch May 1, 2020 16:37
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