Raise IterationError on StopIteration#473
Conversation
|
Thanks Ryan! I think I'd prefer to create our own |
|
@eriknw I'll update the python 3 PR when this gets merged into master. |
|
@eriknw does this need anything else before it can be merged? |
|
This looks pretty good. As a small nit, I would prefer different error messages. For example, |
|
@eriknw the exception is raised in def nth(n, seq):
""" The nth element in a sequence
>>> nth(1, 'ABC')
'B'
"""
if isinstance(seq, (tuple, list, Sequence)):
return seq[n]
else:
try:
return first(itertools.islice(seq, n, None))
except IterationError:
raise IterationError("Element {} does not exist".format(n))With the exception chaining in Python 3, this is handled nicely. |
Catches the IterationError from first and produces a more meaningful exception message.
|
LGTM! |
|
What the errors look like now. @eriknw if this is fine with you, I believe this is also merge-able. |
|
Thanks again. I don't think it's necessary or informative to chain exceptions. In fact, I may actually prefer a little more verbose by using the for _ in it:
break. # skip the first element in `it`
else:
raise IterationError()I suspect this is faster too. |
|
@eriknw I've think you've done it again! 👍 Using for loops seems to be faster on my machine. I'll update the PR. |
|
Would this mean that def second(seq):
it = iter(seq)
for item in it:
break
else:
raise IterationError
for item in it:
return item
else:
raise IterationError |
|
Yeah, that's a good implementation of def second(seq):
it = iter(seq)
for first_element in it:
for second_element in it:
return second_element
raise IterationError('only 1 in seq blah blah')
raise IterationError('empty seq blah blah') |
|
@eriknw I went with the non-nested implementation for second. While they have very similar performance, the nested version seemed to display a greater variance in performance than the non-nested version on my machine. |
|
BTW, there were small performance improvements for most of the functions involved in this PR. 👍 I can post benchmarks if needed. |
|
All timing show the original function first followed by the modified function.
|
|
I think this looks good. One more thing: should we put |
|
@eriknw What do you think about putting the I'm working on a cytoolz PR that syncs the changes in this PR. |
Resolves #465.