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.

Sign up for GitHub

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

Jump to bottom

Use better temporary files mechanism #7929

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Use better temporary files mechanism #7929

merged 1 commit into from
Apr 1, 2020

Conversation

McSinyx
Copy link
Contributor

@McSinyx McSinyx commented Mar 29, 2020 •
edited
Loading

One thing I don't understand though, is why the output CSV file need to be opened as w+ instead of just w.

This should be able to fix GH-7699.

@McSinyx McSinyx force-pushed the tmp-file branch 2 times, most recently from 744fa7d to 7ec31a7 Compare March 29, 2020 10:36
@McSinyx
Copy link
Contributor Author

McSinyx commented Mar 31, 2020

@pfmoore, may I have your review on this?

@pradyunsg pradyunsg added the type: enhancement Improvements to functionality label Mar 31, 2020
@pfmoore
Copy link
Member

pfmoore commented Mar 31, 2020

Sorry @McSinyx I haven't had time to review this yet. I'll get to it in the next day or so - feel free to ping me if it seems like I've got distracted!

pfmoore
pfmoore approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

Apart from one minor nit, this looks OK to me

src/pip/_internal/operations/install/wheel.py Outdated Show resolved Hide resolved
pradyunsg
pradyunsg approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM, other than a rephrasing of the NEWS fragment, and the misplaced comment flagged by @pfmoore. :)

news/7699.bugfix Outdated Show resolved Hide resolved
@pradyunsg pradyunsg mentioned this pull request Apr 1, 2020
Merged
15 tasks
@McSinyx
Copy link
Contributor Author

McSinyx commented Apr 1, 2020

Thanks you both for the nits, which are included in the fixup.

@pfmoore pfmoore merged commit 657cf25 into pypa:master Apr 1, 2020
@pfmoore
Copy link
Member

pfmoore commented Apr 1, 2020

Thanks for your work on this, @McSinyx 🙂

@McSinyx McSinyx deleted the tmp-file branch April 2, 2020 07:01
@pradyunsg pradyunsg mentioned this pull request Apr 2, 2020
Closed
@sbidoul sbidoul mentioned this pull request Apr 26, 2020
Closed
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pfmoore pfmoore pfmoore approved these changes

@xavfernandez xavfernandez xavfernandez approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use better temporary files mechanism in install_unpacked_wheel
4 participants
@McSinyx @pfmoore @xavfernandez @pradyunsg