-
-
Couldn't load subscription status.
- Fork 33.3k
bpo-39682: make pathlib.Path immutable by removing (undocumented) support for "closing" a path by using it as a context manager #18846
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
bpo-39682: make pathlib.Path immutable by removing (undocumented) support for "closing" a path by using it as a context manager #18846
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). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
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.
| 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 |
626c970 to c89a62d Compare | I have made the requested changes; please review again. Thank you! |
| Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
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.
+1. Have you signed the CLA?
| I have! I think it's just making its way through the system currently. |
| Cool, thank you. I'll wait for the bot to notice, then :-) |
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.
Quick request to document in the code why this change is being made, else someone else in the future may be wondering why it's a no-op.
Misc/NEWS.d/next/Library/2020-03-08-11-00-01.bpo-39682.AxXZNz.rst Outdated Show resolved Hide resolved
| 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 |
19bea17 to b260194 Compare | Thank you Brett. I have made the requested changes; please review again. Hope the comment isn't too long. |
| Thanks for making the requested changes! @brettcannon, @pitrou: please review the changes made to this pull request. |
| 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 |
86cf4dc to ca3acfd Compare | I have made the requested changes; please review again. |
| Thanks for making the requested changes! @pitrou, @brettcannon: please review the changes made to this pull request. |
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.
+1 for this, thank you @barneygale .
I'll leave this open a day or two in case people hold strong opinions about deprecating this silent behaviour.
| @barneygale Can you rebase/merge from master to re-run CI on this? |
…upport for "closing" a path by using it as a context manager. Support for using a path as a context manager remains, and is now a no-op. Co-Authored-By: Brett Cannon <54418+brettcannon@users.noreply.github.com>
ca3acfd to 31c116a Compare | Rebased on master. Thanks @pitrou and @brettcannon for helping get this through :) |
Misc/NEWS.d/next/Library/2020-03-08-11-00-01.bpo-39682.AxXZNz.rst Outdated Show resolved Hide resolved
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
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.
Thanks @barneygale; LGTM.
| I have an opinion about deprecation, but it isn't strong. 😄 Basically I'm worried someone out there is relying on this functionality and is going to be caught off-guard that it suddenly stopped working. But as I said, I'm +0 on the deprecation so I won't hold up the PR if people disagree. |
| Just noting that I have a follow-up PR - #19342 - that removes the |
This implements @pitrou's suggestion of retaining support for using a
Pathobject as a context manager, but making it a no-op.https://bugs.python.org/issue39682