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

subprocess.run should alias universal_newlines to text #75937

Closed
andyclegg mannequin opened this issue Oct 11, 2017 · 6 comments
Closed

subprocess.run should alias universal_newlines to text #75937

andyclegg mannequin opened this issue Oct 11, 2017 · 6 comments
Assignees
Labels
3.7 stdlib type-feature

Comments

@andyclegg
Copy link
Mannequin

@andyclegg andyclegg mannequin commented Oct 11, 2017

BPO 31756
Nosy @warsaw, @gpshead, @ncoghlan, @andyclegg, @zooba
PRs
  • #4049
  • 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 = 'https://github.com/gpshead'
    closed_at = <Date 2017-10-23.02:03:49.650>
    created_at = <Date 2017-10-11.10:16:51.865>
    labels = ['3.7', 'type-feature', 'library']
    title = 'subprocess.run should alias universal_newlines to text'
    updated_at = <Date 2017-10-23.02:03:49.649>
    user = 'https://github.com/andyclegg'

    bugs.python.org fields:

    activity = <Date 2017-10-23.02:03:49.649>
    actor = 'gregory.p.smith'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2017-10-23.02:03:49.650>
    closer = 'gregory.p.smith'
    components = ['Library (Lib)']
    creation = <Date 2017-10-11.10:16:51.865>
    creator = 'andrewclegg'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31756
    keywords = ['patch']
    message_count = 6.0
    messages = ['304125', '304149', '304150', '304158', '304198', '304774']
    nosy_count = 5.0
    nosy_names = ['barry', 'gregory.p.smith', 'ncoghlan', 'andrewclegg', 'steve.dower']
    pr_nums = ['4049']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31756'
    versions = ['Python 3.7']

    @andyclegg
    Copy link
    Mannequin Author

    @andyclegg andyclegg mannequin commented Oct 11, 2017

    Following on from https://bugs.python.org/issue6135

    The subprocess module by default returns bytes from subprocess calls. It has a text mode, but this can only be accessed by slightly tangential arguments (setting encoding, errors or universal_newlines).

    ncoghlan notes in msg304118 that this is a similar situation to the binary/text mode settings for open(). From the docs " In text mode, if encoding is not specified the encoding used is platform dependent: locale.getpreferredencoding(False) is called to get the current locale encoding"

    The universal_newlines argument now effectively just turns on the text mode, however this is not an intuitively and obviously discoverable. So to improve usability, and to mirror the file opening behaviour, subprocess calls should be *explicitly* dual mode (binary/text), and have an explicitly named argument to control this.

    My enhancement suggestion is as follows:

    • Add a text=True/False argument that is an alias of universal_newlines, to improve the API.
    • Clearly document that this implies that the encoding will be guessed, and that an explicit encoding can be given if the guess is wrong

    For completeness, the following changes could also be made, although these may be controversial

    • Remove/deprecate the universal_newlines argument
    • Revert the default mode to text (as in Python 2.7), and have a binary=True argument instead

    @andyclegg andyclegg mannequin added 3.7 stdlib type-feature labels Oct 11, 2017
    @zooba
    Copy link
    Member

    @zooba zooba commented Oct 11, 2017

    Really, this is just an alias for universal_newlines in Popen.__init__. So we add the parameter and:

    + if text:
    + universal_newlines = True
    self.universal_newlines = universal_newlines

    And 99% of the change is making it clear in the docs why we have two arguments with the same meaning.

    @zooba
    Copy link
    Member

    @zooba zooba commented Oct 11, 2017

    just an alias

    Which I recognise is in the bug title. My point is that the enhancement itself is less relevant than the rationale and the documentation.

    Without a doc patch, there's really nothing to discuss here other than duplicating APIs (which is probably justified, even though it looks like a wart).

    @andyclegg
    Copy link
    Mannequin Author

    @andyclegg andyclegg mannequin commented Oct 11, 2017

    OK great, I'll get working on a patch.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Oct 12, 2017

    As far as docs phrasing goes, it probably makes sense to frame it as "text" being the preferred argument name in 3.7+, with "universal_newlines" retained indefinitely as a compatibility preserving alias. After all, if that wasn't our intention, we wouldn't be adding the more straightforward alias in the first place.

    As a new Py3-only argument, "text" can also be made keyword-only. (The Popen arg list is so long that most folks treat everything other than the first item as keyword-only anyway, but it doesn't hurt to ask the interpreter to enforce that)

    @gpshead gpshead self-assigned this Oct 20, 2017
    @gpshead
    Copy link
    Member

    @gpshead gpshead commented Oct 23, 2017

    New changeset 7fed7bd by Gregory P. Smith (andyclegg) in branch 'master':
    bpo-31756: subprocess.run should alias universal_newlines to text (bpo-4049)
    7fed7bd

    @gpshead gpshead closed this as completed Oct 23, 2017
    @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
    Labels
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants