Opened 12 years ago

Closed 11 years ago

Last modified 9 years ago

#17627 closed Cleanup/optimization (fixed)

contrib.admin.util is misnamed

Reported by: Paul McMillan Owned by: Luke Granger-Brown
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Wiktor, django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

r17145 included a change which added contrib.admin.util. This module should be contrib.admin.utils in line with similar existing modules throughout the rest of the codebase.

https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/util.py

Attachments (5)

0001-17627-renaming-contrib.admin.util-contrib.admin.util.patch (60.4 KB ) - added by Wiktor 12 years ago.
Patch + deprecation warning + backwards compatibility import
17627.clean.diff (23.1 KB ) - added by Ethan Jucovy 12 years ago.
remove email gunk from patch, and fix deprecation schedule
0001-Fixing-17627-renaming-contrib.admin.util-contrib.adm.patch (60.1 KB ) - added by Luke Granger-Brown 12 years ago.
Updated deprecation timeline to 1.7
0002.patch (120.5 KB ) - added by Luke Granger-Brown 12 years ago.
Same as original patch, rebased against master and also including all util.py -> utils.py changes apart from utils/unittests/util.py
0003.patch (127.9 KB ) - added by Luke Granger-Brown 12 years ago.
Updated version of 0002.patch

Download all attachments as: .zip

Change History (29)

comment:1 by Claude Paroz, 12 years ago

Severity: Release blockerNormal
Type: UncategorizedCleanup/optimization

Yes, it is known that some unification is needed regarding util.py vs utils.py files. However this has not been introduced in r17145, but in r7967 4 years ago. Hence removing the release blocker flag.

comment:2 by Wiktor, 12 years ago

Owner: changed from nobody to Wiktor
Status: newassigned

comment:3 by Wiktor, 12 years ago

Cc: Wiktor added
Has patch: set

by Wiktor, 12 years ago

Patch + deprecation warning + backwards compatibility import

comment:4 by Tomek Paczkowski, 12 years ago

Triage Stage: AcceptedReady for checkin

After applying patch one has to remove pyc files and possibly remove the tests/regressiontests/admin_util directory, that's now empty.

Other than that looks good.

comment:5 by Claude Paroz, 12 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

The general Django deprecation policy is PendingDeprecationWarning -> DeprecationWarning -> removal.
https://docs.djangoproject.com/en/dev/internals/release-process/#internal-release-deprecation-policy

So unless a core committer has admitted a shortened deprecation path, you should respect this. Moreover, when introducing a deprecation, please add in your patch the appropriate lines to the deprecation timeline.

comment:6 by Luke Granger-Brown, 12 years ago

Cc: django@… added
Patch needs improvement: unset

Adding self to cc and unsetting patch needs improvement.

comment:7 by Luke Granger-Brown, 12 years ago

Owner: changed from Wiktor to Luke Granger-Brown
Status: assignednew

Assigning to self after 3 weeks.

comment:8 by Luke Granger-Brown, 12 years ago

Status: newassigned

by Ethan Jucovy, 12 years ago

Attachment: 17627.clean.diff added

remove email gunk from patch, and fix deprecation schedule

comment:9 by Ethan Jucovy, 12 years ago

The latest patch from lukegb didn't apply cleanly -- there was some email-looking gunk at the top of the file. I cleaned it up and regenerated it with svn di against trunk@17610. The patch now applies cleanly.

I also made one minor edit to the patch contents. The previous patch to docs/internals/deprecation.txt announced that the old admin.util import would be removed in Django version 2.0. Per the deprecation policy I believe the old import may be removed two minor versions after the PendingDeprecationWarning is established, i.e. in version 1.{N+2}. On the assumption that this patch will be merged before 1.4 final, my patch now announces that the old import will be removed in 1.6. Of course this will need to be updated again if the patch doesn't make it in for 1.4.

comment:10 by Luke Granger-Brown, 12 years ago

Oops, apologies. That patch was generated using git format-patch and I neglected to remove the header that it adds.

in reply to:  10 comment:11 by Ethan Jucovy, 12 years ago

Replying to lukegb:

Oops, apologies. That patch was generated using git format-patch and I neglected to remove the header that it adds.

Urg, I just realized my patch is broken too. I generated it from svn di which doesn't seem to have printed out diffs for the newly added files / rename dests. So my patch causes admin/utils.py and its regressiontests to be missing.

comment:12 by Luke Granger-Brown, 12 years ago

Cleaned up patch.

I assumed that this patch wouldn't make it in to 1.4, because the first of the 1.4 betas have already come out. I couldn't find anything on 1.7(?) so I assumed it was going for the next version after that.

comment:13 by Carl Meyer, 12 years ago

lukegb is correct that this patch won't go into 1.4; only thing that gets in after the beta is bugfixes, and this doesn't fix a bug.

There will almost certainly be a 1.7, so that would be the correct release to add the deprecation timeline note to. There isn't a section for it yet because obviously no 1.5-targetted patches have gone into trunk yet :-)

by Luke Granger-Brown, 12 years ago

Updated deprecation timeline to 1.7

comment:14 by Luke Granger-Brown, 12 years ago

Okay, deprecation timeline updated for 1.7 (-:

comment:15 by Russell Keith-Magee, 12 years ago

On a related note -- if you want to make this a really sweet target for commit, you could also address the other instances of util.py in the source tree:

$ find . -name "util.py"
./contrib/admin/util.py
./contrib/gis/db/backends/util.py
./contrib/localflavor/it/util.py
./contrib/localflavor/uy/util.py
./db/backends/util.py
./forms/util.py
./utils/unittest/util.py

With the exception of "unittest/util.py" (which is so named because it's a copy from an upstream source), these are all candidates for a util->utils conversion.

in reply to:  15 comment:16 by Luke Granger-Brown, 12 years ago

Replying to russellm:

On a related note -- if you want to make this a really sweet target for commit, you could also address the other instances of util.py in the source tree.

Done in the latest version (0002.patch) of my patch.

Side notes which break backwards compatibility:
Code which does

from django.db import utils 
from django.db.backends import * 

will break with this change, as django.db.backends.utils will overwrite django.db.utils. Not sure how to fix this, but I fixed it in the instances where Django does it:

  • django/db/backends/mysql/base.py
  • django/db/backends/oracle/base.py
  • django/db/backends/postgresql_psycopg2/base.py
  • django/db/backends/sqlite3/base.py

(just realised I forgot to update the deprecation notes - new patch incoming)

by Luke Granger-Brown, 12 years ago

Attachment: 0002.patch added

Same as original patch, rebased against master and also including all util.py -> utils.py changes apart from utils/unittests/util.py

comment:17 by Carl Meyer, 12 years ago

Patch needs improvement: set

Thanks Luke. I'm not happy with the current solution in the patch for the import * problem, it's kind of ugly. When instances of import * cause trouble, that's a golden opportunity to get rid of that instance of import * entirely.

Also, I note an oddity in the patch in deprecation.txt - it's listed under a section for 1.7 but mentions 1.8 near the end of the paragraph. Normally we try not to be redundant in the text and just say "will be removed." not "will be removed in 1.X", since the relevant version is already at the header of the section.

by Luke Granger-Brown, 12 years ago

Attachment: 0003.patch added

Updated version of 0002.patch

comment:18 by Luke Granger-Brown, 12 years ago

Patch needs improvement: unset

Uploaded 0003.patch - replaced import * with a more specific import, and fixed deprecation.txt.

comment:19 by Alexander Hansen, 11 years ago

Patch needs improvement: set

This patch is failing all over the place on latest trunk.
For instance the patch tries to patch some files in django.contrib.localflavors. Local flavors is moved out to separate sub-projects for easier maintenance (The “local flavor” add-ons).

comment:20 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 18ffdb1772ba60e085cff8fd9a1d4a7b129b4032:

Fixed #17627 -- Renamed util.py files to utils.py

Thanks PaulM for the suggestion and Luke Granger-Brown and
Wiktor Kołodziej for the initial patch.

comment:21 by Tim Graham <timograham@…>, 11 years ago

In f40c82213f28109be8eb3ee516bc4a13461352c8:

Added backwards compatability shims for util modules.

refs #17627

comment:22 by Tim Graham <timograham@…>, 9 years ago

In 8a9b0c15a6c0ef60dea3ba3042317520bc201206:

Renamed tests for util -> utils moves; refs #17627.

comment:23 by Tim Graham <timograham@…>, 9 years ago

In 4abfa73c1861c53d43f0448726346866b04b9b72:

[1.7.x] Renamed tests for util -> utils moves; refs #17627.

Backport of 8a9b0c15a6c0ef60dea3ba3042317520bc201206 from master

comment:24 by Tim Graham <timograham@…>, 9 years ago

In 9ce36512fa925231b0496cc10f7d114c069c7c06:

Removed backwards compatibility shims for "util" modules per deprecation timeline.

refs #17627.

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