Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-40636: PEP 618 implementation #20921

Merged
merged 27 commits into from Jun 19, 2020
Merged

bpo-40636: PEP 618 implementation #20921

merged 27 commits into from Jun 19, 2020

Conversation

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jun 16, 2020

https://bugs.python.org/issue40636

}
if (i) {
PyErr_Clear();
return PyErr_Format(PyExc_ValueError, "%s() argument %d is too short",
Copy link
Member Author

@gvanrossum gvanrossum Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It bothers me that the implementation gives different errors depending on whether it's the first iterator that's longer or any of the later ones. It seems to assume that the first iterator is "right" and the others are either too short or too long. But isn't it more the case that all we know is that they aren't all of the same length -- we shouldn't judge which one is too long or too short. Perhaps the errors could be changed to something like "iterator N is shorter/longer than the preceding iterators"?

Copy link
Member

@brandtbucher brandtbucher Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, perhaps these are a bit too terse. I'll think on some other options (I also agree that unifying the messages is probably a good idea).

@gvanrossum gvanrossum changed the title PEP 618 implementation (don't merge yet) PEP 618 implementation Jun 16, 2020
@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jun 16, 2020

I removed the "do not merge" from the title because we don't need three ways to indicate this -- two ways (draft mode and the do-not-merge label) seen quite sufficient.

Also one reason not to merge is now gone -- I accepted the PEP.

@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jun 16, 2020

Oh, you do need to add a bpo issue number and a news blurb.

Copy link
Member

@vstinner vstinner left a comment

Note: I'm kind of confused. @gvanrossum created the PR, but @brandtbucher implemented it?

Python/bltinmodule.c Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 17, 2020

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

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jun 17, 2020

@cool-RR volunteered to take the lead on documentation and testing, which can probably be made in separate PRs. I'm happy to monitor the BPO issue until everything is done.

Ram: be sure to include some round-trip pickle tests for zip(..., strict=True) as well. We didn't discuss it before, but it's a decent chunk of what changed here.

Note: I'm kind of confused. @gvanrossum created the PR, but @brandtbucher implemented it?

He opened the PR from my branch in order to provide review comments on it. I was confused at first, too!

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jun 17, 2020

@gvanrossum it looks like I don't have permissions to change the title of the PR or un-draft it. Can you do so, linking bpo-40636?

@gvanrossum gvanrossum changed the title PEP 618 implementation bpo-40636: PEP 618 implementation Jun 17, 2020
@gvanrossum gvanrossum marked this pull request as ready for review Jun 17, 2020
@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jun 17, 2020

Sorry about the unusual workflow. Next time I'll just ask the author to create the PR.

@vstinner
Copy link
Member

@vstinner vstinner commented Jun 17, 2020

Well, if there are permission issues, maybe @brandtbucher can create a new PR.

Lib/test/test_builtin.py Show resolved Hide resolved
@@ -0,0 +1,3 @@
:func:`zip` now supports :pep:`618`'s ``strict`` parameter, which raises a
:exc:`ValueError` if the arguments are exhausted at differing lengths.
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(English is not my first language.) Is "differing" correct here?

Suggested change
:exc:`ValueError` if the arguments are exhausted at differing lengths.
:exc:`ValueError` if the arguments are exhausted at different lengths.

Copy link
Member

@brandtbucher brandtbucher Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure "differing" is correct here (I think a strict reading of "different" may mean that no two lengths were the same).

I'll just change it anyways, since "different" probably makes more sense to more people.

return NULL;
}
PyTuple_SET_ITEM(result, i, item);
}
}
return result;
check:
if (PyErr_Occurred() && !PyErr_ExceptionMatches(PyExc_StopIteration)) {
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in the error label is non-trivial. You may add comments. Here I propose:

// next() on argument i raised an exception (not StopIteration)

Python/bltinmodule.c Show resolved Hide resolved
return NULL;
}
PyTuple_SET_ITEM(result, i, item);
}
}
return result;
check:
if (PyErr_Occurred() && !PyErr_ExceptionMatches(PyExc_StopIteration)) {
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error path is not tested: would you mind to write an unit test for it?

i + 1, plural, i);
}
if (PyErr_Occurred()) {
return NULL;
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This errror path is not tested. Would you mind to add two unit tests for it?

  • One test where an iterator raises StopIteration
  • One test where an iterator raises an exception other than StopIteration

... This is bug in this code: if StopIteration is raised, ValueError is not raised whereas a following argument can be not exhausted yet.

zip(a, b,c, strict=True): if a is exhausted, b raises StopIteration, but c is not exhausted => StopIteration is passed through, whereas a ValueError must be raised because c is not exhausted.

Copy link
Member

@brandtbucher brandtbucher Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always trip on the fact that Python code raises StopIteration but C code usually doesn't. Nice catch.

Python/bltinmodule.c Show resolved Hide resolved
Python/bltinmodule.c Show resolved Hide resolved
@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jun 18, 2020

(...and that's why I am not allowed to approve this PR. :-)

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jun 18, 2020

Thanks @vstinner, I think I addressed all of your comments.

The macOS test_resource_tracker_reused failure is unreleated (it's currently failing on other PRs).

Copy link
Member

@vstinner vstinner left a comment

Thanks, the update test suite seems to have a better code coverage ;-)


def test_zip_strict_error_handling(self):

class E(Exception):
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a name longer than a single letter. MyError; Bug, etc. Whatever works :-)

Ditto for "class I:": Iterator, MyIterator, BuggyIter, etc.

if (i) {
// ValueError: zip() argument 2 is shorter than argument 1
// ValueError: zip() argument 3 is shorter than arguments 1-2
const char* plural = i == 1 ? " " : "s 1-"; // ^^^^
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const char* plural = i == 1 ? " " : "s 1-"; // ^^^^
const char* plural = i == 1 ? " " : "s 1-";

raise E
return self.size

l1 = self.iter_error(zip("AB", I(1), strict=True), E)
Copy link
Member

@vstinner vstinner Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check l1 immediately? Currently, it's not trivial to see the relationship between l1 and the expected value. Maybe write assertEqual(self.iter..., [...]) (on multiple lines).

@brandtbucher brandtbucher requested a review from vstinner Jun 19, 2020
Copy link
Member

@vstinner vstinner left a comment

LGTM. Thanks for the better code coverage and additional comments.

I also checked for refleak: ./python -m test test_builtin -R 3:3 pass successfully (but I had to disable PtyTests, it's unrelated to this PR.)

@vstinner vstinner merged commit 310f6aa into python:master Jun 19, 2020
9 of 10 checks passed
@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jun 19, 2020

Thanks!

@cool-RR
Copy link
Contributor

@cool-RR cool-RR commented Jun 19, 2020

Congrats on the merge. If there are any more tests that you'd like me to write, let me know, with details.

@cool-RR
Copy link
Contributor

@cool-RR cool-RR commented Jun 20, 2020

Do you think we should look for places where zip is used in the stdlib that could benefit from strict=True? If I remember correctly there's at least one example in the PEP.

@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jun 20, 2020

I would be very conservative with such changes.

@gvanrossum gvanrossum deleted the zip-strict branch Jun 20, 2020
fasih pushed a commit to fasih/cpython that referenced this issue Jun 29, 2020
zip() now supports PEP 618's strict parameter, which raises a
ValueError if the arguments are exhausted at different lengths.
Patch by Brandt Bucher.

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Ram Rachum <ram@rachum.com>
J-Arun-Mani pushed a commit to J-Arun-Mani/cpython that referenced this issue Jul 21, 2020
zip() now supports PEP 618's strict parameter, which raises a
ValueError if the arguments are exhausted at different lengths.
Patch by Brandt Bucher.

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Ram Rachum <ram@rachum.com>
chrisburr added a commit to chrisburr/cpython that referenced this issue Dec 9, 2020
zip() now supports PEP 618's strict parameter, which raises a
ValueError if the arguments are exhausted at different lengths.
Patch by Brandt Bucher.

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Ram Rachum <ram@rachum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants