Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix: Fix Foldable1 NonEmptyArray instance (requires https://github.com/purescript/purescript-nonempty/pull/39) #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
478fb5b
fix: Fix `Foldable1 NonEmptyArray` instance (requires https://github.…
srghma Oct 24, 2020
09e05aa
fix: Fix `Foldable1 NonEmptyArray` instance -> swap foldl1Impl and fo…
srghma Oct 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/Data/Array/NonEmpty/Internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
"use strict";

exports.fold1Impl = function (f) {
exports.foldr1Impl = function (f) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, fixed

return function (xs) {
var acc = xs[xs.length - 1];
for (var i = xs.length - 2; i >= 0; i--) {
acc = f(xs[i])(acc);
}
return acc;
};
};

exports.foldl1Impl = function (f) {
return function (xs) {
var acc = xs[0];
var len = xs.length;
Expand Down
7 changes: 5 additions & 2 deletions src/Data/Array/NonEmpty/Internal.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ derive newtype instance foldableWithIndexNonEmptyArray :: FoldableWithIndex Int

instance foldable1NonEmptyArray :: Foldable1 NonEmptyArray where
foldMap1 = foldMap1Default
fold1 = fold1Impl (<>)
fold1 = foldl1Impl (<>)
foldr1 = foldr1Impl
foldl1 = foldl1Impl

derive newtype instance unfoldable1NonEmptyArray :: Unfoldable1 NonEmptyArray
derive newtype instance traversableNonEmptyArray :: Traversable NonEmptyArray
Expand All @@ -56,7 +58,8 @@ derive newtype instance monadNonEmptyArray :: Monad NonEmptyArray
derive newtype instance altNonEmptyArray :: Alt NonEmptyArray

-- we use FFI here to avoid the unncessary copy created by `tail`
foreign import fold1Impl :: forall a. (a -> a -> a) -> NonEmptyArray a -> a
foreign import foldr1Impl :: forall a. (a -> a -> a) -> NonEmptyArray a -> a
foreign import foldl1Impl :: forall a. (a -> a -> a) -> NonEmptyArray a -> a

foreign import traverse1Impl
:: forall m a b
Expand Down
18 changes: 14 additions & 4 deletions test/Test/Data/Array/NonEmpty.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Data.FunctorWithIndex (mapWithIndex)
import Data.Maybe (Maybe(..), fromJust)
import Data.Monoid.Additive (Additive(..))
import Data.NonEmpty ((:|))
import Data.Semigroup.Foldable (foldMap1)
import Data.Semigroup.Foldable (foldMap1, foldr1, foldl1)
import Data.Semigroup.Traversable (traverse1)
import Data.Tuple (Tuple(..))
import Data.Unfoldable1 as U1
Expand Down Expand Up @@ -297,12 +297,22 @@ testNonEmptyArray = do
log "Unfoldable instance"
assert $ U1.range 0 9 == NEA.range 0 9

log "foldl should work"
log "foldMap1 should work"
assert $ foldMap1 Additive (fromArray [1, 2, 3, 4]) == Additive 10

log "fold1 should work"
-- test through sum
assert $ sum (fromArray [1, 2, 3, 4]) == 10

log "foldMap1 should work"
assert $ foldMap1 Additive (fromArray [1, 2, 3, 4]) == Additive 10
log "foldr1 should work"
assert $ foldr1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a", "b", "c", "d"]) == "(a(b(cd)))"
assert $ foldr1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a", "b"]) == "(ab)"
assert $ foldr1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a"]) == "a"

log "foldl1 should work"
assert $ foldl1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a", "b", "c", "d"]) == "(((ab)c)d)"
assert $ foldl1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a", "b"]) == "(ab)"
assert $ foldl1 (\l r -> "(" <> l <> r <> ")") (fromArray ["a"]) == "a"

log "traverse1 should work"
assert $ traverse1 Just (fromArray [1, 2, 3, 4]) == NEA.fromArray [1, 2, 3, 4]
Expand Down