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 , 9 years ago
Has patch: | set |
---|
comment:2 by , 9 years ago
Cc: | 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 , 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 , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:7 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:9 by , 9 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I agree, let's drop our version and import six'. As Carl said on the PR a comment explaining the import is needed.
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.