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

Compat cleanup #5581

Merged
merged 5 commits into from
Nov 10, 2017
Merged

Compat cleanup #5581

merged 5 commits into from
Nov 10, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Nov 9, 2017

  • flake8 was globally disabled on the compat module, which is why whitespace issues went undetected by the linter. Removed the global noqa and re-added where necessary to individual lines.
  • simplified and removed unnecessary imports (eg, unnecessary urls imports, old templates imports).
  • RegexURLPattern => URLPattern
  • RegexURLResolver => URLResolver

@@ -254,7 +249,7 @@ def md_filter_add_syntax_highlight(md):
return False

try:
import pytz
import pytz # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting with Django 1.11, pytz is a Django requirement. So django-rest-framework can expect it to exist. I think adding a comment with this information would be helpful so that when Django 1.10 is dropped it is noticed and this workaround is removed.

... when dropping Django 1.10
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

👍

Grrr. GitHub web editor 😡
@carltongibson carltongibson merged commit 8d7ce37 into encode:master Nov 10, 2017
@carltongibson carltongibson added this to the v3.7.4 milestone Nov 10, 2017
@xordoquy
Copy link
Collaborator

Was there really an urgency to remove pytz ?
Did we already removed support for Django 1.10 ?

@carltongibson
Copy link
Collaborator

Just added a note so that we remember when the time comes. pytz will become a hard requirement, so we’ll be able to drop the compat shim.

@@ -253,8 +248,9 @@ def md_filter_add_syntax_highlight(md):
def md_filter_add_syntax_highlight(md):
return False

# pytz is required from Django 1.11. Remove when dropping Django 1.10 support.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@xordoquy it’s just this comment. Nothing is actually changed yet.

@xordoquy
Copy link
Collaborator

oh, perfect !

@rpkilby rpkilby deleted the compat-cleanup branch November 11, 2017 05:21
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Reenable flake8 on compat, cleanup style/imports

* Cleanup compat urls imports

* Refactor compat url pattern/resolver imports

* Add comment re dropping pytz compat

... when dropping Django 1.10

* Strip whitespace

Grrr. GitHub web editor 😡
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.

None yet

4 participants