GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-39682: make `pathlib.Path` immutable by removing (undocumented) support for "closing" a path by using it as a context manager by barneygale · Pull Request #18846 · python/cpython

Conversation

barneygale commented Mar 8, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This implements @pitrou's suggestion of retaining support for using a Path object as a context manager, but making it a no-op.

https://bugs.python.org/issue39682

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

the-knights-who-say-ni added the CLA not signed label

Mar 8, 2020

the-knights-who-say-ni commented Mar 8, 2020

Copy link

Copy Markdown

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 Missing

Our records indicate the following people have not signed the CLA:

@barneygale

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
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting review label

Mar 8, 2020

pitrou requested changes

Mar 8, 2020

View reviewed changes

pitrou left a comment

Copy link

Copy Markdown

Member

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 on the principle, but I don't think we should remove the test entirely, just change it.
Also, please add a NEWS entry. You can do so using blurb or blurb-it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting review label

Mar 8, 2020

bedevere-bot commented Mar 8, 2020

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

Mar 8, 2020

barneygale force-pushed the bpo-39682-pathlib-path-context-manager branch from 626c970 to c89a62d Compare

March 8, 2020 10:51

barneygale commented Mar 8, 2020

Copy link

Copy Markdown

Contributor Author

I have made the requested changes; please review again. Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting change review and removed awaiting changes labels

Mar 8, 2020

bedevere-bot commented Mar 8, 2020

Copy link

Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot requested a review from pitrou

March 8, 2020 11:02

pitrou approved these changes

Mar 8, 2020

View reviewed changes

pitrou left a comment

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting change review labels

Mar 8, 2020

barneygale commented Mar 8, 2020

Copy link

Copy Markdown

Contributor Author

I have! I think it's just making its way through the system currently.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou commented Mar 8, 2020

Copy link

Copy Markdown

Member

Cool, thank you. I'll wait for the bot to notice, then :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

the-knights-who-say-ni added CLA signed and removed CLA not signed labels

Mar 9, 2020

brettcannon requested changes

Mar 9, 2020

View reviewed changes

brettcannon left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Misc/NEWS.d/next/Library/2020-03-08-11-00-01.bpo-39682.AxXZNz.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Mar 9, 2020

bedevere-bot commented Mar 9, 2020

Copy link

Copy Markdown

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

Mar 9, 2020

barneygale force-pushed the bpo-39682-pathlib-path-context-manager branch from 19bea17 to b260194 Compare

March 10, 2020 00:05

barneygale commented Mar 10, 2020

barneygale commented

Mar 21, 2020

View reviewed changes

Comment thread

Lib/pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

pitrou approved these changes

Mar 28, 2020

View reviewed changes

pitrou left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting change review labels

Mar 28, 2020

pitrou commented Mar 28, 2020

Copy link

Copy Markdown

Member

@barneygale Can you rebase/merge from master to re-run CI on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          bpo-39682: make pathlib.Path immutable by removing (undocumented) s…

31c116a

…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 <[email protected]>

barneygale force-pushed the bpo-39682-pathlib-path-context-manager branch from ca3acfd to 31c116a Compare

March 29, 2020 19:34

barneygale commented Mar 29, 2020

Copy link

Copy Markdown

Contributor Author

Rebased on master. Thanks @pitrou and @brettcannon for helping get this through :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aeros reviewed

Mar 30, 2020

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2020-03-08-11-00-01.bpo-39682.AxXZNz.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Misc/NEWS.d/next/Library/2020-03-08-11-00-01.bpo-39682.AxXZNz.rst

d9aa9df

Co-Authored-By: Kyle Stanley <[email protected]>

aeros approved these changes

Mar 30, 2020

View reviewed changes

aeros left a comment

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brettcannon commented Mar 30, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou merged commit 00002e6 into python:master

Apr 1, 2020

bedevere-bot removed the awaiting merge label

Apr 1, 2020

barneygale mentioned this pull request

Apr 3, 2020

bpo-40038: pathlib: remove partial support for preserving accessor when modifying a path #19342

Merged

barneygale commented Apr 3, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

Just noting that I have a follow-up PR - #19342 - that removes the _init() method that was touched by this PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

barneygale mannequin mentioned this pull request

Apr 10, 2022

pathlib.Path objects can be used as context managers #83863

Closed

04d4 mentioned this pull request

Jul 4, 2025

Tikrinant spintos versiją metama error atviriduomenys/spinta#986

Closed

Reviewers

pitrou

pitrou approved these changes

brettcannon

Awaiting requested review from brettcannon

+1 more reviewer

aeros

aeros approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

barneygale commented Mar 8, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Mar 8, 2020

CLA Missing

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Mar 8, 2020

Uh oh!

barneygale commented Mar 8, 2020

Uh oh!

bedevere-bot commented Mar 8, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

barneygale commented Mar 8, 2020

Uh oh!

pitrou commented Mar 8, 2020

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Mar 9, 2020

Uh oh!

barneygale commented Mar 10, 2020

Uh oh!

bedevere-bot commented Mar 10, 2020

Uh oh!

bedevere-bot commented Mar 10, 2020

Uh oh!

barneygale commented Mar 21, 2020

Uh oh!

bedevere-bot commented Mar 21, 2020

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Mar 28, 2020

Uh oh!

barneygale commented Mar 29, 2020

Uh oh!

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barneygale commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

barneygale commented Mar 8, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

brettcannon commented Mar 30, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

barneygale commented Apr 3, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.