Navigation Menu

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

Jump to bottom

subprocess.check_output() accept the check keyword argument #84677

Closed
remilapeyre mannequin opened this issue May 4, 2020 · 7 comments
Closed

subprocess.check_output() accept the check keyword argument #84677

remilapeyre mannequin opened this issue May 4, 2020 · 7 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@remilapeyre
Copy link
Mannequin

remilapeyre mannequin commented May 4, 2020

BPO 40497
Nosy @tiran, @ambv, @remilapeyre
PRs
  • bpo-40497: Fix handling of check in subprocess.check_output() #19897
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2021-09-20.15:09:53.160>
created_at = <Date 2020-05-04.09:54:48.850>
labels = ['type-bug', 'library', '3.11']
title = 'subprocess.check_output() accept the check keyword argument'
updated_at = <Date 2021-09-20.15:09:53.158>
user = 'https://github.com/remilapeyre'

    bugs.python.org fields:

    activity = <Date 2021-09-20.15:09:53.158>
actor = 'lukasz.langa'
assignee = 'none'
closed = True
closed_date = <Date 2021-09-20.15:09:53.160>
closer = 'lukasz.langa'
components = ['Library (Lib)']
creation = <Date 2020-05-04.09:54:48.850>
creator = 'remi.lapeyre'
dependencies = []
files = []
hgrepos = []
issue_num = 40497
keywords = ['patch']
message_count = 7.0
messages = ['368027', '368029', '368031', '368032', '368035', '402239', '402240']
nosy_count = 3.0
nosy_names = ['christian.heimes', 'lukasz.langa', 'remi.lapeyre']
pr_nums = ['19897']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue40497'
versions = ['Python 3.11']
    @remilapeyre
    Copy link
    Mannequin Author

    remilapeyre mannequin commented May 4, 2020

    The subprocess.check_output() raises TypeError when given the check keyword-argument:

    Python 3.8.2 (v3.8.2:7b3ab5921f, Feb 24 2020, 17:52:18) 
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import subprocess
>>> subprocess.check_output(['ls'], check=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 411, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
TypeError: run() got multiple values for keyword argument 'check'

    It should just use True as the default when it's not specified in kwargs.

    @remilapeyre remilapeyre mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 4, 2020
    @tiran
    Copy link
    Member

    tiran commented May 4, 2020

    -1

    check_output() should not accept check=False. Please only improve the error message. I would be fine with accepting check=True, too

    @remilapeyre
    Copy link
    Mannequin Author

    remilapeyre mannequin commented May 4, 2020

    check_output() should not accept check=False.

    I thought about raising ValueError instead but subprocess.check_output([...], check=False) is actually a convenient shortcut over subprocess.run([...], stdout=subprocess.PIPE).stdout and I can't think of much drawbacks if someone explicitly ask for the check to be disabled. Is there any way we could have that?

    @tiran
    Copy link
    Member

    tiran commented May 4, 2020

    IMHO it's both confusing and bad API design to have a function like

        validate_result(..., validate=False)

    Now a reviewer has to check that a developer uses the validate_result() function *and* the developer is not passing validate=False into the function.

    #64096 is also provides a new feature, so it cannot get into Python 3.7 and 3.8.

    @remilapeyre
    Copy link
    Mannequin Author

    remilapeyre mannequin commented May 4, 2020

    Now a reviewer has to check that a developer uses the validate_result() function *and* the developer is not passing validate=False into the function.

    Fair enough, I updated the PR to raise ValueError instead.

    @ambv
    Copy link
    Contributor

    ambv commented Sep 20, 2021

    New changeset 4d2957c by Rémi Lapeyre in branch 'main':
    bpo-40497: Fix handling of check in subprocess.check_output() (GH-19897)
    4d2957c

    @ambv
    Copy link
    Contributor

    ambv commented Sep 20, 2021

    Fixed with #64096 for Python 3.11. Thanks! ✨ 🍰 ✨

    @ambv ambv added 3.11 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes labels Sep 20, 2021
    @ambv ambv closed this as completed Sep 20, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @ambv @tiran