Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compat: (py2) urlparse = urllib.parse (py3) #6262

Merged
merged 2 commits into from Jan 8, 2019

Conversation

stephenfin
Copy link
Contributor

We were mistakenly importing the 'urlparse' function from the Python 2
'urlparse' module, as opposed to the module itself. Correct this.

Signed-off-by: Stephen Finucane stephen@that.guru
Closes: #6261

We were mistakenly importing the 'urlparse' function from the Python 2
'urlparse' module, as opposed to the module itself. Correct this.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: encode#6261
@xordoquy
Copy link
Collaborator

xordoquy commented Oct 20, 2018

now this has been brought into the spotlight, I'd rather we keep a consistent style and import:

from django.utils.six.moves.urllib import parse as urlparse

in the renderers.

edit:
or change the other imports.

We can just use Django's vendored six library, like we do everywhere
else.

Signed-off-by: Stephen Finucane <stephen@that.guru>
@stephenfin
Copy link
Contributor Author

now this has been brought into the spotlight, I'd rather we keep a consistent style and import:

from django.utils.six.moves.urllib import parse as urlparse

in the renderers.

edit:
or change the other imports.

Good call. Done.

@carltongibson carltongibson added this to the 3.9.1 Release milestone Oct 24, 2018
@carltongibson
Copy link
Collaborator

OK, given #6230 this'll be v3.9.1 or not at all.

@carltongibson
Copy link
Collaborator

I'm sorry to say that at this late point in history I'm so rusty on Python 2 compat that I can't remember the back story here.

No doubt, using six is the right way to go, but what's wrong the the code in compat, and where haven't we hit an error before?

Thanks.

@stephenfin
Copy link
Contributor Author

stephenfin commented Nov 5, 2018

That code we're removing is only used in one place and was added as part of the new command. It's been broken from the get-go and should never have been added, given that six provides a perfectly good wrapper. @xordoquy is therefore correct in suggesting we simply replace the broken code with a well-tested wrapper we already use mostly everywhere. Make sense?

@carltongibson
Copy link
Collaborator

...was added as part of the new command.

Right. OK. Fine. Thanks. 😊

casimir added a commit to critizr/django-rest-framework that referenced this pull request Nov 19, 2018
@johnthagen
Copy link
Contributor

Not sure on timelines, but there is also the ongoing work to simply drop Python 2 (#6230, #6277) at which point all of the six code shouldn't be necessary.

@tomchristie
Copy link
Member

Fair enough. Thanks!

@tomchristie tomchristie merged commit c052a86 into encode:master Jan 8, 2019
@stephenfin stephenfin deleted the issue/6261 branch January 8, 2019 23:03
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* compat: (py2) urlparse = urllib.parse (py3)

We were mistakenly importing the 'urlparse' function from the Python 2
'urlparse' module, as opposed to the module itself. Correct this.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: encode#6261

* compat: Remove 'compat.urlparse'

We can just use Django's vendored six library, like we do everywhere
else.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants