Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ns :require sorting support #113

Closed
wants to merge 1 commit into from
Closed

Add ns :require sorting support #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -98,6 +98,9 @@ selectively enabled or disabled:
true if cljfmt should collapse consecutive blank lines. This will
convert `(foo)\n\n\n(bar)` to `(foo)\n\n(bar)`. Defaults to true.

* `:sort-ns-requires?` -
true if cljfmt should sort namespace `(:require)` vectors lexicographically
Defaults to false.

You can also configure the behavior of cljfmt:

Expand Down
32 changes: 31 additions & 1 deletion cljfmt/src/cljfmt/core.cljc
View file
Open in desktop
Expand Up @@ -297,6 +297,34 @@
(defn remove-trailing-whitespace [form]
(transform form edit-all trailing-whitespace? zip/remove))


(defn sort-requires
[require-zloc]
(z/replace
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what you're doing with this function? Also how does it handle changing the indent level?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit is a little tricky. First it collect all the vectors within the (:require..), sorts them, then effectively plays them back in sorted order in the loop, replacing only the vector nodes. This leaves all the original whitepspace nodes in place.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting approach. It would mean that comments wouldn't follow their associated clause, but comments are rare in the ns declaration anyway.

You seem to be looking for vectors, but namespaces in require can just be symbols. Perhaps instead look for non-whitespace tokens?

Maybe you want something more like:

(let [nodes  (partition-by whitespace? children)
      spaces (take-nth 2 nodes)
      forms  (take-nth 2 (rest nodes))]
  (->> forms (sort-by sexpr) (interleave spaces) (apply concat)))

Or even factor out the unravelling:

(defn unravel [n coll]
  (map #(take-nth n (drop % coll)) (range n)))

;; ...

(let [[spaces forms] (->> children (partition-by whitespace?) (unravel 2))]
  (->> forms (sort-by sexpr) (interleave spaces) (apply concat)))

Obviously that's pseudo-ish code, but should give you a rough idea of what I mean.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be cool if it worked with comments too, so I'll take a look at that. We can't rely on the same hard-coded unraveling then though.

require-zloc
(n/replace-children
(z/node require-zloc)
(cons
(n/keyword-node :require)
; because we want to keep the current whitespace and only reorder the requires
(loop [unsorted-requires (->> require-zloc z/node n/children rest)
sorted-requires (->> unsorted-requires (filter #(= :vector (n/tag %))) (sort #(compare (n/sexpr %1) (n/sexpr %2))))
new-children []]
(if (empty? unsorted-requires)
new-children
(let [unsorted-require (first unsorted-requires)]
(if (= :vector (n/tag unsorted-require))
(recur (rest unsorted-requires) (rest sorted-requires) (conj new-children (first sorted-requires)))
(recur (rest unsorted-requires) sorted-requires (conj new-children (first unsorted-requires)))))))))))

(defn require-node? [zloc]
(and
(= :require (z/sexpr (z/down zloc)))
(= (symbol "ns") (z/sexpr (z/next (z/up zloc))))))

(defn sort-ns-requires [form]
(transform form edit-all require-node? sort-requires))

(defn reformat-form [form & [{:as opts}]]
(-> form
(cond-> (:remove-consecutive-blank-lines? opts true)
Expand All @@ -308,7 +336,9 @@
(cond-> (:indentation? opts true)
(reindent (:indents opts default-indents)))
(cond-> (:remove-trailing-whitespace? opts true)
remove-trailing-whitespace)))
remove-trailing-whitespace)
(cond-> (:sort-ns-requires? opts false)
sort-ns-requires)))

(defn reformat-string [form-string & [options]]
(-> (p/parse-string-all form-string)
Expand Down
21 changes: 21 additions & 0 deletions cljfmt/test/cljfmt/core_test.cljc
View file
Open in desktop
Expand Up @@ -234,3 +234,24 @@
(is (= (reformat-string "(juxt +' -')") "(juxt +' -')"))
(is (= (reformat-string "#\"(?i)foo\"") "#\"(?i)foo\""))
(is (= (reformat-string "#\"a\nb\"") "#\"a\nb\"")))

(deftest sorting-ns-requires
(is (= (reformat-string "(ns foo)"
{:sort-ns-requires? true})
"(ns foo)"))
(is (= (reformat-string "(ns foo (:require [b :refer :all] [c :refer :all] [a :refer :all]))"
{:sort-ns-requires? true})
"(ns foo (:require [a :refer :all] [b :refer :all] [c :refer :all]))"))
(is (= (reformat-string "(ns foo (:require [b :refer :all] [c :refer :all] [a :refer :all])) (def test {:require [1 2 3]})"
{:sort-ns-requires? true})
"(ns foo (:require [a :refer :all] [b :refer :all] [c :refer :all])) (def test {:require [1 2 3]})"))
(is (= (reformat-string "(ns foo\n (:require [c :refer :all]\n [b :refer :all]\n [a :refer :all]))"
{:sort-ns-requires? true})
"(ns foo\n (:require [a :refer :all]\n [b :refer :all]\n [c :refer :all]))"))
(is (= (reformat-string "(ns foo\n (:require [c :refer :all]\n [b :refer :all]\n [a :refer :all])\n (:require [d :refer :all]\n [f :refer :all]\n [e :refer :all]))"
{:sort-ns-requires? true})
"(ns foo\n (:require [a :refer :all]\n [b :refer :all]\n [c :refer :all])\n (:require [d :refer :all]\n [e :refer :all]\n [f :refer :all]))"))
(is (= (reformat-string "(ns foo\n (:require [c :refer :all]\n baz\n [b :refer :all]\n [a :refer :all]\n bar))"
{:sort-ns-requires? true})
"(ns foo\n (:require [a :refer :all]\n baz\n [b :refer :all]\n [c :refer :all]\n bar))")))