Code

Opened 2 years ago

Closed 10 months ago

Last modified 10 months ago

#17627 closed Cleanup/optimization (fixed)

contrib.admin.util is misnamed

Reported by: PaulM Owned by: lukegb
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: viciu, 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 viciu 2 years ago.
Patch + deprecation warning + backwards compatibility import
17627.clean.diff (23.1 KB) - added by ejucovy 2 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 lukegb 2 years ago.
Updated deprecation timeline to 1.7
0002.patch (120.5 KB) - added by lukegb 2 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 lukegb 2 years ago.
Updated version of 0002.patch

Download all attachments as: .zip

Change History (26)

comment:1 Changed 2 years ago by claudep

  • Severity changed from Release blocker to Normal
  • Type changed from Uncategorized to Cleanup/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 2 years ago by viciu

  • Owner changed from nobody to viciu
  • Status changed from new to assigned

comment:3 Changed 2 years ago by viciu

  • Cc viciu added
  • Has patch set

Changed 2 years ago by viciu

Patch + deprecation warning + backwards compatibility import

comment:4 Changed 2 years ago by oinopion

  • Triage Stage changed from Accepted to Ready 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 2 years ago by claudep

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 2 years ago by lukegb

  • Cc django@… added
  • Patch needs improvement unset

Adding self to cc and unsetting patch needs improvement.

comment:7 Changed 2 years ago by lukegb

  • Owner changed from viciu to lukegb
  • Status changed from assigned to new

Assigning to self after 3 weeks.

comment:8 Changed 2 years ago by lukegb

  • Status changed from new to assigned

Changed 2 years ago by ejucovy

remove email gunk from patch, and fix deprecation schedule

comment:9 Changed 2 years ago by ejucovy

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 follow-up: Changed 2 years ago by lukegb

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 2 years ago by ejucovy

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 2 years ago by lukegb

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 2 years ago by carljm

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 2 years ago by lukegb

Updated deprecation timeline to 1.7

comment:14 Changed 2 years ago by lukegb

Okay, deprecation timeline updated for 1.7 (-:

comment:15 follow-up: Changed 2 years ago by 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:

$ 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 2 years ago by lukegb

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 2 years ago by lukegb

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 2 years ago by carljm

  • 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 2 years ago by lukegb

Updated version of 0002.patch

comment:18 Changed 2 years ago by lukegb

  • Patch needs improvement unset

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

comment:19 Changed 14 months ago by wckd

  • 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 10 months ago by Tim Graham <timograham@…>

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

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 10 months ago by Tim Graham <timograham@…>

In f40c82213f28109be8eb3ee516bc4a13461352c8:

Added backwards compatability shims for util modules.

refs #17627

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.