Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16788 closed Cleanup/optimization (duplicate)

remove import * from urls.py

Reported by: wim@… Owned by: draix
Component: Core (Other) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Let's face it: import * is ugly and breaks guidelines on what is good practice in Python code. (And pylint complains about it. :) ) Let's send them into oblivion!

import patterns
or
import patterns, urls
will do in most cases :)

contrib/admindocs/urls.py:from django.conf.urls.defaults import *
contrib/auth/urls.py:from django.conf.urls.defaults import *
contrib/comments/urls.py:from django.conf.urls.defaults import *
contrib/databrowse/urls.py:from django.conf.urls.defaults import *
contrib/flatpages/urls.py:from django.conf.urls.defaults import *
contrib/staticfiles/urls.py:from django.conf import settings
contrib/staticfiles/urls.py:from django.conf.urls.static import static

contrib/auth/tests/urls.py:from django.conf.urls.defaults import patterns, url
contrib/flatpages/tests/urls.py:from django.conf.urls.defaults import *
contrib/formtools/tests/urls.py:from django.conf.urls.defaults import *
contrib/messages/tests/urls.py:from django.conf.urls.defaults import *
contrib/sitemaps/tests/urls.py:from django.conf.urls.defaults import *

Attachments (1)

patch_ticket_16788.diff (4.0 KB) - added by wim@… 5 years ago.
patch which removes import * magic from urls

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by anonymous

Easy pickings: set

comment:2 Changed 5 years ago by wim@…

Sorry! Now with code blocks :)

Let's face it: import * is ugly and breaks guidelines on what is good practice in Python code. (And pylint complains about it. :) ) Let's send them into oblivion!

import patterns

or

import patterns, urls


will do in most cases :)

contrib/admindocs/urls.py:from django.conf.urls.defaults import *
contrib/auth/urls.py:from django.conf.urls.defaults import *
contrib/comments/urls.py:from django.conf.urls.defaults import *
contrib/databrowse/urls.py:from django.conf.urls.defaults import *
contrib/flatpages/urls.py:from django.conf.urls.defaults import *
contrib/staticfiles/urls.py:from django.conf import settings
contrib/staticfiles/urls.py:from django.conf.urls.static import static

contrib/auth/tests/urls.py:from django.conf.urls.defaults import patterns, url
contrib/flatpages/tests/urls.py:from django.conf.urls.defaults import *
contrib/formtools/tests/urls.py:from django.conf.urls.defaults import *
contrib/messages/tests/urls.py:from django.conf.urls.defaults import *
contrib/sitemaps/tests/urls.py:from django.conf.urls.defaults import *

Changed 5 years ago by wim@…

Attachment: patch_ticket_16788.diff added

patch which removes import * magic from urls

comment:3 Changed 5 years ago by draix

Component: UncategorizedCore (Other)
Owner: changed from nobody to draix
Status: newassigned
Triage Stage: UnreviewedAccepted

That's correct, I'm adding some tests also to your observation:

$ grep -R "import \*" * |grep -v ".svn" |grep "/urls.py"
django/contrib/admindocs/urls.py:from django.conf.urls.defaults import *
django/contrib/auth/urls.py:from django.conf.urls.defaults import *
django/contrib/comments/urls.py:from django.conf.urls.defaults import *
django/contrib/databrowse/urls.py:from django.conf.urls.defaults import *
django/contrib/flatpages/tests/urls.py:from django.conf.urls.defaults import *
django/contrib/flatpages/urls.py:from django.conf.urls.defaults import *
django/contrib/formtools/tests/urls.py:from django.conf.urls.defaults import *
django/contrib/formtools/tests/wizard/wizardtests/urls.py:from django.conf.urls.defaults import *
django/contrib/gis/tests/geoapp/urls.py:from django.conf.urls.defaults import *
django/contrib/messages/tests/urls.py:from django.conf.urls.defaults import *
django/contrib/sitemaps/tests/urls.py:from django.conf.urls.defaults import *
tests/modeltests/test_client/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/admin_views/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/admin_widgets/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/comment_tests/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/conditional_processing/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/context_processors/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/file_uploads/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/generic_inline_admin/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/generic_views/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/middleware_exceptions/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/model_permalink/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/special_headers/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/syndication/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/templates/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/test_client_regress/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/urlpatterns_reverse/urls.py:from django.conf.urls.defaults import *
tests/regressiontests/views/urls.py:from django.conf.urls.defaults import *
tests/urls.py:from django.conf.urls.defaults import *

Well, actually we have lots of work to do regarded to this topic on the whole Django code base.

Doing some research on the trunk codebase:

$ grep -R "import \*" * |grep -v ".svn" |wc -l
     251

I'll start working on removing "import *" 's form urls.py, but maybe we also should also move to other areas of the code on this topic.

comment:4 Changed 5 years ago by Peter Baumgartner

doh! I just finished this draix: https://github.com/django/django/pull/41

comment:5 Changed 5 years ago by Aymeric Augustin

Patch needs improvement: set

For consistency, this should be fixed in the tests and docs too.

comment:6 Changed 5 years ago by Peter Baumgartner

Has patch: set
Patch needs improvement: unset

New commit added to the pull request that covers docs and tests, https://github.com/django/django/pull/41/files

comment:7 Changed 5 years ago by wim@…

Hi Peter,

Thanks for the patch! I want to run it and the tests to mark it as Ready for Checkin, however, unfortunately, I am unable to apply your patch, because it is not a svn diff. Could you make it into a svn diff?

As far as I know, patches are supplied here as svn diffs at the ticket. Did this policy change and/or did I miss something?

Wim

comment:8 in reply to:  7 Changed 5 years ago by Taavi Taijala

Replying to wim@…:

Hi Peter,

Thanks for the patch! I want to run it and the tests to mark it as Ready for Checkin, however, unfortunately, I am unable to apply your patch, because it is not a svn diff. Could you make it into a svn diff?

As far as I know, patches are supplied here as svn diffs at the ticket. Did this policy change and/or did I miss something?

Wim

The docs do say that git diff files are acceptable and that the project accepts pull requests. Of course not everyone is setup for those.

comment:9 Changed 5 years ago by Julien Phalip

Hmmm, this is probably a duplicate of #14675. A pretty extensive patch was posted there a few days ago too. Can you check it to avoid duplication of efforts? Thanks!

comment:10 Changed 5 years ago by Wim Feijen <wim@…>

Resolution: duplicate
Status: assignedclosed

Thanks Julien.

draix, baumer1122, thanks for your work.

This ticket is indeed a duplicate of #14675. I'm sorry.

comment:11 Changed 5 years ago by Ramiro Morales

Adding this comment just to say "Thank you" to all the contributors to this ticket for their work. Unfortunately we all didn't realize there was another ticket with similar work already open and spare you the duplicate effort. The good news is that we've just fixed #14675 :) Thanks again!

comment:12 Changed 5 years ago by draix

Oka, cool. No problem. Good news is it's fixed!

Thanks everyone.

Regards,
Martin

Note: See TracTickets for help on using tickets.
Back to Top