bpo-29696 Use structseq in string.Formatter.parse iterator response #3980

pablogsal · 2017-10-12T23:42:41Z

This PR solves bpo-29696 improving the readability of the return value of Formatter.parse. Following the requirements of @rhettinger and @serhiy-storchaka, the c-code upstream of the string library was modified to maintain the string module lightweight. This also improves the help section of the result class with a brief description of the result fields.

Now, instead of:

>>> Formatter().parse("mira como bebebn los peces en el {rio} {de} {la} plata")
<formatteriterator object at 0x7f1fc7c7f150>
>>> next(_)
('mira como bebebn los peces en el ', 'rio', '', None)

we obtain

>>> Formatter().parse("mira como bebebn los peces en el {rio} {de} {la} plata")
<formatteriterator object at 0x7f1fc7c7f150>
>>> next(_)
FormatterItem(literal_text='mira como bebebn los peces en el ', field_name='rio', format_spec='', conversion=None)

Please, indicate any change that is needed and I will be more than happy to change it. 😄

https://bugs.python.org/issue29696

rhettinger

Can you please add tests.

bedevere-bot · 2017-10-14T16:01:55Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2017-10-14T17:37:39Z

I have made the requested changes; please review again

bedevere-bot · 2017-10-14T17:37:41Z

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

facundobatista

Thanks for this work!

Patch applied OK and tests run fine.

I annotated some changes to do.

Thanks again!!

facundobatista · 2017-10-17T01:12:43Z

Lib/test/test_unicode.py

+        self.assertEqual(formatter.n_sequence_fields, 4)
+        self.assertNotEqual(formatter.__class__,tuple)
+        self.assertIsInstance(formatter,tuple)
+        self.assertEqual(formatter.__class__.__name__,"FormatterItem")


Please comply PEP-8 in these last three lines.

facundobatista · 2017-10-17T01:13:43Z

Misc/NEWS.d/next/Library/2017-10-13-00-35-15.bpo-29696.aWzShQ.rst

@@ -0,0 +1,2 @@
+Use namedtuple (structseq ovbject) in string.Formatter.parse iterator


typo in "ovbject"

facundobatista · 2017-10-17T01:15:41Z

Objects/stringlib/unicode_format.h

        PyObject *format_spec_str = NULL;
        PyObject *conversion_str = NULL;
-        PyObject *tuple = NULL;
+        PyObject* res = NULL;


please follow the convention and write PyObject *res

bedevere-bot · 2017-10-17T01:16:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2017-10-17T20:29:31Z

I have made the requested changes; please review again.

bedevere-bot · 2017-10-17T20:29:33Z

Thanks for making the requested changes!

@rhettinger, @facundobatista: please review the changes made to this pull request.

serhiy-storchaka · 2017-10-17T20:37:13Z

Objects/stringlib/unicode_format.h


-        tuple = PyTuple_Pack(4, literal_str, field_name_str, format_spec_str,
-                             conversion_str);
+        Py_XINCREF(literal_str);


None of these pointers is NULL, isn't?

And why increment refcounters of non-borrowed references and later decrement them?

Fixed in 814cb3b. That's right, none of the pointers is NULL. The reason for the increment and decrement is because I was getting segfaults so I tried to use idempotent incref-decref mirroring the ones that happen in PyTuple_Pack and after to check if any of the segfaults was due to incorrect incref/decref. I just removed both the increfs and the decrefs and everything is OK. Sorry for the confusion.

serhiy-storchaka · 2017-10-17T20:40:31Z

Objects/stringlib/unicode_format.h

+static PyTypeObject FormatterIterResultType;
+
+static PyStructSequence_Desc formatter_iter_result_desc = {
+    "FormatterItem",


Shouldn't a module name be added?

Sorry, I am confused. Do you mean to expose the type in a module or to add a doc to the second field?

Shouldn't the name of the type include the name of the module?

Done in 3ee0266.

serhiy-storchaka · 2017-10-17T20:45:29Z

Lib/test/test_unicode.py

+        self.assertEqual(formatter, expected_formatter)
+
+        for result, expected in zip(formatter, expected_formatter):
+            self.assertEqual(expected,


First argument is an actual value, second argument is an expected value.

Fixed in 814cb3b.

serhiy-storchaka · 2017-10-17T20:46:04Z

Lib/test/test_unicode.py

+                    (result.literal_text,
+                     result.field_name,
+                     result.format_spec,
+                     result.conversion,))


Redundant trailing comma.

Fixed in 814cb3b.

serhiy-storchaka · 2017-10-17T20:46:35Z

Misc/NEWS.d/next/Library/2017-10-13-00-35-15.bpo-29696.aWzShQ.rst

@@ -0,0 +1,2 @@
+Use namedtuple (structseq object) in string.Formatter.parse iterator


"named tuple"

Fixed in 814cb3b.

serhiy-storchaka · 2017-10-17T21:13:12Z

Lib/test/test_unicode.py

+                     result.field_name,
+                     result.format_spec,
+                     result.conversion)
+                    ,expected)


Oh, this comma looks just ugly.

I would use 4 separate tests:

self.assertEqual(result.literal_text, expected[0]) ...

Done in ff5651c.

serhiy-storchaka · 2017-10-17T21:14:13Z

Objects/stringlib/unicode_format.h

-        Py_XDECREF(format_spec_str);
-        Py_XDECREF(conversion_str);
-        return tuple;
+


There is a leak in case of error.

Decrefs are needed only in case of error. If values are set to a named tuple, they are not needed.

Rename done to error and add return before it.

Done in ff5651c.

serhiy-storchaka · 2017-10-17T21:17:17Z

Objects/stringlib/unicode_format.h

-                             conversion_str);
+        PyStructSequence_InitType(&FormatterIterResultType, &formatter_iter_result_desc);
+        Py_INCREF((PyObject *) &FormatterIterResultType);
+        res = PyStructSequence_New(&FormatterIterResultType);


Check error.

Done in ff5651c.

serhiy-storchaka · 2017-10-17T21:18:02Z

Objects/stringlib/unicode_format.h


-        tuple = PyTuple_Pack(4, literal_str, field_name_str, format_spec_str,
-                             conversion_str);
+        PyStructSequence_InitType(&FormatterIterResultType, &formatter_iter_result_desc);


Wait, do you initialize the type every time when create an instance?

Sorry, I cannot find the appropriate PyInit_{module} function to initialize the type so at this point I was doing it here so there is some implementation to discuss. My apologies 😞 . Where should I initialize the StructSeq?

Sorry, I just found where _string is initialized and I am initializing the type in PyInit__string in f10d429. Please, correct me if this is not the correct place.

bedevere-bot · 2017-10-17T21:18:34Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2017-10-17T22:01:30Z

I have made the requested changes; please review again

bedevere-bot · 2017-10-17T22:01:32Z

Thanks for making the requested changes!

@serhiy-storchaka, @facundobatista, @rhettinger: please review the changes made to this pull request.

serhiy-storchaka

Just added yet few nitpicks.

But actually I'm not convinced that this change is needed.

serhiy-storchaka · 2017-10-18T07:29:24Z

Objects/stringlib/unicode_format.h

        Py_XDECREF(format_spec_str);
        Py_XDECREF(conversion_str);
-        return tuple;
+        return res;


Just return NULL. And initializing res with NULL at line 1035 is not needed.

serhiy-storchaka · 2017-10-18T07:30:16Z

Objects/stringlib/unicode_format.h

+        PyStructSequence_SET_ITEM(res, 3, conversion_str); 
+
+        return res;
+    error:


It would look cleaner if add an empty line before a label.

serhiy-storchaka · 2017-10-18T07:31:35Z

Objects/stringlib/unicode_format.h

+    {"literal_text", "Span of literal text."},
+    {"field_name", "Specifies the object whose value is to be formatted."},
+    {"format_spec", "Contains a specification of how the value should be presented."},
+    {"conversion", "The conversion to be used. One of: ‘s’ (str), ‘r’ (repr) and ‘a’ (ascii)."},


This line looks too long. And maybe the previous line too.

Technically the PR is good.

pablogsal · 2017-10-18T09:27:45Z

IMHO this change makes things a bit more consistent. In lots of places when a tuple is returned, a structseq is used to improve readability on the returned result. Some examples of this are:

grp.struct_group
os.terminal_size
pwd.struct_passwd
resource.struct_rusage
signal.struct_siginfo
time.struct_time
spwd.struct_spwd
sys.float_info
sys.int_info
string.FormatterItem
sys.hash_info
sys.getwindowsversion
sys.flags
sys.version_info
sys.thread_info

facundobatista · 2017-10-18T22:28:31Z

@rhettinger hello! This PR now has some tests.. are these enough for you?

I just run all the tests, so if you're we could merge this...

pablogsal added 2 commits October 13, 2017 00:33

Use namedtuple in string.Formatter.parse iterator response

4428687

Add NEWS entry

37fa89f

the-knights-who-say-ni added the CLA signed label Oct 12, 2017

bedevere-bot added the awaiting review label Oct 12, 2017

rhettinger requested changes Oct 14, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 14, 2017

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 14, 2017

Add tests for the FormatterItem

6cfb991

facundobatista requested changes Oct 17, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Oct 17, 2017

Fix typos and make test PEP8 compliant

5122774

bedevere-bot removed the awaiting changes label Oct 17, 2017

bedevere-bot added the awaiting change review label Oct 17, 2017

serhiy-storchaka reviewed Oct 17, 2017

View reviewed changes

Remove idempotent incref and decref and fix typos and style

814cb3b

serhiy-storchaka previously requested changes Oct 17, 2017

View reviewed changes

bedevere-bot removed the awaiting change review label Oct 17, 2017

bedevere-bot added the awaiting changes label Oct 17, 2017

pablogsal added 2 commits October 17, 2017 22:43

Add error checking, fix leak and clean tests

ff5651c

Add the name of the module to FormatterItem

3ee0266

Initialize the FormatterIterResultType in PyInit__string

f10d429

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 17, 2017

serhiy-storchaka reviewed Oct 18, 2017

View reviewed changes

pablogsal force-pushed the bpo29696 branch from 6318dc4 to be88a9d Compare October 18, 2017 09:44

Minor refactors

7065a5e

pablogsal force-pushed the bpo29696 branch from be88a9d to 7065a5e Compare October 18, 2017 09:46

facundobatista approved these changes Oct 18, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 18, 2017

pablogsal closed this Jul 3, 2018

pablogsal deleted the bpo29696 branch May 19, 2021 18:57

		@@ -0,0 +1,2 @@
		Use namedtuple (structseq ovbject) in string.Formatter.parse iterator

		@@ -0,0 +1,2 @@
		Use namedtuple (structseq object) in string.Formatter.parse iterator

Uh oh!

bpo-29696 Use structseq in string.Formatter.parse iterator response #3980

bpo-29696 Use structseq in string.Formatter.parse iterator response #3980

Uh oh!

Conversation

pablogsal commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhettinger left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 14, 2017

Uh oh!

pablogsal commented Oct 14, 2017

Uh oh!

bedevere-bot commented Oct 14, 2017

Uh oh!

facundobatista 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!

bedevere-bot commented Oct 17, 2017

Uh oh!

pablogsal commented Oct 17, 2017

Uh oh!

bedevere-bot commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Oct 12, 2017 •

edited

Loading

pablogsal Oct 17, 2017 •

edited

Loading

pablogsal Oct 17, 2017 •

edited

Loading

pablogsal Oct 17, 2017 •

edited

Loading

pablogsal commented Oct 18, 2017 •

edited

Loading