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

fix(python): Ensure generator does not raise StopIteration #262

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

paleolimbot
Copy link
Member

As noted in #258, the Python tests are failing! I'm not sure the details of why this started now, but StopIteration was being raised in a generator which should just return instead.

@paleolimbot paleolimbot merged commit 1e596f7 into apache:main Jul 19, 2023
1 check passed
@paleolimbot paleolimbot deleted the python-tests branch July 19, 2023 14:04
@@ -894,8 +894,11 @@ cdef class ArrayStream:
return array

def __iter__(self):
while True:
yield self.get_next()
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is also the use case for yield from in Python

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I just looked it up and my head is about to explode but it does sound cleaner/the thing we should be doing 🤯

Copy link
Member

Choose a reason for hiding this comment

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

I was also refreshing my Python knowledge on this, and so it's actually in the __next__ method that a StopIteration needs to be raised, not in __iter__

So another way to implement this would be:

def __iter__(self):
    return self

def __next__(self):
    return self.get_next()

(but the current one works fine as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants