GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||
GeezerVison is showing github.com — bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close by Ev2geny · Pull Request #22431 · python/cpython Conversation
|
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I just can't figure out why bedevere/issue-number check is failing. Where do I have to mention it? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hmm, I'm not sure I like the API in this solution. If I want to write portable code (Linux and Windows), I would then have to use delete=True and delete_on_close=False in every since call, right? This is going to look at bit convoluted. I'd much rather this was a distinct type of NamedTemporaryFile. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
@blais , yes, this PR assumes, you would need to use There were a lot of suggestions on how to fix this in the original issue14243 discussion with no clear conclusion on what is the best approach. I have chosen this one as it is backwards compatible and does not change philosophy of existing code. I will wait for code reviews to provide their feedback. There must be someone, who takes a final decision on this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It would be wiser to create an alternative interface for this that is safe to use between platforms by default, even if it is more constrained, even if that means providing a completely different context manager. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
OK. So I read the comments in the bug and the workaround here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@zooba Do you have time to take a look at this PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@zooba , just checking whether you managed to look at this |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Can you re-target for 3.12 (Doc/whatsnew/3.10.rst) please? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@MaxwellDupre this is incorrect. Double backticks are usually preferred over single backticks in Please read https://devguide.python.org/documenting/ for a full guide on ReST syntax. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Code change looks good, but we need to get the docs in order.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is way more details than we want here - this will end up in a list with 1000 other changes.
Most of this should move to specific documentation, and then this entry can link to it with :class:`NamedTemporaryFile`
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is fixed. The spesific documentation is updated and linked to with :class:tempfile.NamedTemporaryFile``
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Needs to move to 3.12.rst, and can be about half the length of what's currently in the NEWS file
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Implemented. Whats new is moved to 3.12.rst and only 2 lines now
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Looks like you made a git mistake. Hopefully someone else can help you (maybe just close this PR and create a new one from a fresh clone of the main branch). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…new/3.12.rst and in Lib/tempfile.py" This reverts commit 07076e8.
There was an error while loading. Please reload this page.
Yes, this was something with git. I merged master branch into my PR, but it marked lots of files as being changed. I reverted this meanwhile. But I need some guidelines how to move forward. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Below is the the fixed up Improved Modules section, with each item in proper alphabetical order. I could not commit it because git requires all conflicts fixed in one merge, and I have no idea how to merge your tempfile change with what someone else did since your patch. Use git log or git blame to find the issue that made the alternative change and what it was meant to do. replacement textImproved Modules ================dis
os
pathlib
sqlite3
tempfileThe :class: threading
unicodedata
Note: a few months ago, you did a force push instead of a simple update merge and push. Force pushes sometimes result in the behavior experienced today and are seldom needed. Unclear exactly what you did today. I recommend you fix conflict in your local clone, then merge upstream/main, then push to fork. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@terryjreedy , thanks for this information, but I am afraid I don't think I understand what to do with it and how exactly to move forward. What I want is to refresh my pull request with whatever is latest in the current main branch. so did merge with the current main, which obviousely introduced a lot of new files, but when I pushed it to github it looks like it made impression, that I tried to modify them, whilst I only transfered them fom the main. Also, the work on PR is not finished yet, as there were some more recent reviews from @eryksun |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There are still conflicts. I'd recommend closing this PR and starting over with the changes you want to make. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Just for the record: back then I did rebasing from the master (rather than merge). And then I had to do force push, if I remember correctly |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Based on the suggestions of @gvanrossum and @JelleZijlstra I will close this PR and create a new one |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I new PR 97015 has been created to replace this one. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.