redplanetlabs / specter

Clojure(Script)'s missing piece
Apache License 2.0
2.52k stars 105 forks source link

stay-then-continue prevents transformation to NONE #274

Closed npetryk closed 5 years ago

npetryk commented 5 years ago

Hey again, Nathan.

I'm not sure if this is really issue, because it is a logical consequence of the pre-order traversal that stay-then-continue produces, but it may be worth calling out as a gotcha in the docs.

Basically, if you write a recursive navigator using stay-then-continue and then you attempt to setval or transform a non-leaf node to NONE, you end up changing the current node to a keyword, which will likely produce errors.

A simple (contrived) reproduction follows (Using specter 1.1.0)

(def FOLLOW-ALL-PTRS
  (specter/recursive-path
   [] p
   (specter/cond-path
    :ptr (specter/stay-then-continue (specter/must :ptr) p)
    vector? [specter/ALL p])))

(specter/setval
 [FOLLOW-ALL-PTRS]
 specter/NONE
 {:ptr [{:a 1 :ptr nil}
        {:b 2 :ptr nil}]})

which produces

CompilerException java.lang.IllegalArgumentException: contains? not supported on type: clojure.lang.Keyword

This can be surprising, especially if you dont know that specter/NONE is a keyword and you are used to it sort of being a "magic" value.

I may not be thinking correctly about this, but it seems that stay-then-continue breaking with specter/NONE would be fairly common unless the recursive navigator was setup to handle keywords specifically, but again I could be wrong.

Think this warrants a note in the docs?

npetryk commented 5 years ago

Now that I've written it, the expected thing here would be to stop recursion the moment you see structure transformed into a NONE, but I'm not sure if that's possible

nathanmarz commented 5 years ago

The behavior is fine, but probably worth adding a little more to the docs about NONE at https://github.com/nathanmarz/specter/wiki/List-of-Navigators#none Feel free to make a pull request to https://github.com/nathanmarz/specter-wiki