bpo-15216: TextIOWrapper support change encoding after creation #2343
Conversation
|
@methane, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @loewis to be potential reviewers. |
Doc/library/io.rst
Outdated
There was a problem hiding this comment.
Apparently the default value for newline is Ellipsis, not None.
Doc/library/io.rst
Outdated
There was a problem hiding this comment.
Is it ok if data has been written? If so, it should say so.
methane
force-pushed
the
reconfigure-encoding
branch
from
25b65fb to
6213e31
Compare
errors should be 'strict' when encoding is not None and errors is not passed.
It make C implementation complicated, and behavor seems inconsistent.
|
@pitrou Would you review again? |
Doc/library/io.rst
Outdated
| .. versionadded:: 3.7 | ||
|
|
||
| .. method:: reconfigure(*, line_buffering=None, write_through=None) | ||
| .. method:: reconfigure(*, encoding, errors, newline, \ |
There was a problem hiding this comment.
The signature should make it clear that parameters are optional (e.g. encoding=None or whatever the default value is).
There was a problem hiding this comment.
Default value is just a placeholder to detect "not specified".
It's different between Pure Python implementation and C implementation.
How can I document it?
There was a problem hiding this comment.
Is None supported in both cases? If yes, use None.
There was a problem hiding this comment.
Not passing newline means "keep current newline".
On the other hand, newline=None means "reset to default newline".
There was a problem hiding this comment.
Then perhaps reconfigure(*[, encoding][, errors][, newline][, line_buffering][, write_through])?
| self.assertEqual(txt.errors, 'replace') | ||
|
|
||
| txt.reconfigure(errors='ignore') | ||
| self.assertEqual(txt.encoding, 'ascii') |
There was a problem hiding this comment.
Why no assertEqual on txt.errors here?
| && !(newline[0] == '\r' && newline[1] == '\0') | ||
| && !(newline[0] == '\r' && newline[1] == '\n' && newline[2] == '\0')) { | ||
| PyErr_Format(PyExc_ValueError, | ||
| "illegal newline value: %s", newline); |
There was a problem hiding this comment.
Perhaps use %r here? If there are control characters in newline, it will be difficult to read otherwise.
There was a problem hiding this comment.
I can't because newline is char*.
And this code is not changed, just moved.
| { | ||
| PyObject *old = self->readnl; | ||
| if (newline == NULL) { | ||
| self->readnl = NULL; |
There was a problem hiding this comment.
Shouldn't you Py_CLEAR the old value here? Ditto below.
There was a problem hiding this comment.
old will be written back to self->readnl on error, or Py_XDECREF(old) ed on success.
| self->readtranslate = (newline == NULL); | ||
| self->writetranslate = (newline == NULL || newline[0] != '\0'); | ||
| if (!self->readuniversal && self->readnl != NULL) { | ||
| assert(PyUnicode_KIND(self->readnl) == PyUnicode_1BYTE_KIND); |
There was a problem hiding this comment.
Is this guaranteed because of validate_newline? If so, perhaps add a comment.
Modules/_io/textio.c
Outdated
| errors = _PyUnicode_FromId(&PyId_strict); /* borrowed */ | ||
| } | ||
| else if (!PyUnicode_Check(errors)) { | ||
| // Check 'errors' argument here because AC doesn't support |
There was a problem hiding this comment.
"Argument Clinic" would be more explicit than "AC", IMHO :-)
Modules/_io/textio.c
Outdated
| if (encoding != Py_None || errors != Py_None) { | ||
| if (self->decoded_chars != NULL) { | ||
| _unsupported("It is not possible to set the encoding " | ||
| "of a non seekable file after the first read"); |
There was a problem hiding this comment.
Why "non seekable"? Does it matter?
There was a problem hiding this comment.
Also, what happens if newline is changed while self->decoded_chars is not NULL? Should this all go in textiowrapper_change_encoding?
There was a problem hiding this comment.
"non seekable" is wrong. I forgot to update it.
And yes, changing newline should be prohibited.
5 participants
https://bugs.python.org/issue15216