Skip to content
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

gh-120057: Add os.environ.refresh() method #120059

Merged
merged 13 commits into from
Jun 10, 2024
Merged

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 4, 2024

Lib/os.py Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

vstinner commented Jun 5, 2024

I rebased the PR to fix a merge conflict.

I addressed @eryksun's and @JelleZijlstra's reviews: thanks!

Doc/library/os.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

@serhiy-storchaka @erlend-aasland: Would you mind to review this change?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

What about os.environb.refresh()? Does it exist? Does it work?

Lib/os.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

@serhiy-storchaka: I addressed your review. Would you mind to review my updated PR?

What about os.environb.refresh()? Does it exist? Does it work?

I added a test on os.environb.refresh(). Yes, it works as expected :-)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Should os.environb.refresh() be documented? We perhaps do not want to attract too much attention to it, as its effect is the same as of os.environ.refresh().

Doc/library/os.rst Outdated Show resolved Hide resolved
Test: use directly os.putenv().
@vstinner
Copy link
Member Author

Should os.environb.refresh() be documented? We perhaps do not want to attract too much attention to it, as its effect is the same as of os.environ.refresh().

Hum, I prefer to not document it at https://docs.python.org/dev/library/os.html#os.environb :-)

@vstinner vstinner closed this Jun 10, 2024
@vstinner vstinner reopened this Jun 10, 2024
@vstinner
Copy link
Member Author

(Ooops, I closed on [Close] instead of [Comment], sorry.)

Doc/library/os.rst Outdated Show resolved Hide resolved
Lib/test/test_os.py Show resolved Hide resolved
Lib/os.py Show resolved Hide resolved
Lib/test/test_os.py Outdated Show resolved Hide resolved
* Test os.unsetenv()
* Document os.unsetenv()
* Minor refactoring
@vstinner
Copy link
Member Author

@serhiy-storchaka: PR updated for os.unsetenv().

Oh, I didn't know that Python has two functions (putenv, unsetenv) which modify directly the environment without updating os.environ.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Lib/test/test_os.py Show resolved Hide resolved
@vstinner vstinner enabled auto-merge (squash) June 10, 2024 16:08
@vstinner vstinner merged commit 7aff2de into python:main Jun 10, 2024
36 checks passed
@vstinner vstinner deleted the env_refresh branch June 10, 2024 16:34
@@ -193,6 +193,10 @@ process and user.
to the environment made after this time are not reflected in :data:`os.environ`,
except for changes made by modifying :data:`os.environ` directly.

The :meth:`!os.environ.refresh()` method updates :data:`os.environ` with
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR, the :meth: and :func: roles will implicitly add parentheses in the rendered output; you do not need to add them explicitly.

vstinner added a commit to vstinner/cpython that referenced this pull request Jun 20, 2024
…)"

This reverts commit 7aff2de.

The function is controversial and it was decided to remove it.
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants