fmap API that accepts an arbitrary walk does not use function f

[gaurav-arya]

In the signature fmap(walk::AbstractWalk, f, x, ys...) in the public API, the function f is ultimately not used. Arguably, since this API does not use f, it should be deprecated and replaced by something that is not called fmap (which should be reserved for the special case of an ExcludeWalk where f is applied).

However, these changes should be made with caution due to potentially breaking effects. See the body of PR of #61 for some additional info.

Edit: it's possible #32 helps with a lot of this

[ToucheSir]

I don't remember the exact decision behind this, but @darsnack might.

[darsnack]

That's looks like leftover from before AnonymousWalk but I can't see the commits due to a rebase. Either way, I agree that f should be removed at the least. Probably making the future API "walk-first" by calling the walk directly is better. Calling it a map seems wrong.

[gaurav-arya]

To me, renaming the function to recursivewalk (with the f removed), changing the standard fmap as well as fcollect to call it , and deprecating the fmap with first-arg walk sounds like a very appealing plan and also easy to do without touching the rest of Functors. The name recursivewalk is appealing to me because it turns an arbitrary walk which takes in any recurse function into a walk function which enforces self-recursion.

I'd be interested to hear what other's thoughts are though, because if we make a breaking change let's make sure we're at least doing it right:) also unaware whether or not people have actually been using this API in practice

[ToucheSir]

In my mind and APIs in other programming languages, walk already implies recursive. We do muddy the waters a bit in Functors by turning it into a noun, but if anything that's more reason to not use it for function names.

[gaurav-arya]

Since all the walks are callable structs I think it's fair to call it a verb? (Except with the dispatch hidden as the 0th arg, so hm maybe that's a noun :)

So I guess the first question is if we want to keep the (walk::AbstractWalk)(recurse, xs...) API? I.e. walk a specific node but with an arbitrary recursive function for the children. I was sort of working off that assumption, in which context making the recursion-enforcing version be recursivewalk made some sense (but yeah, I see the verb vs noun inconsistency now...)

[gaurav-arya]

Can I ask how #32 fits into all this?

[darsnack]

Taking in a recurse function separately is what makes the various walks composable.

[gaurav-arya]

I agree, I just thought that there might be other ways of doing the same thing that don't require #61, by replacing recurse with an outer_walk that isn't a fancy closure and is by default equal to the walk itself. This would enforce recursion in the usual walk API with the callable structs, I think.

But I think the current approach is also fine and pretty readable, and more flexible in case that flexibility is somehow needed in the future -- just mentioned it due to @ToucheSir 's comment that walks should enforce self-recursion. (Although actually, do we think this flexibility of allowing something other than recursion is unnecessary?)

What do we think of the general approach of keeping walks.jl as is, deprecating the fmap signature and replacing its functionality with an appropriate verb? (dowalk, runwalk, etc.?)

[gaurav-arya]

Sigh, nerd-sniped myself into doing a prototype of the idea for getting rid of the recurse construction: #63

[gaurav-arya]

Okay, empirical evidence suggests that there does not exists a non-breaking solution beyond keeping the current fancy recurse closure, and just renaming the function something like runwalk and deprecating the old one.

[darsnack]

Yeah storing the outer walk in a field is something I considered in #43, but I decided against it. It can result in silent errors if the walk isn't constructed properly. Part of the usability of fmap(walk, ...) (or recursivewalk(walk, ...)) is that we define the recursion for you so that it works correctly.

[gaurav-arya]

It was really close to non-breaking to simply set walk(x, ys...) to recurse to itself by default, i.e. recurse=walk by default, which didn't require extra fields. Sadly, this broke dispatch for multiple Functors since walk(x, y) was erroneously dispatched to walk(recurse, x), and it would be breaking to require custom walks to annotate recurse::AbstractWalk. So I went with a separate function runwalk for the recurse on itself functionality.