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.

Sign up for GitHub

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

Jump to bottom

gh-87901: os.popen: Remove the encoding argument #92836

Merged
merged 3 commits into from
May 19, 2022
Merged

gh-87901: os.popen: Remove the encoding argument #92836

merged 3 commits into from
May 19, 2022

Conversation

methane
Copy link
Member

@methane methane commented May 16, 2022

No description provided.

@methane methane mentioned this pull request May 16, 2022
Closed
@vstinner vstinner added the needs backport to 3.11 only security fixes label May 16, 2022
@vstinner
Copy link
Member

Since the parameter was added to Python 3.11, I guess your intent is to backport this change to 3.11.

What's the rationale for adding the parameter and then to remove it? #87901 doesn't give details.

@methane
Copy link
Member Author

methane commented May 16, 2022

I want to fix EncodingWarning in stdlib as possible.

Adding encoding parameter is the straight way to fix EncodingWarning. User can pass encoding explicitly.

This PR doesn't fix the EncodingWarning in os.popen. It navigate users to migrate to subprocess.Popen if they want to use consistent encoding regardless UTF-8 mode.

@vstinner
Copy link
Member

Since I wish I could deprecate os.popen(), I like the idea of suggesting users to use the subprocess module directly. It's just that I was surprised to see the parameter added and then removed. Now I understand the rationale, thanks.

vstinner
vstinner approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just added a remark on the doc.

Doc/library/os.rst Outdated
:func:`locale.getpreferredencoding(False) <locale.getpreferredencoding>`_.
The :ref:`Python UTF-8 Mode <utf8-mode>` affects both encodings.

Use :class:`subprocess.Popen` to control encoding and many other options.
Copy link
Member

Choose a reason for hiding this comment

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

The last sentence "... use subprocess to control encoding ..." is fine. But I'm not sure that it's worth it to maintain a documentation of the encoding used by subprocess here. I'm not against it ;-)

vstinner
vstinner approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@methane methane merged commit 96f6583 into python:main May 19, 2022
@methane methane deleted the revert-os-popen branch May 19, 2022 02:42
@miss-islington
Copy link
Contributor

Thanks @methane for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 19, 2022
@bedevere-bot
Copy link

GH-92939 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2022
@methane @miss-islington
pythongh-87901: Remove the encoding argument from os.popen (pythonGH-…
3854081 
…92836)

(cherry picked from commit 96f6583)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
miss-islington added a commit that referenced this pull request May 19, 2022
@miss-islington @methane
gh-87901: Remove the encoding argument from os.popen (GH-92836)
aa55985 
(cherry picked from commit 96f6583)

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
@JelleZijlstra JelleZijlstra mentioned this pull request May 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

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

Successfully merging this pull request may close these issues.
4 participants
@methane @vstinner @miss-islington @bedevere-bot