-
Notifications
You must be signed in to change notification settings - Fork 263
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
Raise IterationError on StopIteration #473
base: master
Are you sure you want to change the base?
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.