Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#14675 closed Cleanup/optimization (fixed)

Update docs and project template to avoid "import *" for url patterns

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

Description

Now that we have http://code.djangoproject.com/changeset/13590, we should probably avoid the import * for urls in our docs and project template.

Attachments (11)

template.diff (415 bytes) - added by pupeno 3 years ago.
Explicitly included functions from urls.default for the project template.
django-itself.diff (746 bytes) - added by pupeno 3 years ago.
Explicitly included functions from urls.default in django itself.
contrib-apps.diff (4.6 KB) - added by pupeno 3 years ago.
Explicitly included functions from urls.default in contrib apps.
docs.diff (10.2 KB) - added by pupeno 3 years ago.
Explicitly included functions from urls.default in documentation.
contrib-apps-docs.diff (4.7 KB) - added by pupeno 3 years ago.
Explicitly included functions from urls.default in documentation for contrib apps.
tests.diff (13.7 KB) - added by pupeno 3 years ago.
Explicitly included functions from urls.default in tests.
14675-remove-import-star-from-urlconfs.patch (34.4 KB) - added by cdestigter 3 years ago.
all-in-one patch, & fixed typo in regressiontests/urlpatterns_reverse/included_named_urls2.py
14675-remove-import-star-from-urlconfs-2.patch (40.3 KB) - added by cdestigter 3 years ago.
all-in-one patch, with tweaked docs and made from django.conf.urls import patterns work (& the standard way to import url functions)
14675-remove-import-star-from-urlconfs-3.patch (38.1 KB) - added by dmclain 3 years ago.
14675-remove-import-star-from-urlconfs-4.patch (48.3 KB) - added by ramiro 3 years ago.
Patch updated to apply cleanly as of now
14675-remove-import-star-from-urlconfs-5.patch (48.4 KB) - added by ramiro 3 years ago.
Updated patch

Download all attachments as: .zip

Change History (26)

comment:1 Changed 3 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by gabrielhurley

I've often been bothered by the from django.conf.urls.defaults import * pattern myself, since we so often encourage people not to use import *. While technically that module does define __all__, I'd still rather see patterns, url, include, et. al. imported explicitly anyway. So I'm all for this change.

Changed 3 years ago by pupeno

Explicitly included functions from urls.default for the project template.

Changed 3 years ago by pupeno

Explicitly included functions from urls.default in django itself.

Changed 3 years ago by pupeno

Explicitly included functions from urls.default in contrib apps.

Changed 3 years ago by pupeno

Explicitly included functions from urls.default in documentation.

Changed 3 years ago by pupeno

Explicitly included functions from urls.default in documentation for contrib apps.

Changed 3 years ago by pupeno

Explicitly included functions from urls.default in tests.

comment:3 Changed 3 years ago by pupeno

Hello,

I wanted to make my first contribution to Django and this looked simple enough, I hope I did it correctly. My goal with this set of patches was to totally eradicate the import of * from urls.default. I've separated it in various parts so it's easy to pick what you consider is the correct set of changes starting with the small change to the template for new projects up to modifying every contrib app and test to follow this best practices.

comment:4 Changed 3 years ago by pupeno

Oh, something I didn't modify is the documentation for Django < 1.3, as I presume that should be left alone as it is now.

comment:5 Changed 3 years ago by Keryn Knight <keryn@…>

I'm totally in favour of deprecating the star import, as it's something that's bugged me previously.

After a cursory glance of the changeset and the diffs pupeno has provided, I'd like to suggest that the documentation diff may need further work, to clarify the change as it affects handler404 & handler500.

Mostly, That takes care of setting ``handler404`` in the current module. [...] occurs in a couple of places, and whilst still semi-accurate now that we fallback instead of grabbing via a star import, it feels like further explanation might be expounded. However, as the changeset doesn't seem to have any doc changes, I'm not sure if this is already covered elsewhere.

comment:6 Changed 3 years ago by gabrielhurley

  • Component changed from Documentation to Core framework
  • Has patch set
  • Patch needs improvement set

Agreed with Keryn Knight that a few words in the docs would be helpful. However, this change is much more relevant to the rest of the codebase than it is to the docs, so I'm changing the component.

I have not tested this patch. Someone needs to apply it against trunk and verify that it passes the full test suite as well.

Changed 3 years ago by cdestigter

all-in-one patch, & fixed typo in regressiontests/urlpatterns_reverse/included_named_urls2.py

Changed 3 years ago by cdestigter

all-in-one patch, with tweaked docs and made from django.conf.urls import patterns work (& the standard way to import url functions)

comment:7 Changed 3 years ago by cdestigter

It struck me that we're making things awfully verbose by getting rid of the 'import *'...

So to compensate, I removed the '.defaults', which has always bugged me.

It's just easier to remember than 'from django.conf.urls.defaults import patterns'. The 'defaults' should be implied.

This shouldn't break anything, the code is still in defaults.py.

comment:8 Changed 3 years ago by cdestigter

  • Patch needs improvement unset

comment:9 Changed 3 years ago by ramiro

This has been partially fixed in [15401].

comment:10 Changed 3 years ago by carljm

Based on discussion in IRC with Alex and Russell, adding the ability to import from just "django.conf.urls" is good, but a feature not a bugfix, so not a candidate for 1.3 at this point. And we don't want to do two passes through all the contrib apps etc, so we're just going to hold off on the rest of this patch until after 1.3 is released.

comment:11 Changed 3 years ago by carljm

In [15415]:

Refs #14675 - Added import of url() function in project template urls.py, added commented sample named url, and converted existing sample patterns to use url().

comment:12 Changed 3 years ago by julien

  • milestone changed from 1.3 to 1.4

Changing the milestone as per Carl's comment above.

comment:13 Changed 3 years ago by jaddison

  • milestone 1.4 deleted
  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:14 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

14675-remove-import-star-from-urlconfs-2.patch fails to apply cleanly on to trunk

Changed 3 years ago by ramiro

Patch updated to apply cleanly as of now

Changed 3 years ago by ramiro

Updated patch

comment:15 Changed 3 years ago by ramiro

  • Resolution set to fixed
  • Status changed from new to closed

In [16818]:

Fixed #14675 -- Completed removal of from django.conf.urls.default import * usage.

This applies to both our own [test] code and documentation examples. Also:

  • Moved the functions and handlers from django.conf.urls.defaults up to django.conf.urls deprecating the former module.
  • Added documentation for handler403.
  • Tweaked the URLs topic document a bit.

Thanks to pupeno and cdestigter for their great work contributing patches.

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.