Remove fold1 from Foldable1 #128
Conversation
|
I'm not particularly keen on this personally, see discussion in #125 |
|
To reduce the amount of breakage outside of core / to make it easier to upgrade, could we rewrite foldMap1Default so that it’s based on foldl1 rather than removing it entirely? |
|
We cannot implement foldMap1 with foldl1 because the accumulator and the values have to be of the same type. |
|
How about adding a Functor constraint and using |
|
Oh right. I don’t think we should promote an inefficient default implementation of foldMap1 that has to traverse twice though, especially since Foldable1 instance will have to be updated anyway and that the existing implementation of fold1 can easily be turned into an implementation of foldMap1. |
|
I think in many contexts, an extra traversal adds a (more or less) negligible performance cost, and having the default implementation there will make upgrading slightly less of a pain so I think I'd still prefer to keep it - especially since, as I just spotted, my proposed implementation is basically the same as the existing one. Most of this library is pretty poor performance-wise; if you care about performance you should probably avoid it and use specialized folds instead anyway. |
|
Should we also bother with foldMap1DefaultL and foldMap1DefaultR default implementations (where foldMap1DefaultR is implemented with foldr1 and we alias foldMap1DefaultL to foldMap1Default) for consistency with Data.Foldable? |
|
That works for me. Are you imagining putting a deprecation warning on foldMap1Default saying to use foldMap1DefaultL instead? |
kl0tl
force-pushed
the
remove-fold1-from-Foldable1
branch
from
11bca07 to
dc70cf3
Compare
kl0tl
force-pushed
the
remove-fold1-from-Foldable1
branch
from
dc70cf3 to
22a7587
Compare
|
To clarify, is this PR ready to be merged? I think all feedback has been addressed. |
|
Looks like this PR broke these libraries:
I'll submit PRs removing the |
|
PRs have been submitted:
|
|
I opened purescript/purescript-ordered-collections#43 to fix the Foldable1 instance for NonEmptySet, it should be the last one in core! |
Neither Foldable nor Bifoldable have a fold or a bifold method, is there any reason for Foldable1 to have fold1?
The only use of foldMap1Default in core libraries occurs in the Foldable1 instance for NonEmptyArray but this is easy enough to fix with a JavaScript implementation (like we do for foldr1 and foldl1), which should also be more efficient since it wouldn’t have to iterate through the array twice.
Close #125, close #117.