-
Notifications
You must be signed in to change notification settings - Fork 647
Conversation
for ; raised == nil; o, raised = Next(f, iter) { | ||
if ret, raised := IsTrue(f, o); raised != nil { | ||
return nil, raised | ||
} else if !ret { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to avoid else when possible.
if ret, raised := ... {
return ...
}
if !ret {
return ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is consequential, but he couldn't use that syntax. He would have to declare ret
outside of the if
:
ret, raised := IsTrue(f, o)
if raised != nil {
return nil, raised
}
if !ret {
return False.ToObject(), nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out. The code I wrote isn't correct for that reason. But declaring outside the if is the style I've followed in the source code. It is more verbose (and also pollutes the outer scope's namespace more) but I think the clarity is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But declaring outside the if is the style I've followed in the source code.
I will change it to early return, but for the record I lifted that pretty much straight from Contains
in core.go
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... declaring outside the if is the style I've tried to follow in the source code... :D
if !raised.isInstance(StopIterationType) { | ||
return nil, raised | ||
} | ||
f.RestoreExc(nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that there's so much boilerplate for iterating over a sequence. seqForEach almost does what you want but doesn't support breaking out of the loop early. We could add support for that by returning an additional bool value from the callback which signals whether to continue. Might be messy though. I'll leave it up to you whether you want to try something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try and see if it gets messy. Thanks for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another issue is that seqForEach
itself only returns the exception. It would have to be modified to return an *Object
as well. Worse than that really -- an object for the false case and one from the true case (and those would probably need to be parameters as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. Yeah. I think I remember trying to do this at some point and ran into the same thing. Let's leave things as-is and address this down the road with a more general mechanism.
Implement the `all` as defined here: * https://docs.python.org/2/library/functions.html#all
Implement the `any` as defined here: * https://docs.python.org/2/library/functions.html#any Note that a new helper function `seqFindFirst` was added that is used to implement `builtinAll`, `builtinAny`, and `seqContains`.
Implement the `cmp` as defined here: * https://docs.python.org/2/library/functions.html#cmp Implementing this builtin required implementing a fair amount of supporting logic to do 3-way comparisons. The 3-way comparison implementation in CPython is somewhat complex, thus there may be some cases that still need support. This is a good start, though.
Implement the
all
as defined here: