I am trying to make LZMAFile (which implements BufferedIOBase) use a BufferedReader in read mode. However this broke test_lzma.FileTestCase.test_read1_multistream(), which calls read1() with the default size argument. This is because BufferedReader.read1() does not accept size=-1: >>> stdin = open(0, "rb", closefd=False) >>> stdin <_io.BufferedReader name=0> >>> stdin.read1() # Parameter is mandatory Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: read1() takes exactly 1 argument (0 given) >>> stdin.read1(-1) # Does not accept the BufferedIOBase default Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: read length must be positive >>> stdin.read1(0) # Technically not positive b'' Also, the BufferedIOBase documentation does not say what the size=-1 value means, only that it reads and returns up to -1 bytes.

Looking at the test suite: * read1() of LZMAFile and GzipFile (both implementing BufferedIOBase) are asserted to return a non-zero result until EOF * LZMAFile.read1(0) is asserted to return an empty string * BufferedReader.read1(-1) is asserted to raise ValueError * There are also tests of read1() methods on HTTPResponse and ZipFile.open() objects, but those methods are undocumented It seems the most consistent way forward would be to: * Define BufferedIOBase.read1(-1) to read and return an arbitrary number of bytes, more than zero unless none are available due to EOF or non-blocking mode. Maybe suggest that it would return the current buffered data or try to read a full buffer of data (with one call) and return it if applicable. * Change the signature to BufferedReader.read1(size=-1) and implement the size=-1 behaviour

Looking at this again, I think a less intrusive way forward would be to: * Document that in 3.6, the required signature is now BufferedIOBase.read1(size). An implementation no longer has to provide a default size, and no longer has to accept negative sizes. * Explicitly document the behaviour of each concrete implementation like GzipFile.read1(-1) etc, if this behaviour is intentional * Fix the BufferedReader error so that “read length must not be negative” Relaxing the read1() signature would allow wider or easier use of BufferedReader, e.g. to implement HTTPResponse as I suggested in Issue 26499. The advantage would be using existing code that is well tested, used, optimized, etc, rather than a custom BufferedIOBase implementation which for the HTTP case is buggy.

Shouldn't read1(-1) be the same as read1(sys.maxsize)? I.e. read from a buffer without limit.

Calling BufferedReader.read1(sys.maxsize) gives me a MemoryError. Making read1(-1) equivalent to read1(sys.maxsize) only makes sense where the return value already has a predetermined size, and only a limited buffer needs to be allocated. Another interpretation is to return an arbitrary, modest buffer size. This is what I ended up doing with LZMAFile.read1() in Issue 23529: return no more than 8 KiB. It is not equivalent to sys.maxsize because more than 8 KiB is possible if you ask for it. HTTPResponse (for non-chunked responses) is similar, but uses a default of 16 KiB.

> Define BufferedIOBase.read1(-1) to read and return an arbitrary number > of bytes, more than zero unless none are available due to EOF or > non-blocking mode. Maybe suggest that it would return the current > buffered data or try to read a full buffer of data (with one call) and > return it if applicable. This sounds reasonable to me.

Okay here is a patch implementing read1(-1) in BufferedReader and BytesIO (my original proposal). Other changes: * Changed read1(size=-1) → read1([size]), because BufferedReader and BytesIO do not accept keyword arguments (see also Issue 23738) * Defined size=-1 to mean an arbitrary non-zero size * Change BufferedReader.read1() to return a buffer of data * Change BytesIO.read1() to read until EOF * Add tests to complement existing read1(size) tests for BufferedReader (includes BufferedRandom), BufferedRWPair, and BytesIO

New changeset b6886ac88e28 by Martin Panter in branch 'default': Issue #23214: Implement optional BufferedReader, BytesIO read1() argument https://hg.python.org/cpython/rev/b6886ac88e28

New changeset d4fce64b1c65 by Martin Panter in branch 'default': Issue #23214: Remove BufferedReader.read1(-1) workaround https://hg.python.org/cpython/rev/d4fce64b1c65

- .. method:: read1(size=-1) + .. method:: read1([size]) I don't understand this change: the default size is documented as -1. Did I miss something? + If *size* is −1 (the default) Is the "−" character deliberate in "−1"? I would expected ``-1``.

The minus sign might have been deliberate at some stage, but I realize it is more accessible etc if it was ASCII -1, so I will change that. The idea of changing the signature is to avoid people thinking it accepts a keyword argument. See e.g. Issue 25030 for seek(offset, whence=SEEK_SET), Issue 14586 for truncate(size=None).

2016-10-21 5:17 GMT+02:00 Martin Panter <report@bugs.python.org>: > The idea of changing the signature is to avoid people thinking it accepts a keyword argument. See e.g. Issue 25030 for seek(offset, whence=SEEK_SET), Issue 14586 for truncate(size=None). Ah. Maybe we should modify the code to accept a keyword argument? :-) Or we might use the strange "read1(\, size=1)" syntax of Argument Clinic.

Modifying the API to accept a keyword argument is not worthwhile IMO. That means modifying modules like http.client and zipfile (which use the wrong keyword name), and user-defined implementations may become incompatible. The question of documentation is discussed more in Issue 23738. I think one or two people were concerned about using the Arg Clinic slash.

New changeset 2af6d94de492 by Martin Panter in branch 'default': Issue #23214: Fix formatting of -1 https://hg.python.org/cpython/rev/2af6d94de492