zip/right does not skip over uneval nodes

[plexus]

rewrite-clj.zip/right skips over whitespace, comments, commas, newlines, but does not skip over uneval nodes.

I thought this was quite surprising. Is this intentional?

[plexus]

@xsc could you comment on this? would you consider accepting a PR that changes this behavior?

In Depot we now have our own versions of left/right/next all to deal with uneval nodes. Every time we accidentally use rewrite-clj.zip/right or next instead of our own version we run into problems. See https://github.com/Olical/depot/blob/master/src/depot/zip.clj

[lread]

Hi @plexus, since this could break things like code formatters, do you think it should it be an option when creating the zipper?

[plexus]

The first question is if it's a bug or a feature. If it's a bug then I think the right thing to do is to add a rewrite-clj.zip2 namespace with the new behavior, or add variants of left/right/next to rewrite-clj.zip with different names. That way you don't break existing consumers.

If it's a feature then we'll just continue to work around it outside of rewrite-clj

[sogaiu]

I would also prefer to have the behavior of uneval being skipped if comments are configured to be skipped.

On a related note, I'm investigating the idea of a zipper that doesn't skip comments but does skip whitespace. Is a general mechanism for tweaking / configuring what is skipped at zipper-creation time of any interest?

[borkdude]

What if one wanted to rewrite the uneval nodes?

[plexus]

What if one wanted to rewrite comments?

[borkdude]

The difference from a Clojure perspective between ;; and #_ is that #_ actually goes through the reader. Anything invalid in #_ will cause the reader to throw. E.g. #_{:a}. So technically it doesn't belong in the "whitespace" category of things that are plainly ignored by the reader.

[plexus]

Sure, but from a code semantics perspective it's as if these nodes don't exist. These rewrite-clj zipper operations are higher level operations over the lower level plain clojure zipper, that allow you to work with code at a more semantic level. If you care about more than that then go a level lower and use plain zipper operations.

If the zipper doesn't jump over uneval nodes then consumers need to check for them at every single step, thus creating wrapper functions for all zipper navigation functions. Every consumer will have to do this, and every consumer will at first not realize until they get bitten by this.

But... the ship has sailed at this point I think. This issue is 2.5 years old, and even back then this API probably already had too much real-world use to still tinker with the semantics. I don't think it makes sense to change it now for existing consumers, the only option would be to add an option or a parallel set of operations that handle these different cases. Maybe it would be possible to specify which behaviour you want when constructing the zipper initially.

[borkdude]

@plexus I was merely stating an observation, not an opinion. Adding an option to exclude certain types of nodes might be nice. Maybe this could be made more general than only :uneval type of nodes. Maybe a predicate of the node?

[lread]

Now that rewrite-clj has risen from its long slumber, I am excited that we will be able to actually do something here.

The idea voiced by @sogaiu & @plexus of specifying something at zipper creation makes sense to me.

And @borkdude's idea of a predicate on the node sounds interesting to me.

We'll have to decide if the predicate describes what you'll see or what you'll skip.
Maybe what you'll skip would more match the typical usage.

Another option is a predicate on the zloc instead of the node. This way you could decide to see/skip nodes based on their relative location.

The default predicate would match current rewrite-clj behavior, but we could provide predefined predicates, starting with one that skips uneval nodes too. If that would save users some typing/errors, not sure, would have to experiment.

The raw navigation fns (next*, right*, etc) would remain unaffected by this change, they will always navigate over all nodes.

[borkdude]

Thinking ahead, I think it might be best to add an option map, not just a predicate, to allow for more options.

{:skip-node? (fn [node] ...)} perhaps?

[lread]

Thinking ahead, I think it might be best to add an option map, not just a predicate, to allow for more options.

{:skip-node? (fn [node] ...)} perhaps?

Yes, good point, zipper creation fns already have an option map, we'd add our predicate to it.

[borkdude]

I ran into this myself in this issue: babashka/babashka#825
If we would support the above option, my solution would have become: :skip-node? (complement zip/sexpr-able?)

[lread]

Some old notes from me transcribed from Slack 2021-05-13:

Heya folks! I’m continuing to think about custom skip support for the rewrite-clj zipper.

I’m thinking a :skip-pred option at zipper creation time as suggested-ish in issue 70.
As those in the know know, the current skip behaviour is hardcoded to skip Clojure whitespace and comments.

The :skip-pred will allow folks to skip whatever nodes they like.
For example to skip all nodes that are not sexpr-able a :skip-pred might be set to:

 #(z/find % z/up* (complement z/sexpr-able?)

This says, I’m not interested in seeing nodes where the node or any of its parents are not sexpr-able.
This seems good to me, but am always happy to hear feedback.

Things get a little interesting if :skip-pred is very restrictive and makes me wonder what moving up and down might mean.

For example, if I want to skip everything but comments, there is no down on a comment, so fine, but up? I suppose it would always navigate to the zipper root node?

Or if we skip everything but a container node, let’s say a set, we would see all sets, but unless a set contained a set we would not otherwise see its children through navigation. If set1 contains a vector that contains set2, it seems to me that an up on set2 would bring me to set1. Would a down on set1 bring me to set2? I’m not sure yet, but these are the kinds of questions that are coming to mind.

[lread]

And from 2022-02-24 in response to a query:

The generalized solution was to skip nodes based on a user provided predicate. If I remember correctly, I was looking into what to do if all nodes ended up being skipped (and how that fits in with current implementation/behaviour), and how to handle a movement if the currently located inside a node that the predicate would skip.
I’ll dig up my old notes and update the issue.

See above. In short: a little more thinking is required.