bpo-21071: struct.Struct.format type is now str #845
Conversation
|
@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. |
Misc/NEWS
Outdated
There was a problem hiding this comment.
Unicode instead → Unicode string
bytes string → byte string [or bytes object if you prefer]
Doc/library/struct.rst
Outdated
There was a problem hiding this comment.
3.6 → 3.7
I think either “byte string” or “bytes object” would read better.
There was a problem hiding this comment.
Maybe use just str and bytes? There is nothing specific to the Unicode standard here.
There was a problem hiding this comment.
Fixed, I used "str" and "bytes" types.
|
@vadmium, @serhiy-storchaka: I updated my PR to take in account your remarks. Would you mind to review my PR again, please? |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Since this change breaks backward compatibility, it should be documented in What's New.
What is the result of the discussion on Python-Dev?
Misc/NEWS
Outdated
There was a problem hiding this comment.
I think :class: is not needed.
Modules/_struct.c
Outdated
There was a problem hiding this comment.
Why not make s_format an instance of str?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
FWIW I already did this in my format-str.patch. I was able to get a char* out of the string with PyUnicode_AsUTF8.
No answer: https://mail.python.org/pipermail/python-dev/2017-March/147688.html |
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
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM, but add a reference to the issue and wait yet short time for a reaction on Python-Dev.
Doc/whatsnew/3.7.rst
Outdated
There was a problem hiding this comment.
Add a reference to issue.
Doc/whatsnew/3.7.rst
Outdated
There was a problem hiding this comment.
Actually I'm not sure that mentioning this change here is needed. This is not a new feature.
Lib/test/test_struct.py
Outdated
There was a problem hiding this comment.
IMO, self.assertIsInstance(s.format, str) would be better to understand (for future readers) the behavior change.
|
@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: @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. |
It's today. I began by rebasing my change. |
7 participants
No description provided.