Opened 9 years ago

Closed 9 years ago

#24927 closed Cleanup/optimization (fixed)

django.utils.six and django.utils.encoding both define python_2_unicode_compatible

Reported by: Marten Kenbeek Owned by: Markus Holtermann
Component: Utilities Version: dev
Severity: Normal Keywords:
Cc: me@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

django.utils.six and django.utils.encoding define the exact same method definition of python_2_unicode_compatible, which is not very DRY. It might be a good idea to replace one of them with a convenience import of the other.

Change History (10)

comment:1 by Matthew Somerville, 9 years ago

Has patch: set

The function was added to six in 1.9.0, copied from Django's code. PR at https://github.com/django/django/pull/4784 to have it include one from the other, I was unsure whether it was better to maintain the function in encoding.

comment:2 by Andriy Sokolovskiy, 9 years ago

Cc: me@… added
Easy pickings: unset

If we're going to use the method from six, seems we should to deprecate import from django utils, and replace imports inside the django code to use six import instead of utils.

comment:3 by Carl Meyer, 9 years ago

I don't think it's worth putting users through a deprecation process on the import location of python_2_unicode_compatible, when we could just provide a non-deprecated import shim and leave it there until Python 2 support is dropped entirely. That's a lot of hassle for Django users in order to save us a single import line in django.utils.encoding.

I do think it's good to switch to relying on the implementation in six.

comment:4 by Markus Holtermann, 9 years ago

Owner: changed from nobody to Markus Holtermann
Status: newassigned

comment:5 by Markus Holtermann, 9 years ago

Triage Stage: UnreviewedAccepted

comment:6 by Carl Meyer, 9 years ago

@markush there was already a PR contributed, see above

comment:7 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Markus Holtermann, 9 years ago

Oh, missed that. Thanks Carl.

comment:9 by Aymeric Augustin, 9 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I agree, let's drop our version and import six'. As Carl said on the PR a comment explaining the import is needed.

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

Resolution: fixed
Status: assignedclosed

In c91bc68:

Fixed #24927 -- Used python_2_unicode_compatible from six

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