-
Notifications
You must be signed in to change notification settings - Fork 2.4k
prefer stdlib TemporaryDirectory #7680
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
Conversation
9e5031d to
5746e62
Compare
c1d26e2 to
84ac784
Compare
84ac784 to
e6620c8
Compare
eamanu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we just need to wait for 3.7 drop :-)
In fact python-poetry/poetry-core#460 seems to leave poetry committed to its own implementation until support for python 3.9 (or windows!) can be dropped |
e6620c8 to
8c7cd5f
Compare
|
@dimbleby try to avoid force push |
|
@eamanu thanks but no thanks. per my last comment this is unlikely to be merged for quite some time and it is absolutely more sensible that when that time comes it will be best represented as a single commit on top of All due respect but you have now made two low-value comments on this MR, please stop. |
8c7cd5f to
1e8e831
Compare
1e8e831 to
daf5147
Compare
daf5147 to
ddc6ecc
Compare
ddc6ecc to
8b66b68
Compare
8b66b68 to
b46efeb
Compare
b46efeb to
a3deb9c
Compare
|
with support for python 3.9 now dropped, |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplace usages of a custom temporary_directory context manager and default tempfile.TemporaryDirectory with the standard library’s TemporaryDirectory while enabling ignore_cleanup_errors to avoid cleanup failures, now that Python 3.7 compatibility is dropped. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The switch between
TemporaryDirectory(...)andtempfile.TemporaryDirectory(...)is inconsistent across files; consider standardizing on one import style to keep usage uniform. - Since
ignore_cleanup_errors=Trueis now repeated at each call site, consider wrappingTemporaryDirectoryin a small local helper/contextmanager so the Windows-specific behavior is centralized and easier to adjust later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The switch between `TemporaryDirectory(...)` and `tempfile.TemporaryDirectory(...)` is inconsistent across files; consider standardizing on one import style to keep usage uniform.
- Since `ignore_cleanup_errors=True` is now repeated at each call site, consider wrapping `TemporaryDirectory` in a small local helper/contextmanager so the Windows-specific behavior is centralized and easier to adjust later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dropping python 3.7 per #7674 moves poetry past the Windows python 3.7 bugs that made the homegrown
temporary_directory()worthwhileSummary by Sourcery
Enhancements: