bpo-21071: struct.Struct.format type is now str #845

vstinner · 2017-03-27T11:11:35Z

No description provided.

mention-bot · 2017-03-27T11:11:38Z

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mdickinson, @Yhg1s and @serhiy-storchaka to be potential reviewers.

vadmium · 2017-03-27T11:34:53Z

Misc/NEWS

@@ -291,6 +291,9 @@ Extension Modules
 Library
 -------

+- bpo-21071: struct.Struct.format is now a Unicode instead, instead of a bytes


Unicode instead → Unicode string
bytes string → byte string [or bytes object if you prefer]

vadmium · 2017-03-27T11:40:39Z

Doc/library/struct.rst

@@ -443,6 +443,9 @@ The :mod:`struct` module also defines the following type:

      The format string used to construct this Struct object.

+      .. versionchanged:: 3.6
+         The format string is now a Unicode string, instead of a bytes string.


3.6 → 3.7
I think either “byte string” or “bytes object” would read better.

Maybe use just str and bytes? There is nothing specific to the Unicode standard here.

Fixed, I used "str" and "bytes" types.

vstinner · 2017-06-14T21:27:55Z

@vadmium, @serhiy-storchaka: I updated my PR to take in account your remarks. Would you mind to review my PR again, please?

serhiy-storchaka

Since this change breaks backward compatibility, it should be documented in What's New.

What is the result of the discussion on Python-Dev?

serhiy-storchaka · 2017-06-15T04:46:04Z

Misc/NEWS

@@ -362,6 +362,9 @@ Extension Modules
 Library
 -------

+- bpo-21071: struct.Struct.format type is now :class:`str` instead of


I think :class: is not needed.

serhiy-storchaka · 2017-06-15T04:47:00Z

Modules/_struct.c

@@ -1957,8 +1957,8 @@ s_pack_into(PyObject *self, PyObject **args, Py_ssize_t nargs, PyObject *kwnames
 static PyObject *
 s_get_format(PyStructObject *self, void *unused)
 {
-    Py_INCREF(self->s_format);
-    return self->s_format;
+    return PyUnicode_FromStringAndSize(PyBytes_AS_STRING(self->s_format),


Why not make s_format an instance of str?

I tried to write minimum changes. I expect that most of the code is written to work with C strings char*, not with Python Unicode strings.

FWIW I already did this in my format-str.patch. I was able to get a char* out of the string with PyUnicode_AsUTF8.

vstinner · 2017-06-15T22:46:40Z

What is the result of the discussion on Python-Dev?

No answer: https://mail.python.org/pipermail/python-dev/2017-March/147688.html

vstinner · 2017-06-15T22:55:22Z

Since this change breaks backward compatibility, it should be documented in What's New.

Ok, done.

The change is now documented in the changelog (Misc/NEWS), twice in What's New in Python 3.7 (struct module and Porting to Python 3.7), and in struct documentation. I don't think that you can document it more :-)

Seriously, I don't think that anyone rely on the exact type of struct.Struct.format. If someone cares, it will be trivial to notice, and also likely trivial to fix the code.

serhiy-storchaka

LGTM, but add a reference to the issue and wait yet short time for a reaction on Python-Dev.

serhiy-storchaka · 2017-06-16T04:20:28Z

Doc/whatsnew/3.7.rst

@@ -425,6 +431,9 @@ Changes in the Python API
  ``makedirs()``.
  (Contributed by Serhiy Storchaka in :issue:`19930`.)

+* The :attr:`struct.Struct.format` type is now :class:`str` instead of
+  :class:`bytes`


Add a reference to issue.

serhiy-storchaka · 2017-06-16T04:21:37Z

Doc/whatsnew/3.7.rst

+struct
+------
+
+The :attr:`struct.Struct.format` type is now :class:`str` instead of


Actually I'm not sure that mentioning this change here is needed. This is not a new feature.

ok, removed

berkerpeksag · 2017-06-16T12:52:51Z

Lib/test/test_struct.py

@@ -618,6 +618,14 @@ def test_issue29802(self):
        # Shouldn't crash.
        self.assertEqual(struct.unpack(b'b', b'a'), (b'a'[0],))

+    def test_format_attr(self):
+        s = struct.Struct('=i2H')
+        self.assertEqual(s.format, '=i2H')


IMO, self.assertIsInstance(s.format, str) would be better to understand (for future readers) the behavior change.

vstinner · 2017-06-20T12:49:52Z

@serhiy-storchaka: "LGTM, but add a reference to the issue and wait yet short time for a reaction on Python-Dev."

Serhiy wrote an email to my old thread:
https://mail.python.org/pipermail/python-dev/2017-June/148360.html

@ncoghlan wrote "As long as it's noted in the "Porting to Python 3.7" section of the 3.7 What's New guide, this seems like a sensible change to me."

It's the case, I added an entry to Porting to Python 3.7. I will wait until Friday (June 23) evening.

vstinner · 2017-06-23T13:00:18Z

It's the case, I added an entry to Porting to Python 3.7. I will wait until Friday (June 23) evening.

It's today. I began by rebasing my change.

the-knights-who-say-ni added the CLA signed label Mar 27, 2017

vadmium reviewed Mar 27, 2017

View reviewed changes

brettcannon added the type-feature A feature request or enhancement label Mar 27, 2017

serhiy-storchaka reviewed Jun 15, 2017

View reviewed changes

serhiy-storchaka approved these changes Jun 16, 2017

View reviewed changes

berkerpeksag reviewed Jun 16, 2017

View reviewed changes

bpo-21071: struct.Struct.format type is now str

c541510

vstinner merged commit f87b85f into python:master Jun 23, 2017

vstinner deleted the struct_format branch June 23, 2017 13:11

Uh oh!

bpo-21071: struct.Struct.format type is now str #845

bpo-21071: struct.Struct.format type is now str #845

Uh oh!

Conversation

vstinner commented Mar 27, 2017

Uh oh!

mention-bot commented Mar 27, 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!

vstinner commented Jun 14, 2017

Uh oh!

serhiy-storchaka left a comment

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!

vstinner commented Jun 15, 2017

Uh oh!

vstinner commented Jun 15, 2017

Uh oh!

serhiy-storchaka left a comment

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!

vstinner commented Jun 20, 2017

Uh oh!

vstinner commented Jun 23, 2017

Uh oh!

Uh oh!