classification
Title: argparse.FileType for '-' doesn't work for a mode of 'rb'
Type: behavior Stage: test needed
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Marcel H2, bethard, eli.bendersky, evan_, josh.r, markgrandi, moritz, naufraghi, palaviv, paul.j3, r.david.murray, sedrubal
Priority: high Keywords: patch

Created on 2012-02-29 11:01 by anacrolix, last changed 2017-07-12 17:21 by r.david.murray.

Files
File name Uploaded Description Edit
argparse-filetype-stdio-modes.patch anacrolix, 2012-03-15 18:11
14156.patch bethard, 2012-07-22 21:06 review
14156-2.patch palaviv, 2016-03-05 17:56 patch with tests working review
Messages (23)
msg154612 - (view) Author: Matt Joiner (anacrolix) Date: 2012-02-29 11:01
If an argument of '-' is handled by argparse.FileType, it defaults to sys.stdin. However a mode of 'rb' is ignored, the returned file object does not work with raw bytes.
msg154641 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2012-02-29 15:27
Yes, the problem is in FileType.__call__ - the handling of '-' is pretty simplistic. Patches welcome.
msg155761 - (view) Author: Matt Joiner (anacrolix) Date: 2012-03-14 16:17
Roger that. I'll start on a patch for this in a month or two if all goes well.
msg155923 - (view) Author: Matt Joiner (anacrolix) Date: 2012-03-15 18:11
Steven, patch attached. I lost steam in the unittests with all the meta, suffice it that the names match the file descriptors of the stream sources. i.e. FileType('rb') would give a file with name=0, and so forth. My chosen method also allows other mode flags as well as custom bufsizes.
msg162342 - (view) Author: Moritz Klammler (moritz) Date: 2012-06-05 13:51
I don't know how if this is the perfect solution but it keeps the program from crashing.

1154c1154,1157
<                 return _sys.stdin
---
>                 if 'b' in self._mode:
>                     return _sys.stdin.buffer
>                 else:
>                     return _sys.stdin
1156c1159,1162
<                 return _sys.stdout
---
>                 if 'b' in self._mode:
>                     return _sys.stdout.buffer
>                 else:
>                     return _sys.stdout
msg166170 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2012-07-22 21:06
The fix looks right, but we definitely need a test. I tried to write one, but I'm not sure how to do this properly given how test_argparse redirects standard input and output (so that fileno() doesn't work anymore). I've attached my current (failing) attempt to test this.
msg215299 - (view) Author: paul j3 (paul.j3) * Date: 2014-04-01 07:42
A related issue http://bugs.python.org/issue13824
"argparse.FileType opens a file and never closes it"

I proposed a FileContext class that returns a `partial(open, filename,...)' context object.  The issues of how to deal with stdin/out are similar.
msg215410 - (view) Author: paul j3 (paul.j3) * Date: 2014-04-03 00:11
There are a couple of complications to using 'fileno'.

We probably don't want to close 'sys.stdin' or 'sys.stdout' (not even if they are redirected to other files?).  That means using:

    open(sys.stdin.fileno(), ..., closefd=False)

'closefd', on the other hand, has to be True for string file specifications.

But in 'test_argparse.py', 'sys.stdout' is redirected to an 'io.StringIO'.  This has many of the same features as an open file, but 'fileno' is not implemented.  So the TypeFile probably needs to make an exception for this case.  I don't how this will play with a 'BytesIO' for 'wb' cases.
msg221271 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-06-22 16:17
Nosy-ing myself since I just ran into it. Annoying issue that precludes from using argparse's builtin '-' recognition for reading binary data.

I'll try to carve some time later to look at the patches.
msg221456 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-06-24 13:27
The patch looks reasonable? Is the only remaining problem with crafting the test?
msg221457 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2014-06-24 13:28
[sorry, the first question mark shouldn't be - the patch indeed looks reasonable to me]

Steven - how about launching a subprocess for stdin tests to avoid weird issues?
msg261218 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-03-05 17:56
I fixed the tests to work with Steven patch. Also changed the patch to open sys.std{in,out} with closefd=False.

I changed the 'io.StringIO' that we redirect the stdout, stderr to. Now the 'StdIOBuffer' return the real stdout,stderr when '-' is passed to FileType. This was already done in the 'stderr_to_parser_error' function previously after the call to 'parse_args'.
msg282851 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-12-10 13:47
Pinging as mentioned in the devguide.
msg282922 - (view) Author: Matt Joiner (anacrolix) Date: 2016-12-11 14:55
This is why I stopped contributing to Python.
msg282948 - (view) Author: paul j3 (paul.j3) * Date: 2016-12-11 21:21
The problem with the argparse backlog is that the original author of the module is too busy with other things.  And no one has stepped into his shoes.  There are people with experience in apply patches, and people who know the argparse code well, but few, if any with both skills (and/or the time to invest in this module).  

In addition the module has some serious backward compatibility issues.  I know of several patches that were applied, and then withdrawn because of unforseen (or at least untested) compatibility problems.  

While I commented earlier, I don't recall testing it.  I just tried it now, and ran into problems - until I realized this isn't compatible with Python2.7.  Py3 is the development world, but there's still a lot of PY2 use (e.g look at Stackoverflow argparse questions).  On SO if people have problems with FileType, I often recommend that they just accept the filename, and take care of opening it themselves.  

To raise the attention to this patch I'd suggest 

- making the case that it is really needed

- demonstrating that it has been field tested, and is free of backward compatibility issues.
msg283210 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-12-14 18:32
Hi paul thanks for looking into this. First are you sure this is a bug in python 2? If so I will happily port this patch once it is reviewed.
As for use cases you may look at issue #26488. Although the patch was rejected you can see that I first used argparse.FileType and moved it because of this issue. I can't prove this patch is free of backward's compatibility issue's but there are tests now which should help to avoid problem.
msg283213 - (view) Author: paul j3 (paul.j3) * Date: 2016-12-14 19:10
The problem with Python2.7 is that 'open' does not take 'closefd', or any of the other parameters that were added for Python3.

    open(name[, mode[, buffering]])

'rb' may make a difference on Py2 on Windows, but I haven't done any work in the environment in a long time.

I wasn't aware of that other issue.  Some core Python developers have participated in that one.  I suspect a lot of the discussion is beyond my level of expertise.

I once wrote that I thought 'FileType' was included primarily as an example of a 'type' factory.  Something users could copy and extend for their own use.  Bethard corrected me, saying that it was meant for quick-n-dirty script uses, ones with an input file, output file and a few options.  In a bigger scripts, the users are encouraged to open/close files in 'with' contexts.

See http://bugs.python.org/issue22884 and the issues I reference there.
msg283216 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-12-14 20:00
As we talk here about stdin and stdout which we don't want to close I think this issue is irrelevant to python2. In any case the patch in issue #13824 cover that problem.
I actually write a lot of small scripts and if I need to open the file in binary mode I don't use FileType because of this issue.
Any way I think this discussion is irrelevant because it does not seem like any core developer is currently working on argparse.
msg283463 - (view) Author: Evan Andrews (evan_) * Date: 2016-12-17 03:45
This issue is relevant to Python 2 on Windows since you need to disable the EOL conversion if you're trying to receive binary data on stdin.

See: http://stackoverflow.com/questions/2850893/reading-binary-data-from-stdin
msg293095 - (view) Author: Matteo Bertini (naufraghi) * Date: 2017-05-05 10:31
Bumped in this bug yesterday, sadly a script working (by chance) in Python2 doesn't work in Python3 because of this bug.
msg294544 - (view) Author: Marcel H (Marcel H2) Date: 2017-05-26 10:03
I want to see this fixed in python3.x as well, please :) the patch should be the same
msg297997 - (view) Author: (sedrubal) Date: 2017-07-09 21:07
What is the problem with using the patch by moritz (https://bugs.python.org/issue14156#msg162342) and change the unit test to this:


class TestFileTypeWB(TempDirMixin, ParserTestCase):
    ...
    successes = [
        ...,
        ('-x - -', NS(x=sys.stdout.buffer, spam=sys.stdout.buffer)),
    ]


and respectively for stdin?
msg298237 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-07-12 17:21
The biggest problem, as paul.j3 says, is to get someone from core to review the argparse issues.  I am currently planning to make argparse one of my foci in a sprint we are doing at the beginning of September, so there is some hope....

Any reviews/testing people do on argparse patches between now and then will be helpful.
History
Date User Action Args
2017-07-12 17:21:09r.david.murraysetnosy: + r.david.murray
messages: + msg298237
2017-07-09 21:07:18sedrubalsetnosy: + sedrubal
messages: + msg297997
2017-05-26 10:03:33pitrousetnosy: - pitrou
2017-05-26 10:03:09Marcel H2setnosy: + Marcel H2

messages: + msg294544
versions: + Python 3.6, Python 3.7
2017-05-05 10:31:29naufraghisetnosy: + naufraghi
messages: + msg293095
2016-12-17 03:45:24evan_setnosy: + evan_
messages: + msg283463
2016-12-15 12:47:52anacrolixsetnosy: - anacrolix
2016-12-14 20:00:53palavivsetmessages: + msg283216
2016-12-14 19:10:41paul.j3setmessages: + msg283213
2016-12-14 18:32:58palavivsetmessages: + msg283210
2016-12-11 21:21:20paul.j3setmessages: + msg282948
2016-12-11 14:55:27anacrolixsetmessages: + msg282922
2016-12-10 21:06:06paul.j3setpriority: normal -> high
2016-12-10 13:47:48palavivsetmessages: + msg282851
2016-04-02 00:32:18martin.panterunlinkissue26488 dependencies
2016-03-28 09:06:48SilentGhostlinkissue26488 dependencies
2016-03-05 17:56:49palavivsetfiles: + 14156-2.patch
nosy: + palaviv
messages: + msg261218

2014-09-30 14:31:00serhiy.storchakasetstage: test needed
type: crash -> behavior
versions: - Python 3.2, Python 3.3
2014-06-24 13:28:12eli.benderskysetmessages: + msg221457
2014-06-24 13:27:20eli.benderskysetmessages: + msg221456
2014-06-22 16:17:05eli.benderskysetnosy: + eli.bendersky

messages: + msg221271
versions: + Python 3.3, Python 3.4, Python 3.5
2014-04-30 19:09:21markgrandisetnosy: + markgrandi
2014-04-03 00:11:01paul.j3setmessages: + msg215410
2014-04-01 07:43:00paul.j3setmessages: + msg215299
2014-04-01 03:25:38paul.j3setnosy: + paul.j3
2014-03-27 23:26:56josh.rsetnosy: + josh.r
2012-07-22 21:06:30bethardsetfiles: + 14156.patch

messages: + msg166170
2012-06-05 13:51:40moritzsetversions: + Python 3.2, - Python 3.3
nosy: + moritz

messages: + msg162342

type: behavior -> crash
2012-03-15 18:11:30anacrolixsetfiles: + argparse-filetype-stdio-modes.patch
keywords: + patch
messages: + msg155923
2012-03-14 16:17:47anacrolixsetmessages: + msg155761
2012-02-29 15:27:10bethardsetmessages: + msg154641
2012-02-29 11:25:07eric.araujosetnosy: + pitrou, bethard
2012-02-29 11:01:10anacrolixcreate