Opened 10 years ago
Closed 10 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 , 10 years ago
| Has patch: | set |
|---|
comment:2 by , 10 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 , 10 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 , 10 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:5 by , 10 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:7 by , 10 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
comment:9 by , 10 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.