kkinnear / zprint

Executables, uberjar, and library to beautifully format Clojure and Clojurescript source code and s-expressions.
MIT License
544 stars 47 forks source link

Option for `:rod` to not add newline after arglist when whole defn can fit into single line #282

Open yqrashawn opened 1 year ago

yqrashawn commented 1 year ago

No guide but fn-style is :guide for this sequence: ["rf/defn"....

The code is at https://github.com/status-im/status-mobile/blob/8c0e18899edf4995f47214215f0c8f6f02a70c08/src/status_im2/setup/log.cljs#L36

(rf/defn set-log-level
  [{:keys [db]} log-level]
  (let [log-level (or log-level config/log-level)]
    {:db             (assoc-in db [:multiaccount :log-level] log-level)
     :logs/set-level log-level}))

This clojure.core/defn in macro also cause the same error https://github.com/status-im/status-mobile/blob/8c0e18899edf4995f47214215f0c8f6f02a70c08/src/utils/re_frame.clj#L53

Are there any way to ignore these defn?

kkinnear commented 1 year ago

My apologies. I upgraded :style :rod with a better implementation, but neglected to similarly upgrade :style :rod-no-ma-nl when I did so. Without too much trouble, you can change your options map to do the same using 1.2.4.

One approach would be to make the definition for :style :rod-ma-no-nl be the same as the definition for :style :rod, but have the :multi-arity-nl? value be false instead of true. You could define :rod-ma-no-nl in the :style-map that way.

Alternatively, you could just directly modify the :fn-map for "defn", like this:

projects/zprint % ./zprintm-1.2.4 '{:fn-map {"defn" [:none {:list {:option-fn (partial rodfn {:multi-arity-nl? false})}}]}}' <cljfiles/i282.clj
(rf/defn set-log-level [{:keys [db]} log-level]
  (let [log-level (or log-level config/log-level)]
    {:db (assoc-in db [:multiaccount :log-level] log-level),
     :logs/set-level log-level}))

You might want to also do this for defn- if you have them, but if you don't use defn-, what I did above should solve this problem until I release a new definition for :rod-no-ma-nl.

I haven't actually had time to dig into rodguide and figure out why it isn't working, but rodfn was already the more robust of the two implementations, so using that isn't a problem. When I do the next release, I will make the changes to the :style-map that I suggested above, and you will be able to stop doing it explicitly in your own options map.

Thanks for noticing this! I will also put in some more tests for :rod-no-ma-nl to hopefully keep this from happening again.

yqrashawn commented 1 year ago

Hi, I tried what you said about rodfn before, it works except it format all the single line functions into multi line

;; before
(defn easing-in [] (.-in ^js easing))
(defn easing-out [] (.-out ^js easing))

;; after
(defn easing-in []
  (.-in ^js easing))
(defn easing-out []
  (.-out ^js easing))
kkinnear commented 1 year ago

Well, the :rod styles (where 'rod' stands for "rules of defn", for wha that's worth) resulted from Issue #170. I created this style to meet a particular need and it is, therefore, very particular. That doesn't mean that other people don't use it, because in that same issue someone liked the style, but didn't want blank lines between rarities. So that was the reason for :rod-no-ma-nl, where there aren't any newlines between arities. But the person who defined what became the :rod styles said this:

The overall philosophy is to give to certain elements a fixed implication of a newline (e.g. 1 arity = 1 newline), completely disregarding whether something would fit in one line.

Now, I'm perfectly fine in extending the basic :rod style to be more configurable, and so I expect that I can extend the implementation of :rod... to allow single arity functions that would fit on one line to actually fit on one line. I will put that high on the list of things to do for the next release.

Presently I'm still deep in trying to get the justification working right for Issue #273 (which has been quite the complex process, as it happens). Once I get that finished, and then finish up another issue that I was in the middle of when I started on #273, I want to push out the release. But I will get to this in the next release which I imagine to be 1.2.6, though the changes for 1.2.5 are substantial enough that I'm contemplating making it 1.3. But we'll see. Anyway, thanks for bringing this up.

yqrashawn commented 1 year ago

Thanks for the detailed reply and your time into this.

kkinnear commented 1 year ago

The failure (which you have already handled) is now fixed in 1.2.5. The additional options are not yet available.

yqrashawn commented 1 year ago

Thanks. Can confirm there's no more error in 1.2.5. Do you want me to close this issue and open another one for the options, or just change the title of this issue? @kkinnear

kkinnear commented 1 year ago

Thanks for the update. No need for another issue, this one is fine. Changing the title would be nice but not a big deal.

kkinnear commented 1 year ago

To support this, I implemented a new capability internally: :one-line-ok? true for guides (which :rod uses). I also implemented an entirely new capability -- "configurable styles", to support this request. Not because this request was so big, but because each request like this was causing a new style to have to be created in the style-map. The current approach "didn't scale", as they say.

So, to get what you want, you will invoke the style :rod-config, using a map instead of just a keyword for :rod-config. For example:

; Today's approach, which isn't what you want

zprint.core=> (czprint i282a {:parse-string? true :style :rod-no-ma-nl})
(defn easing-in []
  (.-in ^js easing))

; This will get you what you want

zprint.core=> (czprint i282a {:parse-string? true :style {:style-call :rod-config :one-line-ok? true}})
(defn easing-in [] (.-in ^js easing))

; And it will still do the rest of the `:rod` stuff the way it does now.

zprint.core=> (czprint i282 {:parse-string? true :style {:style-call :rod-config :one-line-ok? true}})
(rf/defn set-log-level [{:keys [db]} log-level]
  (let [log-level (or log-level config/log-level)]
    {:db (assoc-in db [:multiaccount :log-level] log-level),
     :logs/set-level log-level}))

I'm sorry it took a long time to work through the configurable styles, but I think it will pay off over time.

Thanks for pointing out the problem with :rod-no-ma-nl as well as asking for the "do it on one line if you can" feature in :rod.

yqrashawn commented 7 months ago

Hi @kkinnear, thanks for the detailed explanation, and please excuse my delayed response. I tried the new :rod-config You can see in the diff, seems the :multi-arity-nl? false option doesn't work. I added the async_storage.cljs file below

diff --git a/.zprintrc b/.zprintrc
index 4ff4c9001f..e59374ef0c 100644
--- a/.zprintrc
+++ b/.zprintrc
@@ -1,6 +1,4 @@
 ;; -*- mode: clojure -*-
 ;; vi: ft=clojure
 {:style [:community
-         ;; {:multi-arity-nl? false, :one-line-ok? true, :style-call
-         ;; :rod-config}
-        ]}
+         {:multi-arity-nl? false, :one-line-ok? true, :style-call :rod-config}]}
diff --git a/src/react_native/async_storage.cljs b/src/react_native/async_storage.cljs
index 067e0a9ea5..7c5a6fe3c1 100644
--- a/src/react_native/async_storage.cljs
+++ b/src/react_native/async_storage.cljs
@@ -10,20 +10,19 @@
 (def ^:private writer (transit/writer :json))

 (defn clj->transit [o] (transit/write writer o))
-(defn transit->clj
-  [o]
+(defn transit->clj [o]
   (try (transit/read reader o) (catch :default e (log/error e))))

 (defn set-item!
-  ([k value] (set-item! k value identity))
+  ([k value]
+   (set-item! k value identity))
   ([k value cb]
    (-> ^js async-storage
        (.setItem (str k) (clj->transit value))
        (.then (fn [_] (cb)))
        (.catch (fn [error] (log/error "[async-storage]" error))))))

-(defn set-item-factory
-  []
+(defn set-item-factory []
   (let [tmp-storage (atom {})
         debounced (goog.functions/debounce (fn []
                                              (doseq [[k v] @tmp-storage]
@@ -32,8 +31,7 @@
                                            debounce-ms)]
     (fn [items] (swap! tmp-storage merge items) (debounced))))

-(defn get-items
-  [ks cb]
+(defn get-items [ks cb]
   (-> ^js async-storage
       (.multiGet (to-array (map str ks)))
       (.then (fn [^js data]
@@ -42,8 +40,7 @@
                         (zipmap ks)))))
       (.catch (fn [error] (cb nil) (log/error "[async-storage]" error)))))

-(defn get-item
-  [k cb]
+(defn get-item [k cb]
   (-> ^js async-storage
       (.getItem (str k))
       (.then (fn [^js data]

async_storage.cljs

(ns react-native.async-storage
  (:require
    ["@react-native-async-storage/async-storage" :default async-storage]
    [cognitect.transit :as transit]
    goog.functions
    [taoensso.timbre :as log]))

(def ^:private debounce-ms 250)

(def ^:private reader (transit/reader :json))
(def ^:private writer (transit/writer :json))

(defn clj->transit [o] (transit/write writer o))
(defn transit->clj
  [o]
  (try (transit/read reader o)
       (catch :default e
         (log/error e))))

(defn set-item!
  ([k value] (set-item! k value identity))
  ([k value cb]
   (-> ^js async-storage
       (.setItem (str k)
                 (clj->transit value))
       (.then (fn [_] (cb)))
       (.catch (fn [error]
                 (log/error "[async-storage]" error))))))

(defn set-item-factory
  []
  (let [tmp-storage (atom {})
        debounced   (goog.functions/debounce (fn []
                                               (doseq [[k v] @tmp-storage]
                                                 (swap! tmp-storage dissoc k)
                                                 (set-item! k v)))
                                             debounce-ms)]
    (fn [items]
      (swap! tmp-storage merge items)
      (debounced))))

(defn get-items
  [ks cb]
  (-> ^js async-storage
      (.multiGet (to-array (map str ks)))
      (.then (fn [^js data]
               (cb (->> (js->clj data)
                        (map (comp transit->clj second))
                        (zipmap ks)))))
      (.catch (fn [error]
                (cb nil)
                (log/error "[async-storage]" error)))))

(defn get-item
  [k cb]
  (-> ^js async-storage
      (.getItem (str k))
      (.then (fn [^js data]
               (-> data
                   js->clj
                   transit->clj
                   cb)))
      (.catch (fn [error]
                (cb nil)
                (log/error "[async-storage]" error)))))
kkinnear commented 7 months ago

No worries about the time lag. I certainly understand.

So, I think we haven't fully communicated here. In that I didn't understand what you wanted, completely and I think that :multi-line-nl? false doesn't do what you think. For example:

(czprint i286a {:parse-string? true :style [:community {:multi-arity-nl? false :one-line-ok? true :style-call :rod-config}]})
(defn set-item!
  ([k value]
   (set-item! k value identity))
  ([k value cb]
   (-> ^js async-storage
       (.setItem (str k) (clj->transit value))
       (.then (fn [_] (cb)))
       (.catch (fn [error] (log/error "[async-storage]" error))))))
nil
zprint.core=> (czprint i286a {:parse-string? true :style [:community {:multi-arity-nl? true :one-line-ok? true :style-call :rod-config}]})
(defn set-item!
  ([k value]
   (set-item! k value identity))

  ([k value cb]
   (-> ^js async-storage
       (.setItem (str k) (clj->transit value))
       (.then (fn [_] (cb)))
       (.catch (fn [error] (log/error "[async-storage]" error))))))

From this you can see that :multi-line-nl? true puts a blank line between arities, which you don't want. But nothing, yet, will put the first arity of the above example all on one line. You might think that :one-line-ok? true would do that, but it doesn't. I think (without yet looking) that I can probably get it to do so. But what it does is put an entire function definition on one line, if it fits. Thus:

 (czprint i286b {:parse-string? true :style [:community {:multi-arity-nl? false :one-line-ok? true :style-call :rod-config}]})
(defn clj->transit [o] (transit/write writer o))

So, let me see what I can do with individual arities to make them one-line if they will fit, in addition to entire function definitions.

Thanks for bringing this up!

kkinnear commented 7 months ago

I am so confused!!! I thought I knew what you wanted and I implemented it, but when I looked in detail at all of the functions in async_storage.cljs I can't tell if that file respresents what you want the results to look like, or not.

As it happens, it was easy to add what I thought you wanted -- that if :one-line-ok? is true, then you also want individual arities that will format onto a single line to actually format onto a single line. If that is what you wanted, then I've done it.

To make sure I'm doing what you request, here is the file you included. Did it come out the way you want? I'm guessing that it didn't, but I sure don't know what you do want.

% java -jar target/zprint-filter-1.2.9 '{:style [:community {:multi-arity-nl? false :one-line-ok? true :style-call :rod-config}]}'  <cljfiles/async_storage.cljs
(ns react-native.async-storage
  (:require ["@react-native-async-storage/async-storage" :default async-storage]
            [cognitect.transit :as transit]
            goog.functions
            [taoensso.timbre :as log]))

(def ^:private debounce-ms 250)

(def ^:private reader (transit/reader :json))
(def ^:private writer (transit/writer :json))

(defn clj->transit [o] (transit/write writer o))
(defn transit->clj [o]
  (try (transit/read reader o) (catch :default e (log/error e))))

(defn set-item!
  ([k value] (set-item! k value identity))
  ([k value cb]
   (-> ^js async-storage
       (.setItem (str k) (clj->transit value))
       (.then (fn [_] (cb)))
       (.catch (fn [error] (log/error "[async-storage]" error))))))

(defn set-item-factory []
  (let [tmp-storage (atom {})
        debounced (goog.functions/debounce (fn []
                                             (doseq [[k v] @tmp-storage]
                                               (swap! tmp-storage dissoc k)
                                               (set-item! k v)))
                                           debounce-ms)]
    (fn [items] (swap! tmp-storage merge items) (debounced))))

(defn get-items [ks cb]
  (-> ^js async-storage
      (.multiGet (to-array (map str ks)))
      (.then (fn [^js data]
               (cb (->> (js->clj data)
                        (map (comp transit->clj second))
                        (zipmap ks)))))
      (.catch (fn [error] (cb nil) (log/error "[async-storage]" error)))))

(defn get-item [k cb]
  (-> ^js async-storage
      (.getItem (str k))
      (.then (fn [^js data]
               (-> data
                   js->clj
                   transit->clj
                   cb)))
      (.catch (fn [error] (cb nil) (log/error "[async-storage]" error)))))

If this file isn't formatted the way you want, please hand edit this text until it looks like you want it to look and show it to me, and I'll try to figure out what I have to do to get it to look that way. I'm not doing all that well with the git diff approach, since I don't know what is right or wrong from your viewpoint in the diff.

For instance, the file you gave me has a number of places where things that could fit on one line do not. It is not all that hard to make that happen in zprint for certain functions if that is what you really want. I'll show you how once I know what you are actually looking for. Thanks.

yqrashawn commented 7 months ago

Truly sorry for confusing you, I feel awful doing that.

The rest of the diff are all fine. It's the below part what I don't want, since I specified :multi-arity-nl? false and ([k value] (set-item! k value identity)) still got formatted into multiline even though it can fit into a single line.

 (defn set-item!
-  ([k value] (set-item! k value identity))
+  ([k value]
+   (set-item! k value identity))
   ([k value cb]
    (-> ^js async-storage
        (.setItem (str k) (clj->transit value))
        (.then (fn [_] (cb)))
        (.catch (fn [error] (log/error "[async-storage]" error))))))
kkinnear commented 7 months ago

Great. That is the part I fixed:

(czprint i286a {:parse-string? true :style [:community {:multi-arity-nl? false :one-line-ok? true :style-call :rod-config}]})
(defn set-item!
  ([k value] (set-item! k value identity))
  ([k value cb]
   (-> ^js async-storage
       (.setItem (str k) (clj->transit value))
       (.then (fn [_] (cb)))
       (.catch (fn [error] (log/error "[async-storage]" error))))))

It will be in 1.2.9, which I hope will be released in early December.

There will be no configuration change required -- I extended :one-line-ok? to also handle this case.

Just to be pedantic, it is worth noting that the problem you noted had nothing to do with :multi-arity-nl?. What:multi-arity-nl? true does is this:

zprint.core=> (czprint i286a {:parse-string? true :style [:community {:multi-arity-nl? true :one-line-ok? true :style-call :rod-config}]})
(defn set-item!
  ([k value] (set-item! k value identity))

  ([k value cb]
   (-> ^js async-storage
       (.setItem (str k) (clj->transit value))
       (.then (fn [_] (cb)))
       (.catch (fn [error] (log/error "[async-storage]" error))))))

It puts a blank line between arities. It has nothing to do with whether or not something formats on a single line or not. That is controlled by :one-line-ok?, just to be clear. That said, you certainly want to keep :multi-arity-nl? false, because you don't want that blank line. Few do, actually.

I'll let you know when 1.2.9 releases. Thanks again for pursuing this.

yqrashawn commented 7 months ago

What:multi-arity-nl? true does is this:

Ah, now I get it. Thank you for your work and quick response.

kkinnear commented 3 months ago

So much for December. I fixed this in 1.2.9, and I just released it.