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

chore: Resolve RemovedInDjango40Warning warnings. #144

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

christopappas
Copy link
Contributor

@christopappas christopappas commented Nov 1, 2021

Description:

Use django.urls instead of django.conf.urls to resolve REmovedInDjango40Warnings

JIRA:

No associated ticket.

Additional Details

  • Dependencies:: List dependencies on other outstanding PRs, issues, etc.
  • Merge deadline: List merge deadline (if any)
  • Testing instructions: Provide non-trivial testing instructions
  • Author concerns: List any concerns about this PR

Reviewers:

  • @edx/arch-review
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bump if needed
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@christopappas christopappas force-pushed the cpappas/tech-debt-athon branch from 877f7c1 to 892316d Compare November 1, 2021 16:14
Copy link

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

If you find yourself needing to do more things like this, there's a codemod utility at https://github.com/adamchainz/django-upgrade that can do most of it for you (Arbi-BOM is currently running it across edx-platform).

@@ -2,15 +2,16 @@

Add these to your project's `urlpatterns` to avoid duplicating code.
"""
from django.conf.urls import url, include
from django.conf.urls import include
from django.urls import re_path
Copy link

Choose a reason for hiding this comment

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

include is also now available via django.urls, so this whole thing can be simplified to from django.urls import include, re_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, will change

url('', include('social_django.urls', namespace='social')),
re_path(r'^login/$', EdxOAuth2LoginView.as_view(), name='login'),
re_path(r'^logout/$', EdxOAuth2LogoutView.as_view(), name='logout'),
re_path('', include('social_django.urls', namespace='social')),
Copy link

Choose a reason for hiding this comment

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

We'll probably want to change these to use path() later, but I'm fine with punting that until we have more experience and docs for doing it.

@christopappas christopappas force-pushed the cpappas/tech-debt-athon branch from 892316d to 68f175b Compare November 1, 2021 17:25
@christopappas christopappas force-pushed the cpappas/tech-debt-athon branch from 68f175b to b356c31 Compare November 1, 2021 17:26
@christopappas christopappas merged commit 94093bd into master Nov 1, 2021
@christopappas christopappas deleted the cpappas/tech-debt-athon branch November 1, 2021 17:32
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.

4 participants