◐ Shell
reader mode source ↗
Skip to content

bpo-36948: Fix NameError in urllib.request.URLopener.retrieve#13389

Merged
berkerpeksag merged 9 commits into
python:masterfrom
tirkarthi:bpo36948
May 19, 2019
Merged

bpo-36948: Fix NameError in urllib.request.URLopener.retrieve#13389
berkerpeksag merged 9 commits into
python:masterfrom
tirkarthi:bpo36948

Conversation

@tirkarthi

@tirkarthi tirkarthi commented May 17, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Fix NameError in urllib.request.URLopener.retrieve by using correct imports.

https://bugs.python.org/issue36948

isidentical
isidentical requested changes May 17, 2019
View reviewed changes

@isidentical isidentical left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Do this PR needs a news entry?

@tirkarthi

tirkarthi commented May 17, 2019

Copy link
Copy Markdown
Member Author

Do this PR needs a news entry?

@isidentical I have added one already at https://github.com/python/cpython/pull/13389/files#diff-1f45e92fbc7f2721e27165810e333552

@isidentical

isidentical commented May 17, 2019

Copy link
Copy Markdown
Member

I'm just asking, i dont think this is a change that requires a news entry.

@tirkarthi

tirkarthi commented May 17, 2019

Copy link
Copy Markdown
Member Author

I'm just asking, i dont think this is a change that requires a news entry.

This is a user impacting bug where even though the function was deprecated it's now causing NameError that used to work fine. IMO, it's a bug fix and requires a NEWS entry.

@bedevere-bot

bedevere-bot commented May 18, 2019

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@tirkarthi

tirkarthi commented May 19, 2019

Copy link
Copy Markdown
Member Author

I have resorted to using support.temp_dir to create a temporary directory for mkstemp . I guess the file being used error was to adding os.close(fd) to Cleanup where it's closed at the end of tests though the context manager support.temp_dir tries to clean it up on __exit__ causing error on cleaning up tmp_dir with an open file handle. So I have added os.close(fd) on the next line itself. I must admit that I resolved the issue with trial and error since I don't have access to Windows. Suggestions if any windows compatible test would be helpful too.

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented May 19, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

@berkerpeksag berkerpeksag merged commit c661b30 into python:master May 19, 2019
@miss-islington

miss-islington commented May 19, 2019

Copy link
Copy Markdown
Contributor

Thanks @tirkarthi for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2019
@tirkarthi @miss-islington
bpo-36948: Fix NameError in urllib.request.URLopener.retrieve (python… …
bec59f1 
…GH-13389)

(cherry picked from commit c661b30)

Co-authored-by: Xtreak <tir.karthi@gmail.com>
@bedevere-bot

bedevere-bot commented May 19, 2019

Copy link
Copy Markdown

GH-13422 is a backport of this pull request to the 3.7 branch.

@tirkarthi

tirkarthi commented May 19, 2019

Copy link
Copy Markdown
Member Author

Thanks @berkerpeksag for the review. This is a 3.8 only issue so I guess the backport PR to 3.7 can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

@isidentical isidentical isidentical approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tirkarthi @isidentical @bedevere-bot @miss-islington @berkerpeksag @the-knights-who-say-ni