Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 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@… 3 years ago.
patch which removes import * magic from urls

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by anonymous

  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 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 3 years ago by wim@…

patch which removes import * magic from urls

comment:3 Changed 3 years ago by draix

  • Component changed from Uncategorized to Core (Other)
  • Owner changed from nobody to draix
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by baumer1122

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

comment:5 Changed 3 years ago by aaugustin

  • Patch needs improvement set

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

comment:6 Changed 3 years ago by baumer1122

  • 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 follow-up: Changed 3 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 3 years ago by taavi223

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 3 years ago by julien

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 3 years ago by Wim Feijen <wim@…>

  • Resolution set to duplicate
  • Status changed from assigned to closed

Thanks Julien.

draix, baumer1122, thanks for your work.

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

comment:11 Changed 3 years ago by ramiro

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 3 years ago by draix

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

Thanks everyone.

Regards,
Martin

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.