Fork me on GitHub
Slack Archive

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

2020-12-05

Channels

#rewrite-clj
<
2020-12-05
>
lread16:12:43

@borkdude (and anyone else with an interest), I have been playing with an implementation of namespaced maps in rewrite-cljc based on our discussions. It is working splendidly in some aspects but I’m finding that it is not working well for the folks who want to traverse the parsed tree. These folks typically use a zipper and expect a node’s children to be a sequence. To illustrate, let’s have a look at the children of a rewrite-clj parsed map: 

(require '[rewrite-clj.parser :as p]
         '[rewrite-clj.node :as n])

(n/children (p/parse-string
                      "{
  :a 1
  :b 2}")) 
;; => (<newline: "\n"> <whitespace: "  "> <token: :a> <whitespace: " "> <token: 1> <newline: "\n"> <whitespace: "  "> <token: :b> <whitespace: " "> <token: 2>)
Asides: 1. This treatment of maps explains why rewrite-clj allows for an odd number of forms in maps. 2. I see why default node printing was overridden, it was for debugging, it is easy to see at-a-glance what child nodes we have. So what about namespaced maps? Here’s an example that includes whitespace: 
#:my-prefix ,
{ :a 1
  :b 2 }
Rewrite-cljc needs to capture and preserve the whitespace. We were thinking: - that we’d handle namespaced-maps and maps under a single MapNode. - the MapNode would store prefix and auto-resolved? as fields. This doesn’t match expectations of someone who is walking the tree. Someone who is walking the tree expects a to walk nodes that represent the source code as authored: - namespaced-map-node - children - nsmap-prefix-node - auto-resolved? - prefix - optionally whitespace-node(s) - map-node - children (which can include extra whitespace nodes) So we’ll probably need a NamespacedMapNode after all. I understand this representation is less convenient for folks interested solely in parsing, especially if they have no interest in whitespace. - they will have to grab the first node to get the nsmap-prefix-node and the last node to get the map-node - or they can use a zipper that, by default, skips whitespace nodes. - a map node and namespaced-map-node are different and retrieval of info from them is different. I’ll wait for feedback before taking my next implementation stab.

borkdude16:12:29

Ah that makes sense. I have the rewriting perspective less than the parsing perspective.

borkdude16:12:45

although I also use rewriting

borkdude16:12:26

Also if you feel strongly about the dynamic var in sexpr don't take my opinion on it as absolute. In some other contexts it may be more convenient than having to pass it args. Trade-offs.

lread16:12:10

Thanks for the feedback @borkdude, much appreciated. > Also if you feel strongly about the dynamic var in sexpr don’t take my opinion on it as absolute. In some other contexts it may be more convenient than having to pass it args. Trade-offs. I’ve switched to passing in the namespace auto-resolve fn as in an optional opts arg where it is needed. It works ok for the node api, but a can be a bit awkward for some existing zip api functions (at least one is variadic). I’m thinking in the case of the zip api, I’ll experiment holding the opts in zipper itself.

borkdude18:12:18

@lee I now implemented clj-kondo linting inside defrecord and deftype, this should help with rewrite-clj hopefully.

borkdude18:12:37

It's on master, but you can try a binary from the builds if you'd like

lread19:12:51

Awesome @borkdude, thanks! I will give it a try!

borkdude19:12:12

$ clj-kondo --lint src
src/rewrite_cljc/node/keyword.cljc:23:12: warning: unused binding this
src/rewrite_cljc/node/namespaced_map.cljc:48:9: warning: unused binding this
src/rewrite_cljc/node/namespaced_map.cljc:51:11: warning: unused binding this
src/rewrite_cljc/node/namespaced_map.cljc:62:12: warning: unused binding this
src/rewrite_cljc/node/reader_macro.cljc:70:11: warning: unused binding this
src/rewrite_cljc/node/seq.cljc:15:9: warning: unused binding this
src/rewrite_cljc/node/seq.cljc:18:11: warning: unused binding this
src/rewrite_cljc/node/seq.cljc:22:12: warning: unused binding this

borkdude19:12:59

seems to work :)