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 StopIteration behaviour in AsCompleted.batches #1071

Merged
merged 1 commit into from
May 11, 2017

Conversation

slavoutich
Copy link
Contributor

PEP 479: generators should return instead of raise StopIteration

Fixes #1069

    PEP 479: generators should `return` instead of `raise StopIteration`
@pitrou
Copy link
Member

pitrou commented May 9, 2017

Wouldn't it better to fix next_batch to return an empty list instead of raising StopIteration?

@slavoutich
Copy link
Contributor Author

slavoutich commented May 9, 2017

It's rather matter of taste. Either we catch exception, or check next_batch for length. Exception is caught once, and len(self.next_batch()) > 0 will be checked every iteration.

@pitrou
Copy link
Member

pitrou commented May 9, 2017

next_batch isn't an iterator, so I wouldn't expect it to raise StopIteration in any case. Either return an empty list, or raise an error such as ValueError or EOFError.

@slavoutich
Copy link
Contributor Author

I'd prefer raising in that case, but I think that ValueError or EOFError are even worse here than StopIteration. GeneratorExit might be better.

@pitrou
Copy link
Member

pitrou commented May 9, 2017

GeneratorExit, as StopIteration, is not supposed to be raised by normal functions. Actually GeneratorExit is not even an Exception subclass, so an except Exception clause won't catch it...

@slavoutich
Copy link
Contributor Author

Correct. Anyway, I would prefer either stay with StopIteration, or create custom exception, otherwise people can accidentally enter infinite loop, when use AsCompleted.next_batch() directly.

@mrocklin
Copy link
Member

@pitrou ok to merge? Are there any tests that would be appropriate here?

@pitrou
Copy link
Member

pitrou commented May 11, 2017

I don't think it's easy to test this, except perhaps try to catch and check warnings on 3.6+. I'm going to merge.

@pitrou pitrou merged commit 3e46e0c into dask:master May 11, 2017
@slavoutich slavoutich deleted the generator_fix branch May 12, 2017 12:40
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