bpo-20180: use Argument Clinic in itertools module where appropriate #4117

taleinat · 2017-10-25T12:15:30Z

I applied my old patch for itertools from the old "AC Derby" days, fixed all that needed to be fixed and made a few minor AC-related fixes as suggested by @serhiy-storchaka on the issue tracker.

To get this working with the separate clinic/itertoolsmodule.c.h file, I also moved all of the method definition arrays and static object definitions to the end of the file, so that the #include statement could come before them.

Many thanks @serhiy-storchaka for reviewing the patch and providing guidance!

https://bugs.python.org/issue20180

This is required to be able to include the generated AC code from clinic/itertoolsmodule.c.h, which must appear before these definitions but after all typedefs and type declarations.

taleinat · 2017-10-25T12:48:36Z

Note that I purposefully made this in two commits to make the steps clearer. Consider reviewing each commit individually.

taleinat · 2017-10-25T13:37:58Z

Also note: While moving around and re-formatting doc-strings (required for AC), I also improved some of them. Reviewers, please make sure to review doc-string changes.

serhiy-storchaka

I left reviewing the docstrings on to @rhettinger.

serhiy-storchaka · 2017-10-25T16:52:06Z

Modules/itertoolsmodule.c

+all have the same key. The common key for the group is the first item in the
+pair.
+
+Example:


Currently examples are not included in docstrings. I think it is better to keep examples in the module documentation and keep docstrings brief and clear.

There were already examples in some of the doc-strings, e.g. for product() and combinations(), so I added some to doc-strings where there were no examples based on examples I found in the docs.

@rhettinger, should I remove all examples from doc-strings, keep only those which were already in the doc-strings, or keep the new examples as well?

Before adding an example, I would try adding a clear summary of the class. Why did you remove the original summary, “an iterator that returns consecutive keys and groups”?

I actually don't remember, this is from a patch I wrote a long time ago... We can just leave the current doc-string (though it doesn't follow the "start with one short line" principle):

Make an iterator that returns consecutive keys and groups from the iterable. If the key function is not specified or is None, the element itself is used for grouping.

I agree, this summary line is not very informative.

I definitely agree that "returns consecutive keys and groups" isn't informative without more context.

Suggestion (sans example):

Return an iterator of groups of values as (key, group) pairs.

This groups together consecutive values from the given iterator. For each group, a (key, group) 2-tuple is yielded, where ``key'' is the common key value for the group, and ``group'' is an iterator of the values in the group.

Revised suggestion:

Return an iterator of groups of items as (key, group) pairs.

This groups together consecutive items from the given iterator. Each group contains items for which the key gives the same result.

For each group, a (key, group) 2-tuple is yielded, where "key" is the common key value for the group and "group" is an iterator of the items in the group.

This looks better to me. But I'm not an English expert, and in any case the last word on to @rhettinger.

I've changed the doc-string to match my latest suggestion. Will await review by @rhettinger.

serhiy-storchaka · 2017-10-25T16:55:35Z

Modules/itertoolsmodule.c

+/*[clinic input]
+itertools.groupby.__reduce__ as groupby_reduce
+
+    lz: self(type="groupbyobject *")


I think that it is better to rename the parameter than use the self converter. This increases the size of the patch, but in long term it makes the code clearer.

The same for other methods below.

But ask @rhettinger first. His can have different preferences.

I'll wait for @rhettinger's input on this.

serhiy-storchaka · 2017-10-25T16:57:08Z

Modules/itertoolsmodule.c

@@ -137,8 +185,17 @@ groupby_next(groupbyobject *gbo)
    return r;
 }

+/*[clinic input]
+itertools.groupby.__reduce__ as groupby_reduce


Is groupby_reduce used anywhere in non-generated code? If no, it is better to keep the name generated by Argument Clinic.

The same for other methods below.

Do you think we should also remove the as for the new functions, e.g. itertools.groupby.new as groupby_new?

groupby_new and groupby_new__doc__ are used in the initializer of groupby_type.

serhiy-storchaka · 2017-10-25T17:38:22Z

Modules/itertoolsmodule.c

+@classmethod
+itertools.starmap.__new__ as starmap_new
+
+    function: object


Parameters should be positional-only.

serhiy-storchaka · 2017-10-25T17:47:56Z

Modules/itertoolsmodule.c

+@classmethod
+itertools.count.__new__ as count_new
+
+    start: object = NULL


start as long_cnt: object(c_default="NULL") = 0 step as long_step: object(c_default="NULL") = 1

* drop all "as" clauses for function names, renaming the relevant C functions instead * count.__new__: add missing default values for arguments and rename the argument variables back to those in the pre-AC code * starmap.__new__: properly define the arguments as positional-only

vadmium · 2017-10-28T22:45:14Z

Modules/itertoolsmodule.c

+all have the same key. The common key for the group is the first item in the
+pair.
+
+Example:


Before adding an example, I would try adding a clear summary of the class. Why did you remove the original summary, “an iterator that returns consecutive keys and groups”?

vadmium · 2017-10-28T22:53:23Z

Modules/itertoolsmodule.c

-     reduce_doc},
-    {NULL,              NULL}   /* sentinel */
-};
-
 PyDoc_STRVAR(starmap_doc,


Seems no longer used

vadmium · 2017-10-28T22:54:22Z

Modules/itertoolsmodule.c

+Create a starmap object.
+
+Return an iterator whose values are returned from the function evaluated
+with a argument tuple taken from the given iterable.


with argument tuples

vadmium · 2017-10-28T23:00:50Z

Modules/itertoolsmodule.c

+
+Create a chain object.
+
+Alternate chain() contructor taking a single iterable argument


Spelling: constructor

(Also, alternative is more correct than alternate outside the US.)

vadmium · 2017-10-28T23:20:16Z

Modules/itertoolsmodule.c

+
+    ro: self(type="repeatobject *")
+
+Private method returning an estimate of len(list(it)).


Aren’t ro and it the same parameter?

Yes, but ro isn't included in the doc-string and would be unclear to users IMO. Suggestions for something more informative than it are welcome! (self?)

Currently this gives:

>>> help(itertools.repeat.__length_hint__) Help on method_descriptor: __length_hint__(self, /) Private method returning an estimate of len(list(it)).

Why not use self?

vadmium · 2017-10-28T23:34:48Z

Modules/itertoolsmodule.c

@@ -4541,6 +4045,990 @@ are exhausted, the fillvalue is substituted in their place.  The fillvalue\n\
 defaults to None or can be specified by a keyword argument.\n\
 ");

+
+/* including clinic output must come after typdefs and PyObject type


typedefs?

rhettinger · 2017-10-29T23:29:44Z

Wow, this is a stunningly massive diff (+2,890 −1,274) to a file that was only 4,684 lines to begin with. I didn't expect so much code to be moved into a .h file (which seems unwise to me and will complicate my ability to understand the code). Also, I only expected the constructor calls to be given to AC, not all of the internal reduce/setstate methods and whatnot.

That said, thank you for the effort. I start working the diffs and it may take a while.

taleinat · 2018-06-12T18:13:39Z

I've created another PR, #4170, with a small subset of these changes (just itertools.groupby). I suggest starting with getting that sorted out, and then deciding if and how to continue with the rest of the module.

rhettinger

I've reviewed, fixed, and applied pr #4170. Please use that as a model for improving this patch.

In particular, please don't move the PyTypeObject struct definitions to the bottom. This will greatly minimize the code churn and will keep the type structs right by beside the related code (which helps maintainability).

Also, just convert the primary documented interfaces. No need for setstate, reduce, etc. Please respect the existing docstrings as much as possible. The goal is to make signatures visible, not to rewrite the documentation. That said, if you find something that is unclear or incorrect, please point it out and I will take a look.

bedevere-bot · 2018-09-10T18:43:03Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

taleinat · 2018-09-10T18:57:56Z

@rhettinger, will do.

If you prefer, I will gladly continue to split this out into small PRs rather than one large one. Just let me know your preference.

rhettinger · 2018-09-10T21:13:37Z

One big patch will be fine. Following the pattern of PR #4170 will give a much more compact and reviewable single patch.

taleinat · 2018-09-11T08:55:25Z

@rhettinger

please don't move the PyTypeObject struct definitions to the bottom

Some things will have to be moved to the top/bottom due to needing to appear before/after the line #include "clinic/itertoolsmodule.c.h". This includes, for example, the PyTypeObject declarations, which must come before that line.

If this is a deal-breaker, please let me know ASAP.

serhiy-storchaka · 2018-09-11T09:43:28Z

Can forward declarations help?

taleinat · 2018-09-11T11:24:02Z

Can forward declarations help?

I'm working on this now and so far it seems that only the static PyTypeObject declarations need to appear at the top. If @rhettinger prefers, we can duplicate them, keeping them in their current positions as well as at the top of the file before the #include.

taleinat · 2018-09-11T14:16:33Z

See new PR #9164 which is a complete reworking of this as per @rhettinger's directions.

rhettinger · 2018-09-11T15:38:03Z

This PR was superseded by PR 9164. Marking as closed.

taleinat added 2 commits October 25, 2017 15:09

bpo-20180: merge old itertools AC patch and run modern clinic.py

a06e2ec

bpo-20180: move method arrays and type definitions to bottom of file

1d65555

This is required to be able to include the generated AC code from clinic/itertoolsmodule.c.h, which must appear before these definitions but after all typedefs and type declarations.

taleinat requested a review from rhettinger as a code owner October 25, 2017 12:15

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

bedevere-bot added the awaiting merge label Oct 25, 2017

taleinat requested a review from serhiy-storchaka October 25, 2017 12:15

taleinat added the skip news label Oct 25, 2017

vstinner self-requested a review October 25, 2017 15:45

serhiy-storchaka reviewed Oct 25, 2017

View reviewed changes

vadmium reviewed Oct 28, 2017

View reviewed changes

taleinat added 2 commits October 29, 2017 17:21

bpo-20180: address additional code review comments

1a78dfb

bpo-20180: run clinic.py again on itertoolsmodule.c

d87b4e9

rhettinger self-assigned this Oct 29, 2017

taleinat mentioned this pull request Oct 30, 2017

bpo-20180: itertools.groupby Argument Clinic conversion #4170

Merged

rhettinger requested changes Sep 10, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 10, 2018

taleinat mentioned this pull request Sep 11, 2018

bpo-20180: convert most of itertoolsmodule.c to use Argument Clinic #9164

Merged

taleinat added the DO-NOT-MERGE label Sep 11, 2018

rhettinger closed this Sep 11, 2018

taleinat removed the DO-NOT-MERGE label Sep 11, 2018


		Create a chain object.

		Alternate chain() contructor taking a single iterable argument


		ro: self(type="repeatobject *")

		Private method returning an estimate of len(list(it)).

Uh oh!

bpo-20180: use Argument Clinic in itertools module where appropriate #4117

bpo-20180: use Argument Clinic in itertools module where appropriate #4117

Uh oh!

Conversation

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

Uh oh!

Uh oh!

taleinat commented Oct 25, 2017

Uh oh!

taleinat commented Oct 25, 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!

taleinat Oct 25, 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!

taleinat Oct 29, 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!

taleinat Oct 29, 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!

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

Uh oh!

Uh oh!

taleinat commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhettinger left a comment • edited by taleinat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

taleinat commented Oct 25, 2017 •

edited by bedevere-bot

Loading

taleinat Oct 25, 2017 •

edited

Loading

taleinat Oct 29, 2017 •

edited

Loading

taleinat Oct 29, 2017 •

edited

Loading

rhettinger commented Oct 29, 2017 •

edited

Loading

taleinat commented Jun 12, 2018 •

edited

Loading

rhettinger left a comment •

edited by taleinat

Loading

rhettinger commented Sep 10, 2018 •

edited by taleinat

Loading