bpo-15216: TextIOWrapper support change encoding after creation by methane · Pull Request #2343 · python/cpython

methane · 2017-06-23T04:39:57Z

https://bugs.python.org/issue15216

mention-bot · 2017-06-23T04:40:02Z

@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.

pitrou · 2017-06-23T20:39:57Z

Doc/library/io.rst

Apparently the default value for newline is Ellipsis, not None.

pitrou · 2017-06-23T20:40:48Z

Doc/library/io.rst

Is it ok if data has been written? If so, it should say so.

…er creation

errors should be 'strict' when encoding is not None and errors is not passed.

It make C implementation complicated, and behavor seems inconsistent.

methane · 2017-12-14T07:43:11Z

@pitrou Would you review again?

pitrou · 2017-12-14T13:09:46Z

Doc/library/io.rst

      .. versionadded:: 3.7

-   .. method:: reconfigure(*, line_buffering=None, write_through=None)
+   .. method:: reconfigure(*, encoding, errors, newline, \


The signature should make it clear that parameters are optional (e.g. encoding=None or whatever the default value is).

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?

Is None supported in both cases? If yes, use None.

Not passing newline means "keep current newline".
On the other hand, newline=None means "reset to default newline".

Then perhaps reconfigure(*[, encoding][, errors][, newline][, line_buffering][, write_through])?

pitrou · 2017-12-14T13:12:12Z

Lib/test/test_io.py

+        self.assertEqual(txt.errors, 'replace')
+
+        txt.reconfigure(errors='ignore')
+        self.assertEqual(txt.encoding, 'ascii')


Why no assertEqual on txt.errors here?

pitrou · 2017-12-14T13:23:49Z

Modules/_io/textio.c

+        && !(newline[0] == '\r' && newline[1] == '\0')
+        && !(newline[0] == '\r' && newline[1] == '\n' && newline[2] == '\0')) {
+        PyErr_Format(PyExc_ValueError,
+                     "illegal newline value: %s", newline);


Perhaps use %r here? If there are control characters in newline, it will be difficult to read otherwise.

I can't because newline is char*.
And this code is not changed, just moved.

pitrou · 2017-12-14T13:26:03Z

Modules/_io/textio.c

+{
+    PyObject *old = self->readnl;
+    if (newline == NULL) {
+        self->readnl = NULL;


Shouldn't you Py_CLEAR the old value here? Ditto below.

old will be written back to self->readnl on error, or Py_XDECREF(old) ed on success.

pitrou · 2017-12-14T13:32:37Z

Modules/_io/textio.c

+    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);


Is this guaranteed because of validate_newline? If so, perhaps add a comment.

pitrou · 2017-12-14T13:36:09Z

Modules/_io/textio.c

+        errors = _PyUnicode_FromId(&PyId_strict); /* borrowed */
+    }
+    else if (!PyUnicode_Check(errors)) {
+        // Check 'errors' argument here because AC doesn't support


"Argument Clinic" would be more explicit than "AC", IMHO :-)

pitrou · 2017-12-14T13:40:09Z

Modules/_io/textio.c

+    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");


Why "non seekable"? Does it matter?

Also, what happens if newline is changed while self->decoded_chars is not NULL? Should this all go in textiowrapper_change_encoding?

"non seekable" is wrong. I forgot to update it.
And yes, changing newline should be prohibited.

the-knights-who-say-ni added the CLA signed label Jun 23, 2017

pitrou reviewed Jun 23, 2017

View reviewed changes

methane added 2 commits September 16, 2017 02:49

bpo-15216: TextIOWrapper support set encoding, errors and newline aft…

b74d8d6

…er creation

Add note about changing encoding after write

6213e31

methane force-pushed the reconfigure-encoding branch from 25b65fb to 6213e31 Compare September 15, 2017 17:52

methane added 4 commits September 16, 2017 02:56

Add NEWS file

25a8acf

Fix TextIOWrapper.reconfigure(newline=None)

8d1ca37

Fix _pyio.TextIOWrapper.reconfigure(encoding=...)

687e18a

errors should be 'strict' when encoding is not None and errors is not passed.

Don't detect new encoding is alias of old encoding.

24518bc

It make C implementation complicated, and behavor seems inconsistent.

methane added the awaiting review label Dec 14, 2017

pitrou reviewed Dec 14, 2017

View reviewed changes

methane added 5 commits December 15, 2017 20:32

Merge remote-tracking branch 'origin/master' into reconfigure-encoding

ef44458

Doc: Explain keyword only arguments are optional

4d09f6d

test: Add missing assert

3af545d

Fix comments and error message

78c9cd5

prohibit setting newline after first read

b323dcd

pitrou approved these changes Dec 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 19, 2017

doc: newline can't be changed after read

1018b67

methane merged commit 507434f into python:master Dec 21, 2017

bedevere-bot removed the awaiting merge label Dec 21, 2017

methane deleted the reconfigure-encoding branch December 21, 2017 00:59

Uh oh!

Conversation

methane commented Jun 23, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

methane commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

methane commented Jun 23, 2017 •

edited by bedevere-bot

Loading