-
Notifications
You must be signed in to change notification settings - Fork 70
fix: Fix Foldable1 NonEmptyArray
instance (requires https://github.com/purescript/purescript-nonempty/pull/39)
#183
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
Conversation
@@ -1,6 +1,6 @@ | |||
"use strict"; | |||
|
|||
exports.fold1Impl = function (f) { | |||
exports.foldr1Impl = function (f) { |
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.
Isn't this the implementation for foldl1
?
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.
yes, I changed a name, because this name is better
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.
Let me clarify. The function name is foldr1Impl
, but the implementation is what I would define for foldl1Impl
. My issue isn't that the you changed the name. My issue is that the name refers to the wrong implementation. If you changed this to foldl1Impl
and changed the currently named foldl1Impl
to foldr1Impl
, then the names would refer to the correct functions. You would also need to update the tests, since they are referring to the wrong outputs 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.
yes, fixed
test/Test/Data/Array/NonEmpty.purs
Outdated
assert $ foldr1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a"]) == "a" | ||
|
||
log "foldl1 should work" | ||
assert $ foldl1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a", "b", "c", "d"]) == "(a(b(cd)))" |
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.
Pretty sure this should be foldr1
's test:
"( <> "a" <>
"(" <> "b" <>
"(" <> "c" <> "d" <> ")"
<> ")"
<> ")"
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.
indeed, tnx, but the innermost d is not wrapped
the test is taken from https://github.com/purescript/purescript-nonempty/pull/39/files
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! I updated my code.
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'm pretty sure you've swapped foldr1
and foldl1
implementations around.
Merged |
…