kkinnear / zprint

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

Sort require does not apply depending on order in style #327

Open arichiardi opened 3 months ago

arichiardi commented 3 months ago

Hi Kim, I am finally taking a look at this amazing new feature and I think I found an issue:

The order :style seems to matter when using :sort-require - it basically does not sort in this scenario (which is what I first tried and part of our actual conf):

$ echo '(ns foo.bar (:require [a.c :as ac] [a.b :as ab]))' | zprint '{:style [:community :sort-require :how-to-ns :binding-nl]}' 
(ns foo.bar
  (:require
   [a.c :as ac]
   [a.b :as ab]))

If I move :sort-require after :how-to-ns I get the right behavior instead but I lose the previous formatting (which means changing all the files in our project :sweat:).

$ echo '(ns foo.bar (:require [a.c :as ac] [a.b :as ab]))' | zprint '{:style [:community :how-to-ns :sort-require :binding-nl]}' 
(ns foo.bar
  (:require [a.b :as ab]
            [a.c :as ac]))

Maybe I am doing something unexpected or maybe you have another suggestion?

Thanks as always!

kkinnear commented 3 months ago

Thanks for asking! The problem is straightforward to explain, though less so to fix.

While it might seem the vector of styles means that something is formatted using the first style, and then the result is passed to the second style, and formatted by that style, and then passed to the third style, and so on -- that isn't really what happens. For a variety of reasons, not least that formatting something multiple times would be potentially expensive in time.

What actually happens with the vector of styles is that the first style is applied to the options map, and then the next style is applied to the options map, and so on. Because styles are just "shorthand" for changes to the options map. As it happens, the two styles :sort-reqiure and :how-to-ns don't compose at all because both change the value of the :fn-map for ns. Thus, the last one to be processed "wins", and it's changes are the ones that are used.

How to "fix" this at one level is easy, since the changes aren't really in conflict with each other. I can make :sort-require have an argument (since styles can now be parameterized in a slightly awkward way) to also do the :how-to-ns stuff.

A larger task is to figure out if there is a way to actually compose them together, essentially automatically.

In the near term, if you put this in your :style-map, I think you will get what you were trying to accomplish:

   :sort-require-htn
        {:doc "Sort requires & refers in ns macro, possibly with regexes, and in
clude :how-to-ns.",
         :regex-vec [],
         :sort-refer? true,
         :style-fn (fn
                     ([] "sort-require-htn")
                     ([existing-options new-options style-fn-map style-call]
                      {:fn-map {"ns" [:arg1-body
                                      {:list {:hang? false
                                              :indent-arg 1
                                              :option-fn
                                                (partial
                                                  sort-reqs
                                                  (merge-deep
                                                    style-fn-map
                                                    style-call))}}]
                                 ":import" [:flow {:list {:hang? true}}],
                                 ":require" :flow}}))}

For example:

 (czprint i327 {:parse-string? true :style [:sort-require-htn]})
(ns foo.bar
  (:require
   [a.b :as ab]
   [a.c :as ac]))

I don't think of this as a long term solution, as I think that adding an additional parameter to the existing :sort-require is a better approach than just an entirely new style, in the event I don't come up with something more sophisticated. But this entirely new style will let you move forward with both :sort-require and :how-to-ns and see if that works for you. And if it does, tide you over until the next release.

arichiardi commented 3 months ago

Ok cool I see - I was actually exploring the two styles as I was aware of the "short-hand" semantics - and yes I was considering what you just wrote above (thanks for doing all the work for me. I can probably inline these without using a style correct?

In any case I will play around and let you know how it goes :D

arichiardi commented 3 months ago

I have a quick question: there is a (partial sort-reqs ...) - where is that symbol coming from? Cause I see Could not resolve symbol here.

I checked the docs for :style-map but can't figure it out (yet).

kkinnear commented 3 months ago

Expletive deleted! I neglected to put sort-reqs in the list of things accessible to sci, and therefore it is not something you can reference in a use-written option-fn. I think I have an idea how to work around this, I'll try it and get back to you.

kkinnear commented 3 months ago

So, try this:

(czprint i327 {:parse-string? true :style :sort-require :fn-map {":require" [:flow {:list {:hang? false :indent-arg 1}}]}})
(ns foo.bar
  (:require
   [a.b :as ab]
   [a.c :as ac]))

This assumes that you don't have a lot of lists that start with :require other than in ns macros, but I suspect that is the case. I think it gives you a reasonable simulation of what :sort-require and :how-to-ns would look together. If it doesn't, let me know and I'll see if I can find a better way.

Oh, it also ignores what :how-to-ns does for :import clauses. If you need that too, add ":import" [:flow {:list {:indent-arg 1}}] to the :fn-map above after the key-value pair for :require.

I hope this works...

arichiardi commented 3 months ago

That works! I do have one :import outside the ns for but I can live with that :smile: