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

bpo-14156: Make argparse.FileType work correctly for binary file mode… #13165

Merged
merged 3 commits into from Mar 6, 2022

Conversation

MojoVampire
Copy link
Contributor

@MojoVampire MojoVampire commented May 7, 2019

…s when argument is '-'

Also made modes containing 'a' or 'x' act the same as a mode containing 'w' when argument is '-' (so 'a'/'x' return sys.stdout like 'w', and 'ab'/'xb' return sys.stdout.buffer like 'wb').

Expanded tests to cover binary mode, as well as covering mode 'x'/'xb' in general (we had no tests verifying that opening in exclusive write mode actually behaved correctly).

https://bugs.python.org/issue14156

@MojoVampire
Copy link
Contributor Author

@MojoVampire MojoVampire commented May 7, 2019

For the record, the failures in the Azure Pipelines environment appear to be caused by the tests being run in some manner that causes sys.stdout/sys.stderr to be replaced with io.StringIO before the tests even start. The tests work fine on AppVeyor and Travis CI because this isn't happening, and work fine locally; I'm having a hard time figuring out what the Azure Pipelines are doing that triggers the problem. It does look like a number of the test support packages will replace stdout/stderr with StringIO instances, but it's not obvious what I can do to trigger that behavior locally so I can try and fix it (possibly by using the same TextIOWrapper subclass I changed test_argparse to use precisely to support this use case.

Update: Apparently this only occurs in high verbosity/warning levels, which I hadn't been using when running locally. I've tweaked the tests to fallback to using a unique sentinel value for each buffer when the buffer doesn't exist (it's not really about the buffer existing, it's just a convenient way to confirm that the buffer was used without leaving sys.stdout/sys.stderr patched, so using sentinels is fine). The tests now pass in both verbose and non-verbose modes, and are run in both in different automated tests, so the behavior is definitely being fully tested either way.

…dXXX with io.StringIO, breaking our tests that assume a .buffer exists on the old stdout/stderr. Use a sentinel value for these cases to allow the tests to work (we don't really care about the specific buffer, we just need a way to indicate that the logical .buffers match)
auvipy
auvipy approved these changes May 31, 2019
Copy link

@auvipy auvipy left a comment

good job.

@matrixise matrixise self-assigned this Sep 12, 2019
hamishcoleman added a commit to hamishcoleman/debian-minimal-builder that referenced this issue Oct 7, 2019
@Lekensteyn
Copy link

@Lekensteyn Lekensteyn commented Jul 5, 2020

As suggested in the linked Python bug, what about gracefully falling back to plain sys.stdin/sys.stdout if the buffer attribute is not available? That should hopefully reduce the need to add special cases to the test suite and result in simpler test code.

The code looks reasonable otherwise.

@woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Mar 3, 2022

@MojoVampire are you still interested in shepherding this? If not, I can pick up from where you've left off and try to respond to feedback both here and on BPO.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

LGTM!

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 5, 2022

Please add a versionchanged directive in the docs. And maybe an entry in What's New.

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 5, 2022

Or maybe consider it a bugfix?

@woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Mar 6, 2022

Or maybe consider it a bugfix?

As an end user: I think it's worth considering a bugfix!

@serhiy-storchaka serhiy-storchaka merged commit eafec26 into python:main Mar 6, 2022
12 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Mar 6, 2022

Thanks @MojoVampire for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Mar 6, 2022

Sorry, @MojoVampire and @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker eafec26ae5327bb23b6dace2650b074c3327dfa0 3.9

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Mar 6, 2022

GH-31706 is a backport of this pull request to the 3.10 branch.

miss-islington added a commit to miss-islington/cpython that referenced this issue Mar 6, 2022
…s when argument is '-' (pythonGH-13165)

Also made modes containing 'a' or 'x' act the same as a mode containing 'w' when argument is '-'
(so 'a'/'x' return sys.stdout like 'w', and 'ab'/'xb' return sys.stdout.buffer like 'wb').
(cherry picked from commit eafec26)

Co-authored-by: MojoVampire <shadowranger+github@gmail.com>
miss-islington added a commit that referenced this issue Mar 6, 2022
…s when argument is '-' (GH-13165)

Also made modes containing 'a' or 'x' act the same as a mode containing 'w' when argument is '-'
(so 'a'/'x' return sys.stdout like 'w', and 'ab'/'xb' return sys.stdout.buffer like 'wb').
(cherry picked from commit eafec26)

Co-authored-by: MojoVampire <shadowranger+github@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Mar 18, 2022

GH-31979 is a backport of this pull request to the 3.9 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Mar 18, 2022
…e modes when argument is '-' (pythonGH-13165)

Also made modes containing 'a' or 'x' act the same as a mode containing 'w' when argument is '-'
(so 'a'/'x' return sys.stdout like 'w', and 'ab'/'xb' return sys.stdout.buffer like 'wb')..
(cherry picked from commit eafec26)

Co-authored-by: MojoVampire <shadowranger+github@gmail.com>
serhiy-storchaka added a commit that referenced this issue Mar 18, 2022
…e modes when argument is '-' (GH-13165) (GH-31979)

Also made modes containing 'a' or 'x' act the same as a mode containing 'w' when argument is '-'
(so 'a'/'x' return sys.stdout like 'w', and 'ab'/'xb' return sys.stdout.buffer like 'wb').
(cherry picked from commit eafec26)

Co-authored-by: MojoVampire <shadowranger+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants