clj-commons / rewrite-clj

Rewrite Clojure code and edn
https://cljdoc.org/d/rewrite-clj
MIT License
590 stars 55 forks source link

A function to move to top i.e. position [1,1] or a custom position in the zipper #149

Open mjayprateek opened 3 years ago

mjayprateek commented 3 years ago

Problem/Opportunity At times we need to automate changes which requires us to repeatedly (inside code) search for values and make changes. Now while doing that operation on config.edn (or for any other type), after first search, once the zipper location is inside a deeply nested map, there's a problem in searching for brothers of current-location's grandfather or great-grandfather.

This is tough when you don't even know at what level (grandfather or great-grandfather or above that) is the next search item located.

Proposed Solution There should be a method in zip API to either:

  1. a method to move the zipper to top so that I don't have to do that manually for the next search
  2. OR a method to move the zipper to a custom position, say [2, 13]. Though, I'm not sure how useful this would be
  3. OR a find function which lets you search from the beginning instead of the current location

Alternative Solutions I've written my own function to go up continuously till a certain key is found. I begin my successive searches from there on.

Additional context Let me know if you need some concrete examples.

Action I'm happy to contribute a PR with whatever solution the maintainers agree on.

mjayprateek commented 3 years ago

For example in this config.edn file: https://github.com/gojek/ziggurat/blob/master/resources/config.test.edn,

I want to change bootstrap-servers for all the maps inside :stream-routes

so as a first step, I search for :using-string-serde (Please note that I've already collected a set of keys inside :stream-routes as a pre-step and I'm iterating over this set) and change the bootstrap server there.

But, after this when I search for :default (the next key in the set of keys), I need a way to go to top or move to stream-routes.

With current set of APIs, I'll have to write my own function to move to top or to stream-routes so that my next search works again.

I hope I'm clear.

Pasting the above config.edn file here:

{:ziggurat {:app-name             "application_name"
            :env                  [:dev :keyword]
            :nrepl-server         {:port [7011 :int]}
            :datadog              {:host    "localhost"
                                   :port    [8126 :int]
                                   :enabled [false :bool]}
            :statsd               {:host    "localhost"
                                   :port    [8125 :int]
                                   :enabled [false :bool]}
            :metrics              {:constructor "ziggurat.dropwizard-metrics-wrapper/->DropwizardMetrics"}

            :alpha-features       {}
            :sentry               {:enabled                   [false :bool]
                                   :dsn                       "dummy"
                                   :worker-count              [10 :int]
                                   :queue-size                [10 :int]
                                   :thread-termination-wait-s [1 :int]}
            :rabbit-mq-connection {:host            "localhost"
                                   :port            [5672 :int]
                                   :username        "guest"
                                   :password        "guest"
                                   :channel-timeout [2000 :int]}
            :jobs                 {:instant {:worker-count   [4 :int]
                                             :prefetch-count [4 :int]}}
            :rabbit-mq            {:delay       {:queue-name           "application_name_delay_queue_test"
                                                 :exchange-name        "application_name_delay_exchange_test"
                                                 :dead-letter-exchange "application_name_instant_exchange_test"
                                                 :queue-timeout-ms     [100 :int]}
                                   :instant     {:queue-name    "application_name_instant_queue_test"
                                                 :exchange-name "application_name_instant_exchange_test"}
                                   :dead-letter {:queue-name    "application_name_dead_letter_queue_test"
                                                 :exchange-name "application_name_dead_letter_exchange_test"}}
            :retry                {:count   [5 :int]
                                   :type    [:linear :keyword]
                                   :enabled [true :bool]}
            :http-server          {:middlewares  {:swagger {:enabled false}}
                                   :port         [8010 :int]
                                   :thread-count [100 :int]}
            :stream-router        {:default            {:application-id                     "test"
                                                        :bootstrap-servers                  "localhost:9092"
                                                        :stream-threads-count               [1 :int]
                                                        :origin-topic                       "topic"
                                                        :consumer-type                      :default
                                                        :changelog-topic-replication-factor [1 :int]
                                                        :channels                           {:channel-1 {:worker-count [10 :int]
                                                                                                         :retry        {:type    [:linear :keyword]
                                                                                                                        :count   [5 :int]
                                                                                                                        :enabled [true :bool]}}}
                                                        :producer                           {:bootstrap-servers                     "localhost:9092"
                                                                                             :acks                                  "all"
                                                                                             :retries                               5
                                                                                             :max-in-flight-requests-per-connection 5
                                                                                             :enable-idempotence                    false
                                                                                             :value-serializer-class                "org.apache.kafka.common.serialization.StringSerializer"
                                                                                             :key-serializer-class                  "org.apache.kafka.common.serialization.StringSerializer"}}
                                   :using-string-serde {:application-id              "test"
                                                        :bootstrap-servers           "localhost:9092"
                                                        :stream-threads-count        [1 :int]
                                                        :origin-topic                "another-test-topic"
                                                        :default-key-serde           "org.apache.kafka.common.serialization.Serdes$StringSerde"
                                                        :default-value-serde         "org.apache.kafka.common.serialization.Serdes$StringSerde"
                                                        :key-deserializer-encoding   "UTF8"
                                                        :value-deserializer-encoding "UTF8"
                                                        :deserializer-encoding       "UTF8"
                                                        :channels                    {:channel-1 {:worker-count [10 :int]
                                                                                                  :retry        {:count   [5 :int]
                                                                                                                 :enabled [true :bool]}}}}}
            :batch-routes          {:consumer-1 {:consumer-group-id                 "test-consumer-1002"
                                                 :bootstrap-servers                 "localhost:9092"
                                                 :max-poll-records                  [1000 :int]
                                                 :origin-topic                      "topic"
                                                 :commit-interval-ms                [5000 :int]
                                                 :poll-timeout-ms-config            [1000 :int]
                                                 :thread-count                      [2 :int]
                                                 :session-timeout-ms-config         [60000 :int]
                                                 :default-api-timeout-ms-config     [60000 :int]
                                                 :key-deserializer-class-config     "org.apache.kafka.common.serialization.ByteArrayDeserializer"
                                                 :value-deserializer-class-config   "org.apache.kafka.common.serialization.ByteArrayDeserializer"}
                                    :consumer-2 {:consumer-group-id                 "test-consumer-2002"
                                                 :bootstrap-servers                 "localhost:9092"
                                                 :max-poll-records                  [2000 :int]
                                                 :origin-topic                      "topic"
                                                 :poll-timeout-ms-config            [1000 :int]
                                                 :thread-count                      [4 :int]
                                                 :session-timeout-ms-config         [60000 :int]
                                                 :default-api-timeout-ms-config     [60000 :int]
                                                 :key-deserializer-class-config     "org.apache.kafka.common.serialization.ByteArrayDeserializer"
                                                 :value-deserializer-class-config   "org.apache.kafka.common.serialization.ByteArrayDeserializer"}}
            :tracer               {:enabled         [true :bool]
                                   :custom-provider ""}
            :new-relic            {:report-errors false}}}
lread commented 3 years ago

Sorry for being slow to reply @mjayprateek, I've been out with a small injury.

I haven't yet reviewed your issue carefully, but I think adding a top function to the zip API makes sense. The notion is buried in #125.

I'll think about nuances and follow up when I am back to the keyboard in full capacity, which shouldn't be too long.

mjayprateek commented 3 years ago

Sure @lread. There's no hurry here. Please take Care.

lread commented 3 years ago

Hi @mjayprateek I don't know if you are still feeling a pain here.

Have you had a look at docs on sub editing? I wonder if they might scratch your itch. Sub editing allows you to restrict your changes to an isolated sub-tree.

Here's one way to update all :boostrap-servers values, but only under :stream-router's value using postwalk:

(require '[rewrite-clj.zip :as z])

(-> (slurp "https://raw.githubusercontent.com/gojek/ziggurat/c4c6d813dc164c3e7b284fb63f43f9a602c0fd55/resources/config.test.edn")
    z/of-string
    (z/find-value z/next :stream-router)
    z/right
    (z/postwalk (fn [zloc]
                  (when (= (-> zloc z/left z/sexpr) :bootstrap-servers)
                    (z/replace zloc "my-new-value"))))
    z/print-root)
mjayprateek commented 2 years ago

Hi @lread The problem which I generally face is when I'm trying to find a value.

For example, we're using rewrite-clj to automatically upgrade user codebases which use Ziggurat (the library mentioned above)

For this the rewriter code goes inside certain parts of a file and when it encouters a var, it tries look up its definition. At this point my zipper is deep inside some function call and I want to start looking for a var from the top.

At present, the way we do is we navigate to the top and just search for all defs.

I hope that helps.

Below is a sample of code we're using (might not be entirely correct but works for us.)

(defn move-to-top
  "Creates a zloc which is located at the top of the code (at the point where ns is defined)"
  [zipper-loc]
  (loop [zloc zipper-loc]
    (if (some? (z/up zloc))
      (recur (z/up zloc))
      (z/down zloc))))
lread commented 2 years ago

Thanks for the clarification @mjayprateek.

Because you aren't navigating to the ultimate top of the zipper (the :forms root node), I wonder if top might not be a great name for this new proposed function. I think you really want to move to the first non whitespace node, which is where you are automatically navigated to when creating a zipper.

If we add such a function, what to name it? I wonder if first might be a good name.
Would that name intuit its behaviour do you think?

If we do add a top, I'd be inclined to have move to the absolute root node.

mjayprateek commented 2 years ago

Hi @lread Yes, first sounds better but my only concern is that people might confuse it with the leftmost element of current zloc. But, not a big concern.

If top moves to absolute top, that should be totally fine since we can always navigate to first non-whitespace node easily.

mrkam2 commented 5 months ago

Maybe root rather than top? Also one helper function could be to return the current depth?

lread commented 5 months ago

Hi @mrkam2! There is already a root that does a different thing.

I've hand-coded depth functions a few times in the past. Could be a useful addition.

mrkam2 commented 5 months ago

Maybe go-root or root-zipper. The fact that some functions return nodes and some zippers (and sometimes other things) without much differences in their names, is also tricky sometimes. But anyways, having something to go all the way up should be a nice addition, or up until a certain depth level might be a good addition.

lread commented 5 months ago

I agree, I don't think I would have chosen the root naming, but the precedent is the clojure.zip API, and that's the name it chose.

The root naming implies a conversion and completion:

Because of this, I'd probably not reuse root in a fn name for navigation alone.

mrkam2 commented 5 months ago

Is doing (-> zloc z/root z/of-node) not an efficient strategy to get to the top? Does it have any disadvantages over the proposed top function?

lread commented 5 months ago

Hi @mrkam2! Let's see in a REPL:

(require '[rewrite-clj.zip :as z])

(def s "  [1 2 3]")

(def zloc (z/of-string s))

;; we are auto-navigated to the first non-whitespace node
(z/tag zloc)
;; => :vector
(z/string zloc)
;; => "[1 2 3]"

;; but the the absolute top node is the :forms node
(z/tag (z/up zloc))
;; => :forms
(z/string (z/up zloc))
;; => "  [1 2 3]"

;; so if you want to return to the first non-whitespace node, your technique should work fine:
(-> zloc
    z/down z/rightmost ;; some nav
    z/root
    z/of-node
    z/tag) ;; where are we now?
;; => :vector

;; or if you want to navigate to the topmost node you might try:
(-> zloc
    z/down z/rightmost ;; some nav
    z/root
    z/of-node*
    z/tag) ;; where are we now?
;; => :forms