Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(179236)

#15216: Support setting the encoding on a text stream after creation

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 11 months ago by ncoghlan
Modified:
2 years, 6 months ago
Reviewers:
vadmium+py
CC:
loewis, ishimoto, Nick Coghlan, AntoinePitrou, haypo, jwilk_jwilk.net, mrabarnett, Arfrever, inada.naoki, Nikratio, rurpy_yahoo.com, berkerpeksag, Martin Panter, storchaka, Kwamenichols534_yahoo.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 1

Patch Set 3 #

Patch Set 4 #

Total comments: 1

Patch Set 5 #

Total comments: 10

Patch Set 6 #

Patch Set 7 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/io.rst View 1 2 3 4 5 6 2 chunks +14 lines, -2 lines 0 comments Download
Lib/_pyio.py View 1 2 3 4 5 6 2 chunks +60 lines, -5 lines 0 comments Download
Lib/test/test_io.py View 1 2 3 4 5 6 1 chunk +128 lines, -0 lines 0 comments Download
Modules/_io/textio.c View 1 2 3 4 5 6 7 chunks +361 lines, -91 lines 0 comments Download

Messages

Total messages: 3
Nick Coghlan
This looks good to me for the Python side of things, just a trivial typo ...
5 years, 8 months ago #1
Nick Coghlan
Just a couple of minor comments about exception types. I'll add my latest thoughts on ...
2 years, 6 months ago #2
Martin Panter
2 years, 6 months ago #3
https://bugs.python.org/review/15216/diff/10882/Doc/library/io.rst
File Doc/library/io.rst (right):

https://bugs.python.org/review/15216/diff/10882/Doc/library/io.rst#newcode844
Doc/library/io.rst:844: :class:`TextIOWrapper` provides one attribute in
addition to those of
“one attribute” needs updating

https://bugs.python.org/review/15216/diff/19681/Doc/library/io.rst
File Doc/library/io.rst (right):

https://bugs.python.org/review/15216/diff/19681/Doc/library/io.rst#newcode843
Doc/library/io.rst:843: .. method:: set_encoding(encoding, errors=None)
This has wandered under the TextIOBase class, compared to Nikolaus Rath’s
previous patches. Not all text streams do character encoding (e.g. StringIO), so
it should not be part of the base class.

https://bugs.python.org/review/15216/diff/19681/Doc/library/io.rst#newcode846
Doc/library/io.rst:846: handler to *errors* (or *strict*, if *errors* is None).
It would be good to use quotes or some other markup for “strict”, to distinguish
it from the parameter names.

https://bugs.python.org/review/15216/diff/19681/Lib/_pyio.py
File Lib/_pyio.py (right):

https://bugs.python.org/review/15216/diff/19681/Lib/_pyio.py#newcode2008
Lib/_pyio.py:2008: raise ValueError("invalid encoding: %r" % encoding)
On 2017/01/08 03:40:21, Nick Coghlan wrote:
> This is a TypeError (since the supplied encoding is of the wrong type
entirely),
> rather than a ValueError.

The __init__() function raises ValueError. But it seems fine to raise TypeError
in the new function.

https://bugs.python.org/review/15216/diff/19681/Lib/test/test_io.py
File Lib/test/test_io.py (right):

https://bugs.python.org/review/15216/diff/19681/Lib/test/test_io.py#newcode3243
Lib/test/test_io.py:3243: 
These functions were meant to be methods of the TextIOWrapperTest class.

If you remove one of the blank lines, it may increase the amount of context
available when patching the code.

https://bugs.python.org/review/15216/diff/19681/Modules/_io/textio.c
File Modules/_io/textio.c (right):

https://bugs.python.org/review/15216/diff/19681/Modules/_io/textio.c#newcode1431
Modules/_io/textio.c:1431: PyObject *canonical_name = NULL;
Indentation is off

https://bugs.python.org/review/15216/diff/19681/Modules/_io/textio.c#newcode1532
Modules/_io/textio.c:1532: Py_CLEAR(self->encoding);
Can this cause problems if encoding is set to NULL and the garbage collector
runs? E.g. null pointer access if user code calls does a read or write?

https://bugs.python.org/review/15216/diff/19681/Modules/_io/textio.c#newcode1537
Modules/_io/textio.c:1537: self->errors = PyBytes_FromString(errors);
Does this need to check for an exception?

https://bugs.python.org/review/15216/diff/19681/Modules/_io/textio.c#newcode1695
Modules/_io/textio.c:1695: /* FIXME: How do we know that zero is the right state
to not
All you have is the recommendation that zero be the “most common state”:
https://docs.python.org/3/library/codecs.html#codecs.IncrementalEncoder.getstate
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+