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.

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

Add ns :require sorting support #113

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@kipz
Copy link

kipz commented Jan 26, 2018

  • Add a new option: :sort-ns-requires? which defaults to false for now
  • Only sorts :require vectors (ignores symbols, lists etc)
@weavejester

This comment has been minimized.

Copy link
Owner

weavejester commented Jan 26, 2018

Thanks for the PR.

I'll have time for a more thorough review later, but my initial thoughts is that your function is too large, and you're not making use of existing functions so you're duplicating functionality.

You're also using your comments to explain what rather than why; if you're doing that, it's a strong indication that you need to split up your function instead. For example, you've written:

; check if this is a require node
(if-let [req (z/find
              (z/right node)
              z/next
              (fn [[{:keys [children]}]]
                (= :require (-> children first :k))))]

But a better way would be to write:

(if (require-node? node)

See how it conveys the same information, but without the need for a comment? Comments should be used sparingly, to explain why in the cases where it's not obvious. If the what isn't obvious, then more functions are required.

However, I don't think you need to use a loop at all. Instead, use transform and edit-all:

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

This will traverse the form, looking for a zipper loc that adheres to ns-require? and then applies sort-namespaces to transform it. You'll need to write those two functions, of course.

I'll be able to give you more detailed suggestions later on.

@kipz kipz force-pushed the kipz:master branch from 9904420 to 1adb685 Jan 26, 2018

@kipz

This comment has been minimized.

Copy link
Author

kipz commented Jan 26, 2018

Thanks @weavejester for the suggestions - I missed some useful utility functions! I've made some the of the changes you suggest, though I think the sort-requires function needs some more work - perhaps there are some more handy things in there to help?

@kipz kipz force-pushed the kipz:master branch 2 times, most recently from e7cc024 to a4fe98b Jan 26, 2018

(recur (rest unsorted-requires) sorted-requires (conj new-children (first unsorted-requires)))))))))))

(defn require-node? [zloc]
(= :require (z/sexpr (z/down zloc))))

This comment has been minimized.

@weavejester

weavejester Jan 26, 2018

Owner

You need to ensure that the :require is within a top-level ns declaration.

This comment has been minimized.

@kipz

kipz Jan 26, 2018

Author

Good catch. I'll add another test for that.


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

This comment has been minimized.

@weavejester

weavejester Jan 26, 2018

Owner

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

This comment has been minimized.

@kipz

kipz Jan 26, 2018

Author

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.

This comment has been minimized.

@weavejester

weavejester Jan 27, 2018

Owner

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.

This comment has been minimized.

@kipz

kipz Jan 29, 2018

Author

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.

@kipz kipz force-pushed the kipz:master branch from a4fe98b to 0854f02 Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment