Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bpo-25625: add contextlib.chdir#28271

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ambv merged 8 commits intopython:mainfromFFY00:bpo-25625
Oct 19, 2021
Merged

bpo-25625: add contextlib.chdir#28271

ambv merged 8 commits intopython:mainfromFFY00:bpo-25625
Oct 19, 2021

Conversation

FFY00
Copy link
Member

@FFY00FFY00 commentedSep 10, 2021
edited by bedevere-bot
Loading

This is probably the single snippet of code I find myself re-implementing the
most in projects. Not being thread safe is not optimal, but there isn't
really any good way to do so, and that does not negate the huge
usefulness of this function.

Signed-off-by: Filipe Laínslains@riseup.net

https://bugs.python.org/issue25625

This is probably the single snippet of code I find myself re-implementing themost in projects. Not being thread safe is not optimal, but there isn'treally any good way to do so, and that does not negate the hugeusefulness of this function.Signed-off-by: Filipe Laíns <lains@riseup.net>
Co-authored-by: Filipe Laíns <filipe.lains@gmail.com>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Copy link
Member

@tirantiran left a comment

Choose a reason for hiding this comment

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

I'm -1 to add this feature.

The presence of a chdir decorator in Python's stdlib promotes the idea that the feature is a good standard practice. However it is ill-advised to change process global state such as the working directory in most cases.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Technically it looks correct to me. I am not sure that such feature should be added, although the can of worms was opened byredirect_stdout().

FFY00 reacted with thumbs up emoji
@warsaw
Copy link
Member

I agree with the comment that it can be problematic, but OTOH it'svery common to reimplement this code over and over. There's something to be said for providing a solid reference implementation that at least does the best you can do, but with big red warnings for when it can go wrong. Not having this in the stdlib isn't going to stop people from doing it anyway.

ambv reacted with thumbs up emoji

@ambv
Copy link
Contributor

The presence of a chdir decorator in Python's stdlib promotes the idea that the feature is a good standard practice. However it is ill-advised to change process global state such as the working directory in most cases.

I disagree.os.chdir exists since forever, it "promotes" the idea the same way as a context manager would. If anything, the context manager is a better way to do it, since it will at least return to the previous state.

Sure, any global modifications of the environment are potential anti-patterns, like monkey-patching standard input/output that Serhiy already mentioned (seeBPO-30511 for an egregious example why that is a problem).

That being said, again,os.chdir() is a popular facility of the language, used in our unit tests many times. Using a context manager for it would make more sense. In fact,we alreadydo support that.

@1st1
Copy link
Member

I'm really not sure this should be part of contextlib (or the stdlib). I'd just add this as a recipe to the docs.

Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
MemberAuthor

Perpython/steering-council#77, it seems that addition of such a context manager was approved, granted that we have an appropriate warning.
I think the current warning is good enough, does anyone have suggestions?

Otherwise, this PR is now unblocked 🙂

@ambvambv merged commit3592980 intopython:mainOct 19, 2021

Non parallel-safe context manager to change the current working directory.
As this changes a global state, the working directory, it is not suitable
for use in most threaded or aync contexts. It is also not suitable for most
Copy link
Contributor

Choose a reason for hiding this comment

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

typo onaync#29091

jcfr added a commit to jcfr/Slicer that referenced this pull requestApr 29, 2022
This commit implements a non thread-safe context manager to changethe current working directory.Available in Python 3.11 as ``contextlib.chdir`` and adapted frompython/cpython#28271.
jcfr added a commit to Slicer/Slicer that referenced this pull requestApr 29, 2022
This commit implements a non thread-safe context manager to changethe current working directory.Available in Python 3.11 as ``contextlib.chdir`` and adapted frompython/cpython#28271.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ambvambvambv left review comments

@warsawwarsawwarsaw left review comments

@graingertgraingertgraingert left review comments

@tirantirantiran left review comments

@kalvdanskalvdanskalvdans left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@1st11st1Awaiting requested review from 1st11st1 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

10 participants
@FFY00@warsaw@ambv@1st1@graingert@tiran@kalvdans@serhiy-storchaka@the-knights-who-say-ni@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp