Closed mstksg closed 9 years ago
So, I've decided that this is something that is definitely worth making the breaking change for. And better sooner rather than later.
There are two options here that I'm weighing:
accum
to match the desired type. This would silently break all code that used an a -> a -> a
function, where the input and the output/accumulator are the same type (and it would matter when the function is non-commutative). It would error on situations where the input and output are different types, though, at least.mkAccum
, that has the right type and is just like accum
. And erase accum
from the library completely. This has the benefit of breaking all code using accum
, so people can fix...but the drawback is that I really like the name "accum" :'(Just as an additional input, I'll mention something in favor of the current argument order. I don't have a strong opinion on the topic though.
I don't think it will be that often that one will apply both arguments to accum
and friends at the same time. I think it is more likely that the initial state comes from outside, for which the current order of the arguments fits best:
foo :: Monad m => b -> Auto a b
foo = accum $ \b a -> ...
So far I've preferred the current order of arguments in my short experience working with the library. Nevertheless, I think you'll always find reasons for and against flipping the direction of the arguments depending on the situation. I think it is an acceptable default to follow the same order of arguments as foldl
et al. as a means to reduce the element of surprise in the users of these functions.
As a side note, ??
from lens
can be handy in these situations. Example usage in the REPL
> :{
| runState ?? 10 $ do
| s <- get
| put (s + 1)
| return (s > 50)
| :}
(False,11)
Oh, I think I wans't too clear about the change. I was proposing changing
accum :: (b -> a -> b) -> b -> Auto m a b
to
accum :: (a -> b -> b) -> b -> Auto m a b
Changing the ordering in the folding function, not in accum
@mstksg ah, you are right. Among all the excitement of playing with auto
I completely missed your point :)
Nevertheless, I'd be inclined to prefer the “normal” argument order so as to reduce the “surprise factor” as I mentioned above. It's customary to have the b -> a -> b
argument order in left folds, and a -> b -> b
in right folds, I guess that changing those expectations can be a bit frustrating, and perhaps even dangerous if the types of a
and b
match and the given function is not commutative as in the following example:
> foldl (-) 0 [1..5]
-15
> foldr (-) 0 [1..5]
3
Granted, people should be reading the documentation and understanding the types, but it is also helpful not to surprise them :)
After testing changes, I've decided to keep the current order. I couldn't really think of a good alternative name, so that was one reason. I didn't want to really work with maintaining "warnings" for two separate API's that were just slightly different. And, there was one motivating example:
mappender = accum mappend mempty
This is a pretty nice statement...but if we switch the arguments to be input-first, we completely lose how nice all of this...we have to bear with
mappender = accuml (flip mappend) mempty
mappender = accuml (\x acc -> mappend acc x) mempty
I think that it's simply for the use case where the input and output are the same type that the foldr
-based version is weird.
Also, using the foldl
based version helps people avoid needlessly point-free their code...and might lead to more readable code in the long run by encouraging people to make explicit helper functions or spell out lambdas in full.
(How much of this is legitimate and how much is rationalization is up for debate).
For trivia, the main impetus for this change was because the elm programming language has a construct similar to accum
, with the input-first argument ordering (matching foldr
).
I did originally intentionally decide to make
accum
, etc. followfoldl
andscanl
...because they are left folds/scans:so it's nice and all, but it makes it really hard to use it as an "updating function":
Whereas if it was flipped, I could do
this is annoying and makes me very sad.
The only real fix for this would be a huge breaking change across literally all code that uses
accum
ever...and this couldn't be caught by the type checker if the accumulator and input type are the same.Seeing as the library has only just officially been released, it may be that now is the best time to do it...and patch to
0.3.0.0
. Is this worth doing? Maybe. I'll think about it.