Rename scanrLazy to scanlLazy and fix arg order

[drewolson]

The previously-named scanrLazy was actually folding to the left. I've
renamed the function and fixed the argument ordering for the folding
function.

Fixes #159

[hdgarrood]

As it stands this will be a breaking change, which is less than ideal. Can we add a scanrLazy which is actually a lazy right scan with the same type signature? We might be able to release this as a patch-level release that way.

[drewolson]

The function scanr from Haskell[1] has some interesting semantics -- specifically, that the first item of the scan is the result of the entire right fold. In order to implement this in a sane way, we'd probably have to introduce a Z.Lazy type class constraint to the accumulator. This would also be backwards incompatible, right?

It's entirely possible that I'm wrong here and that there is a better way to implement this function.

[1] - https://hackage.haskell.org/package/base-4.12.0.0/docs/Prelude.html#v:scanr

[drewolson]

@hdgarrood I've added back scanrLazy in the way I think is the best we can do given the constraints of the existing signature. It's lazy until the first value from the scan is consumed, at which point the fold is run to completion. I think this is also how haskell's implementation works.

[drewolson]

Any thoughts on this PR?

[hdgarrood]

This seems okay to me 👍 I think it's languished a bit because we generally try to bunch breaking changes up together so that people don't have to deal with them too often, but since 0.14 is coming up soon we can hopefully get this change in there.

[hdgarrood]

Actually, can you add a test for stack safety? That is, run scanrLazy on a big but finite list, say, scanrLazy (\i _ -> i) (take 100000 (iterate (_ + 1) 1) and check that the head is equal to whatever we should expect it to be (100001?)

[drewolson]

@hdgarrood sounds good! I actually don't think this PR is backwards incompatible as it still includes a scanrLazy with the same signature (though different performance characteristics).

I'll add a stack safety test, though I'm somewhat concerned that it isn't actually stack safe.

[hdgarrood]

The behaviour of scanrLazy is completely different though, isn't it? I think this should be released as a breaking change on that basis.

[drewolson]

I've added the test at it indeed is not stack safe. I'll take a look soon into fixing this.

[hdgarrood]

The behaviour of scanrLazy is completely different though, isn't it? I think this should be released as a breaking change on that basis.

Oh, I just spotted that this contradicts what I said before. Ignore that older comment, I don't know what I was thinking, haha.

[drewolson]

I'm at a bit of a loss as to how to make scanrLazy stack safe, as it is monolithic. Considering this is a breaking release anyway, and scanrLazy seems basically useless in practice, should we simply remove it?

I highly doubt anyone is actually using it in the current state and, if they are, they simply need to reverse two arguments and rename the function.

Otherwise, if you have advice for making scanrLazy stack safe I'd be all ears.

[natefaubion]

Maybe reverse <<< scanlLazy ??? <<< reverse?

[drewolson]

@natefaubion that is genius and now seems so obvious :) I've updated the code and it seems to be stack safe.

[hdgarrood]

Ok so now that I actually understand what scanrLazy is doing after seeing it expressed in terms of scanlLazy I’m kind of skeptical of it. Does it even make sense to describe the function as “lazy” when simply getting the head of the result requires forcing the entire input list? It just seems like a weird thing to want to do. Perhaps it’s best to get rid of it after all? Especially since nobody has asked for it as far as I’m aware. (Sorry that I keep changing my mind.)

[natefaubion]

No, it doesn't really make sense. A right scan isn't very useful for this reason. It's similar to a lazy foldl.

[drewolson]

I agree. So considering it is a breaking release, should we simply remove it?

[hdgarrood]

Sounds good to me. Sorry for leading you round in a big circle to end up right back where you started!

[drewolson]

No problem! I've removed scanrLazy.

[JordanMartinez]

To clean up the commit history, should he just force-push the original commit?

[drewolson]

You can also just squash and merge.

…

On Sun, Feb 23, 2020 at 6:02 PM JordanMartinez ***@***.***> wrote: To clean up the commit history, should he just force-push the original commit? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#161?email_source=notifications&email_token=AAAA3UFHBXIGNS4GRPV3SCLREMFBTA5CNFSM4IL2KN6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMWLLHI#issuecomment-590132637>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAA3UBRUXDCVRHTOIOA2RLREMFBTANCNFSM4IL2KN6A> .

[drewolson]

I've squashed all commits down to a single commit.

[natefaubion]

If we don't want to maintain a scanrLazy, should we just call it scanLazy?

[hdgarrood]

I was going to suggest just scan actually. The laziness is implied by the fact that it's in Data.List.Lazy and the fact that it works on lazy lists.

[drewolson]

Happy to rename it to either. Please let me know what you two prefer.

[drewolson]

Note that scanl already exists, so consistency might be a concern as well.

[kl0tl]

If we drop the Lazy suffix should we also rename Data.List.Lazy.foldrLazy to foldr for consistency?

[JordanMartinez]

Yeah, we should probably go for consistency.

I don't like that there can be two functions named the same thing where the only difference is the module from which they are imported. Until one knows to check their imports, it can make reading compiler errors confusing. However, that's more of a beginner issue.

[hdgarrood]

I agree that consistency is good but I think avoiding unnecessary breaking changes generally trumps consistency. I didn't realise this module already has a foldrLazy, so I think the best course of action would be to keep the Lazy suffix so that we can leave foldrLazy alone and not introduce a consistency issue.

[hdgarrood]

So let's call it scanLazy? The existence of scanl in Foldable doesn't really bother me since that one isn't lazy. Also, we shouldn't be trying to make names unique across different modules (reuse of names is inevitable and, I'd argue, even a good thing if you're using qualified imports, which a lot of the time you should be). If errors are confusing, that's something we can address by pushing harder for people to use qualified imports more and/or by improving the compiler errors.

[kl0tl]

If we prefer to not rename foldrLazy then shouldn’t we keep scanlLazy and merge this as is?

[hdgarrood]

That works for me too, I guess. Is the argument that scanlLazy is preferable even if we don't intend to provide a scanrLazy because it remains consistent with the pattern of taking the Foldable version and adding a "Lazy" suffix?

[kl0tl]

Yes, exactly. Even though I’m not fond of the Lazy prefix since we tend to encourage disambiguation by qualified imports elsewhere in the core libraries, at least scanlLazy and foldrLazy remain consistent.