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