Opened 5 years ago

Closed 3 years ago

Last modified 21 months ago

#17627 closed Cleanup/optimization (fixed)

contrib.admin.util is misnamed

Reported by: Paul McMillan Owned by: Luke Granger-Brown
Component: contrib.admin Version: master
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 5 years ago.
Patch + deprecation warning + backwards compatibility import
17627.clean.diff (23.1 KB) - added by Ethan Jucovy 5 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 5 years ago.
Updated deprecation timeline to 1.7
0002.patch (120.5 KB) - added by Luke Granger-Brown 5 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 5 years ago.
Updated version of 0002.patch

Download all attachments as: .zip

Change History (29)

comment:1 Changed 5 years ago by Claude Paroz

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 Changed 5 years ago by Wiktor

Owner: changed from nobody to Wiktor
Status: newassigned

comment:3 Changed 5 years ago by Wiktor

Cc: Wiktor added
Has patch: set

Changed 5 years ago by Wiktor

Patch + deprecation warning + backwards compatibility import

comment:4 Changed 5 years ago by Tomek Paczkowski

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 Changed 5 years ago by Claude Paroz

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 Changed 5 years ago by Luke Granger-Brown

Cc: django@… added
Patch needs improvement: unset

Adding self to cc and unsetting patch needs improvement.

comment:7 Changed 5 years ago by Luke Granger-Brown

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

Assigning to self after 3 weeks.

comment:8 Changed 5 years ago by Luke Granger-Brown

Status: newassigned

Changed 5 years ago by Ethan Jucovy

Attachment: 17627.clean.diff added

remove email gunk from patch, and fix deprecation schedule

comment:9 Changed 5 years ago by Ethan Jucovy

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 Changed 5 years ago by Luke Granger-Brown

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

comment:11 in reply to:  10 Changed 5 years ago by Ethan Jucovy

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 Changed 5 years ago by Luke Granger-Brown

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 Changed 5 years ago by Carl Meyer

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 :-)

Changed 5 years ago by Luke Granger-Brown

Updated deprecation timeline to 1.7

comment:14 Changed 5 years ago by Luke Granger-Brown

Okay, deprecation timeline updated for 1.7 (-:

comment:15 Changed 5 years ago by Russell Keith-Magee

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.

comment:16 in reply to:  15 Changed 5 years ago by Luke Granger-Brown

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)

Changed 5 years ago by Luke Granger-Brown

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 Changed 5 years ago by Carl Meyer

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.

Changed 5 years ago by Luke Granger-Brown

Attachment: 0003.patch added

Updated version of 0002.patch

comment:18 Changed 5 years ago by Luke Granger-Brown

Patch needs improvement: unset

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

comment:19 Changed 3 years ago by Alexander Hansen

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 Changed 3 years ago by Tim Graham <timograham@…>

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 Changed 3 years ago by Tim Graham <timograham@…>

In f40c82213f28109be8eb3ee516bc4a13461352c8:

Added backwards compatability shims for util modules.

refs #17627

comment:22 Changed 21 months ago by Tim Graham <timograham@…>

In 8a9b0c15a6c0ef60dea3ba3042317520bc201206:

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

comment:23 Changed 21 months ago by Tim Graham <timograham@…>

In 4abfa73c1861c53d43f0448726346866b04b9b72:

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

Backport of 8a9b0c15a6c0ef60dea3ba3042317520bc201206 from master

comment:24 Changed 21 months ago by Tim Graham <timograham@…>

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