Skip to content

Conversation

milesfrain
Copy link
Contributor

This is a cosmetic change to follow the naming convention of other fold operations, and make the relationship to foldl more obvious.

Previously:

foldl :: forall     a b.                          (b -> a ->   b) -> b -> f a ->   b
foldM :: forall f m a b. Foldable f => Monad m => (a -> b -> m a) -> a -> f b -> m a

With change:

foldl :: forall     a b.                          (b -> a ->   b) -> b -> f a ->   b
foldM :: forall f m a b. Foldable f => Monad m => (b -> a -> m b) -> b -> f a -> m b

This also more closely matches haskell:

foldM :: (Foldable f, Monad m) => FoldM m a b -> f a -> m b 

@milesfrain
Copy link
Contributor Author

There are a few more foldMs to fix as listed by pursuit, but figured I should first get feedback on whether there's a reason for the current naming convention before making any additional PRs.

@garyb
Copy link
Member

garyb commented Oct 24, 2019

I think this is a good idea. It's a bit of an annoying breaking change since this library is so near the base of the dependency hierarchy, but it seems like the way it should have been done.

Maybe we should rename it to foldlM even to really drive home the point, with the potential for a foldrM when it's sensible too then.

@hdgarrood
Copy link
Contributor

As far as I can tell this isn't a breaking change - it's alpha-equivalent, right? And there's no way of inspecting type variable names except in error messages?

I'm not hugely keen on foldM -> foldlM, personally, given how low in the hierarchy this is. Also, would something need to change for foldrM to be sensible?

@garyb
Copy link
Member

garyb commented Oct 24, 2019

Hah, that'll teach me to pay attention. I thought it was reordering the argument for the fold step function. Given it's not breaking, yeah, I'd leave it as foldM.

I think foldrM is equally sensible to foldlM for a lot of types, maybe all? I meant "when it's sensible" as "for all types that having it could be useful" rather than something needing to change.

@milesfrain
Copy link
Contributor Author

I believe I found all the related foldM changes, and referenced this issue from all those other PRs.

Tried to stay consistent with variable names within the functions, although there are some subtle variations. For example b vs b0.

@michaelficarra
Copy link
Contributor

If anything, I'd say it's foldl that should have its ordering changed. I prefer to name my type variables in the order of their appearance.

@milesfrain
Copy link
Contributor Author

If anything, I'd say it's foldl that should have its ordering changed. I prefer to name my type variables in the order of their appearance.

That might make the differences between foldl and foldr less obvious.

Existing:

foldr :: (a -> b -> b) -> b -> f a -> b
foldl :: (b -> a -> b) -> b -> f a -> b

If foldl is changed for order of appearance:

foldr :: (a -> b -> b) -> b -> f a -> b
foldl :: (a -> b -> a) -> a -> f b -> a

@milesfrain
Copy link
Contributor Author

Maybe we should rename it to foldlM even

I recently misread the foldM docs as "Similar to 'fold'" instead of "Similar to 'foldl'", then was confused why the type of fold was so different.

Searching for foldlM proposals led back to this issue. So even though a breaking change wasn't the original goal of this PR, I think @garyb's renaming proposal is the right thing to do.

I'm not hugely keen on foldM -> foldlM, personally, given how low in the hierarchy this is.

Ideally, we'd have a process to let these breaking changes be as painless as possible.
Considering some ideas for this in purescript/registry#46

@JordanMartinez
Copy link
Contributor

Since we're making breaking changes now, I'm assuming this should be merged, correct? However, should a foldrM also be added?

@hdgarrood
Copy link
Contributor

This isn't a breaking change, but I think we should merge it now, regardless of whether we want to add a separate foldrM.

@JordanMartinez JordanMartinez merged commit ea905c3 into purescript:master Oct 15, 2020
@milesfrain milesfrain deleted the foldm-sig branch October 15, 2020 04:42
@hdgarrood hdgarrood mentioned this pull request Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants