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.reload_environ() function #126268

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 1, 2024

Replace the os.environ.refresh() method with a new os.reload_environ() function.


📚 Documentation preview 📚: https://cpython-previews--126268.org.readthedocs.build/

Replace the os.environ.refresh() method with a new
os.reload_environ() function.
.. function:: reload_environ()

Update :data:`os.environ` and :data:`os.environb` with changes to the
environment made by :func:`os.putenv`, by :func:`os.unsetenv`, or made
Copy link
Member

Choose a reason for hiding this comment

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

I think that it is better to say "the process environment".
os.putenv and os.unsetenv update the cache, so there is no need to reload after them. You should refer to the C functions.
Please add a note that this function is not thread safe. Calling it while the environment is modified in other thread has undefined behavior. Reading from os.environ or calling os.getenv during reloading can return empty result.

Copy link
Contributor

@rruuaanng rruuaanng Nov 1, 2024

Choose a reason for hiding this comment

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

If the process environment is mentioned, perhaps it can be specifically mentioned that it is the current process environment (I think)

Edit
Or, it can be called the current program.

Copy link
Member Author

Choose a reason for hiding this comment

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

os.putenv() and os.unsetenv() don't update os.environ: see test_reload_environ().

@vstinner
Copy link
Member Author

vstinner commented Nov 1, 2024

@serhiy-storchaka: Please review the updated PR. I addressed your review.

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. Let a native speaker to review the wording.

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

vstinner commented Nov 1, 2024

cc @zooba @ncoghlan

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Not a native speaker but here are some suggestions. For native speakers: @python/proofreaders

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

vstinner commented Nov 1, 2024

@picnixz: I applied your suggestions.

Doc/library/os.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

LGTM! (although see @AA-Turner's suggested docs wording tweaks)

Co-authored-by: Adam Turner <[email protected]>
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Pro tips from someone who had lots of linter errors due to suggestions in the past: when submitting a suggestion, I usually Ctrl+A (or "Select all" on mobile) to check whether the text has trailing whitespaces or not. This helps reducing linter errors.

Doc/library/os.rst Outdated Show resolved Hide resolved
Co-authored-by: Bénédikt Tran <[email protected]>
Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@vstinner vstinner merged commit 4a0d574 into python:main Nov 5, 2024
36 checks passed
@vstinner vstinner deleted the reload_environ branch November 5, 2024 07:43
@vstinner
Copy link
Member Author

vstinner commented Nov 5, 2024

Merged, thanks for reviews!

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.

8 participants