kkinnear / zprint

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

List indentation applied to defprotocol and defrecord #176

Closed jcf closed 3 years ago

jcf commented 3 years ago

Hi Kim,

I hope this issue finds you well! Thanks for the awesome library, and for all the hardwork you put into not just building but supporting everyone who uses it.

I'm working on a project where I'd love to have a fully-fledged .zprint.edn that will handle almost anything we throw at it so I'm working on documenting use of new and old configuration options as well as adding some rules for popular libraries in he Clojure ecosystem. In the process of doing so I've encountered something odd that feels like a bug but I'm not sure.

Given the following ~/.zprint.edn:

{:search-config? true
 :width          80}

And the following $PWD/.zprint.edn:

{:map   {:comma? false :sort-in-code? true :sort? true}
 :style [:binding-nl :community :extend-nl :hiccup :how-to-ns :map-nl :pair-nl]
 :width 80}

I'm seeing zprint format code like so:

(ns fmt
  (:require
   [com.stuartsierra.component :as component]
   [clojure.string :as str]))

(defprotocol IThing
 (thing [this]))

(defrecord App [db]
 component/Lifecycle
 (start [this] (assoc this :db (atom {::fmt "please"})))
 (stop [this] (dissoc this :db))

 IThing
 (thing [this] (:db this)))

Ideally the namespace would be sorted but I see in the docs that the :how-to-ns style doesn't sort things. What's truly perplexing is why both the defprotocol and defrecord have a single space rather than two.

I'm invoking zprint like so:

zprint "$(<../.zprint.edn)" <fmt.clj

This is with brew install --cask zprint and version 1.1.1 on macOS.

Am I misunderstanding the set of options I'm using or maybe missing something?

jcf commented 3 years ago

Removing some of the styles help.

{:map   {:comma? false :sort-in-code? true :sort? true}
 :style [:how-to-ns]
 :width 80}
(ns fmt
  (:require
   [com.stuartsierra.component :as component]
   [clojure.string :as str]))

(defrecord App [db]
  component/Lifecycle
    (start [this] (assoc this :db (atom {::fmt "please"})))
    (stop [this] (dissoc this :db)))

I've removed the :community style and have reached a point that looks pretty great (to me at least!)

$HOME/.zprint.edn

{:search-config? true
 :width          80}

$PWD/.zprint.edn

{:extend {:flow? true :indent 0 :nl-separator? true}
 :fn-map {"are"     [:none {:list       {:indent-only? true}
                            :next-inner {:list {:indent-only? false}}}]
          "s/cat"   [:binding {:binding {:force-nl? false}}]
          "s/fdef"  :arg1-force-nl
          "testing" :arg1-force-nl
          "try"     :flow}
 :map    {:comma? false :sort-in-code? true :sort? true}
 :style  [:binding-nl :extend-nl :hiccup :how-to-ns :justified :map-nl :pair-nl]
 :width  80}

Some example Clojure:

(ns fmt
  (:require
   [com.stuartsierra.component :as component]
   [clojure.string :as str]
   [clojure.spec.alpha :as s]
   [clojure.test :refer [are deftest is testing]]))

(defprotocol IPretty
  (pretty [this]))

(defn- init-state [_app] {:zprint/cool? true})

(defrecord App [db]
  component/Lifecycle
  (start [this]
    (if (some? db)
      this
      (let [state (init-state this)]
        ;; Some side effects would typically take place now.
        (assoc this :db (atom (assoc state ::fmt "please"))))))
  (stop [this] (dissoc this :db))

  IPretty
  (pretty [this] true))

(s/fdef string->string
  :args (s/cat :string string?)
  :ret  string?)

(defn string->string
  [string]
  (cond-> string
    (str/ends-with? string "bar") (subs 0 (rand-int (dec (count string))))))

(deftest string->string-works
  (testing "who knows what"
    (is (= {} (string->string "okay"))))
  (are [in out] (= out (string->string in))
    "foo" "foo"
    "foobar" "tests don't like rand-int"))

My only question is whether the :community style should have records and protocols indented with a single space? My understanding is that the community style is heavily linked to clojure-mode from Emacs, and the indentation doesn't match but maybe that's me misunderstanding what the community style is all about.

I'll close this issue as I'm not sure there's anything to fix.

Thanks again, Kim!

kkinnear commented 3 years ago

Thanks for noticing this -- and thanks for the kind words too. They make a difference!

As I read the community guidelines (which is where I took the :community style from), you are correct:

Use 2 spaces to indent the bodies of forms that have body parameters. This covers all def forms, ...

I changed the function types for both defprotocol and defrecord and the changes will appear in the next release. This required defining some new -body function types, and updates to several other tables in the code to make it all work the way that it should.

If you really want {:style :community} now, with the correct indentation on defprotocol and defrecord too, you can get this behavior (with a bit of work) immediately, thus:

Here is the default zprint output

% ./zprintm-1.1.1 <i176.clj
(ns fmt
  (:require [com.stuartsierra.component :as component]
            [clojure.string :as str]))

(defprotocol IThing
  (thing [this]))

(defrecord App [db]
  component/Lifecycle
    (start [this] (assoc this :db (atom {::fmt "please"})))
    (stop [this] (dissoc this :db))
  IThing
    (thing [this] (:db this)))

Here is the {:style :community}" output, unmodified:

% ./zprintm-1.1.1 '{:style :community}' <i176.clj
(ns fmt
  (:require [com.stuartsierra.component :as component]
            [clojure.string :as str]))

(defprotocol IThing
 (thing [this]))

(defrecord App [db]
 component/Lifecycle
   (start [this] (assoc this :db (atom {::fmt "please"})))
   (stop [this] (dissoc this :db))
 IThing
   (thing [this] (:db this)))

Here is the {:style :community} output modified to look the way it will in the next release:

% ./zprintm-1.1.1 '{:style :community :fn-map {"defprotocol" [:arg1-force-nl {:list {:indent-arg 2} :next-inner {:list {:indent-arg 1}}}] "defrecord" [:arg2-extend {:list {:indent-arg 2} :next-inner {:list {:indent-arg 1}}}]}}' <i176.clj
(ns fmt
  (:require [com.stuartsierra.component :as component]
            [clojure.string :as str]))

(defprotocol IThing
  (thing [this]))

(defrecord App [db]
  component/Lifecycle
    (start [this] (assoc this :db (atom {::fmt "please"})))
    (stop [this] (dissoc this :db))
  IThing
    (thing [this] (:db this)))

If you put this into your ~/.zprintrc file, or your ./zprint.edn file, it should give you what you are looking for:

{:fn-map {"defprotocol" [:arg1-force-nl
                         {:list {:indent-arg 2},
                          :next-inner {:list {:indent-arg 1}}}],
          "defrecord" [:arg2-extend
                       {:list {:indent-arg 2},
                        :next-inner {:list {:indent-arg 1}}}]},
 :style :community}

This basically forces an option map whenever defprotocol or defrecord are formatted, and forces the :indent-arg, which is the thing that :style :community uses, to be 2 for the top level of the formatting, and resets it to what :style :community wants for code lower down. Kind of a hack, but I think it will do the job until the next version comes out. Which I hope will be in a few weeks.

As an aside, I'm wondering why you are doing this:

zprint "$(<../.zprint.edn)" <fmt.clj

As I read this, you are getting the shell to insert an up-directory zprint.edn file into your command line?

The point of the {:search-config? true} in your ~/.zprintrc file is to get zprint to look into the current directory, and if finds a .zprintrc file there to use it, and if it doesn't to look for a .zprint.edn file there, and if it doesn't find that to look in the next directory up. And to keep doing this until it finds a file or ends up running out of directories. A better explanation is here in the zprint reference.

Are you doing this because your .zprint.edn file isn't getting seen? There have been some bugs (recently fixed) where errors in the up-directory .zprintrc and .zprint.edn files were detected but not signaled, causing the files to essentially be ignored. I believe that version 1.1.1 has that working correctly. It was Issue #167, and fixed in 1.1.0 -- at least I think it is fixed. But now I'm wondering...

Anyway, thanks again for noticing this community issue. I'm not a :style :community user myself nor, as it happens, an emacs user either, so I'm sometimes not as on top of the community issues as I might be. If you notice other things, please don't hesitate to let me know!

kkinnear commented 3 years ago

Oh, yes, I almost forgot. I see that you are using are. I created Issue #81 because someone asked for better formatting for are a number of years ago. I am in the final stages of implementing a new internal architecture for doing formatting outside of what comes naturally in the existing architecture, and I'm expecting to support the are formatting discussed in Issue #81. While I have plenty of trivial and contrived examples of are, what I don't have are any truly realistic example of are in use. I spent some time wandering around GitHub looking for them, but didn't really find anything useful. In the off chance you have some realistic uses of are (for example, which don't fit on two lines) which you would be willing to share, I love to see them so that the are support I'm working on will be more robust. If you format them the way that you would like to see zprint format them, that would be even better. Thanks!

jcf commented 3 years ago

Thank you for the amazing reply, @kkinnear! You are a :star:.

As an aside, I'm wondering why you are doing this:

zprint "$(<../.zprint.edn)" <fmt.clj

As I read this, you are getting the shell to insert an up-directory zprint.edn file into your command line?

I was invoking zprint from Org-mode so wanted to explicitly pull in the right configuration file. The search path stuff works just fine without that in my normal daily use.

Oh, and I found your clojure.test/are solution after writing my own inferior version. It's now in a few of my repos. Thank you again!

arichiardi commented 3 years ago

Thank you for fixing the :communty style - adding another :star: to that one above :smile:

kkinnear commented 3 years ago

This is fixed in 1.1.2.

For example:

zprint.core=> (print i176)

(ns fmt
  (:require
   [com.stuartsierra.component :as component]
   [clojure.string :as str]))

(defprotocol IThing
 (thing [this]))

(defrecord App [db]
 component/Lifecycle
 (start [this] (assoc this :db (atom {::fmt "please"})))
 (stop [this] (dissoc this :db))

 IThing
 (thing [this] (:db this)))nil
zprint.core=> (zprint-file-str i176 "x")
"\n(ns fmt\n  (:require [com.stuartsierra.component :as component]\n            [clojure.string :as str]))\n\n(defprotocol IThing\n  (thing [this]))\n\n(defrecord App [db]\n  component/Lifecycle\n    (start [this] (assoc this :db (atom {::fmt \"please\"})))\n    (stop [this] (dissoc this :db))\n  IThing\n    (thing [this] (:db this)))"
zprint.core=> (print *1)

(ns fmt
  (:require [com.stuartsierra.component :as component]
            [clojure.string :as str]))

(defprotocol IThing
  (thing [this]))

(defrecord App [db]
  component/Lifecycle
    (start [this] (assoc this :db (atom {::fmt "please"})))
    (stop [this] (dissoc this :db))
  IThing
    (thing [this] (:db this)))

Thanks for pointing it out!