-
Notifications
You must be signed in to change notification settings - Fork 136
Add optional column-oriented wrapping via :max-line-length #414
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,6 +278,12 @@ | |
| (defn- reader-conditional? [zloc] | ||
| (and (reader-macro? zloc) (#{"?" "?@"} (-> zloc z/down token-value str)))) | ||
|
|
||
| (defn- structural-wrap-under-forbidden-reader-macro? [zloc] | ||
| (some (fn [a] | ||
| (and (reader-macro? a) | ||
| (not (reader-conditional? a)))) | ||
| (take-while some? (iterate z/up* (z/up* zloc))))) | ||
|
|
||
| (defn- find-next-keyword [zloc] | ||
| (z/find zloc z/right #(n/keyword-node? (z/node %)))) | ||
|
|
||
|
|
@@ -397,7 +403,8 @@ | |
| :remove-surrounding-whitespace? true | ||
| :remove-trailing-whitespace? true | ||
| :sort-ns-references? false | ||
| :split-keypairs-over-multiple-lines? false}) | ||
| :split-keypairs-over-multiple-lines? false | ||
| :max-line-length nil}) | ||
|
|
||
| (defmulti ^:private indenter-fn | ||
| (fn [_sym _context [type & _args]] type)) | ||
|
|
@@ -484,9 +491,6 @@ | |
| (defn- insert-newline-left [zloc] | ||
| (z/insert-left* zloc (n/newlines 1))) | ||
|
|
||
| (defn split-keypairs-over-multiple-lines [form] | ||
| (transform form edit-all map-key-without-line-break? insert-newline-left)) | ||
|
|
||
| (defn reindent | ||
| ([form] | ||
| (indent (unindent form))) | ||
|
|
@@ -630,6 +634,108 @@ | |
| (map count)) | ||
| max 0 (str/split lines #"\r?\n")))) | ||
|
|
||
| (defn- map-key-needs-split-for-layout? [zloc opts] | ||
| (when (map-key-without-line-break? zloc) | ||
| (let [split? (:split-keypairs-over-multiple-lines? opts) | ||
| max-len (:max-line-length opts) | ||
| max-on? (pos-int? max-len)] | ||
| (cond | ||
| (and split? (not max-on?)) true | ||
| max-on? (> (node-end-position zloc) max-len) | ||
| :else false)))) | ||
|
|
||
| (defn- split-map-keypairs-for-layout [form opts] | ||
| (transform form edit-all | ||
| #(map-key-needs-split-for-layout? % opts) | ||
| insert-newline-left)) | ||
|
|
||
| (defn split-keypairs-over-multiple-lines [form] | ||
| (split-map-keypairs-for-layout form {:split-keypairs-over-multiple-lines? true})) | ||
|
|
||
| (defn- ns-import-or-use-list-zloc? [zloc] | ||
| (and (ns-reference? zloc) | ||
| (#{:import :use} | ||
| (try (first (z/sexpr zloc)) | ||
| (catch #?(:clj Exception :cljs :default) _ nil))))) | ||
|
|
||
| (defn- ns-require-libspec-vector-zloc? [zloc] | ||
| (and (z/vector? zloc) | ||
| (ns-reference? (z/up zloc)) | ||
| (= :require (try (first (z/sexpr (z/up zloc))) | ||
| (catch #?(:clj Exception :cljs :default) _ nil))))) | ||
|
|
||
| (defn- max-len-structural-wrap-target? [zloc] | ||
| (and (not (ns-require-libspec-vector-zloc? zloc)) | ||
| (or (#{:vector :map :set :fn} (z/tag zloc)) | ||
| (= :namespaced-map (z/tag zloc)) | ||
| (and (z/list? zloc) | ||
| (or (not (ns-reference? zloc)) | ||
| (ns-import-or-use-list-zloc? zloc)))))) | ||
|
|
||
| (defn- single-line-direct-children? [zloc] | ||
| (when zloc | ||
| (loop [c (z/down zloc)] | ||
| (cond | ||
| (nil? c) true | ||
| (z/linebreak? c) false | ||
| :else (recur (z/right* c)))))) | ||
|
|
||
| (defn- libspec-vector-not-first-after-require? [zloc] | ||
| (when (ns-require-libspec-vector-zloc? zloc) | ||
| (loop [p (z/left* zloc)] | ||
| (cond | ||
| (nil? p) false | ||
| (z/whitespace-or-comment? p) (recur (z/left* p)) | ||
| (and (token? p) (= :require (z/sexpr p))) false | ||
| :else true)))) | ||
|
|
||
| (defn- wrap-ns-require-libspec-rows-to-max-length [form max-len] | ||
| (transform form edit-all | ||
| (fn [zloc] | ||
| (and (libspec-vector-not-first-after-require? zloc) | ||
| (single-line-direct-children? (z/up zloc)) | ||
| (> (node-end-position zloc) max-len))) | ||
| insert-newline-left)) | ||
|
Comment on lines
+692
to
+698
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why there's a separate function for libspecs? Why is breaking the lines here different to other forms?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General structural wrap inserts newlines between direct children of lists/vectors. Breaking “at the first child boundary” inside [my.ns :as m] splits the same require entry across lines, which is not ideal. If we later get configurable break rules (#413), this could become one configured rule among others.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to attempt this feature in two steps:
I don't think we should try to hard code specific behavior, as (a) we'll have to eventually remove it in favor of a configurable system, and (b) we don't want to break things when we switch over systems. |
||
|
|
||
| (defn- wrap-max-line-length-at-child-boundaries [zloc max-len] | ||
| (cond | ||
| (= :namespaced-map (z/tag zloc)) | ||
| (if-let [inner (some-> zloc z/down z/right)] | ||
| (if (= :map (z/tag inner)) | ||
| (let [wrapped (wrap-max-line-length-at-child-boundaries inner max-len)] | ||
| (or (some-> wrapped z/up) zloc)) | ||
| zloc) | ||
| zloc) | ||
| (not (max-len-structural-wrap-target? zloc)) | ||
| zloc | ||
| (not (single-line-direct-children? zloc)) | ||
| zloc | ||
| :else | ||
| (let [first-el (-> zloc z/down (skip-whitespace-and-commas z/right*))] | ||
| (if-not first-el | ||
| zloc | ||
| (let [last-zloc | ||
| (loop [current first-el] | ||
| (if-some [nxt (-> current z/right* (skip-whitespace-and-commas z/right*))] | ||
| (recur (if (> (node-end-position nxt) max-len) | ||
| (insert-newline-left nxt) | ||
| nxt)) | ||
| current))] | ||
| (z/up last-zloc)))))) | ||
|
|
||
|
Comment on lines
+700
to
+725
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is looks like the logic for line breaks depends on the form, but why is that the case? Why not add line breaks at the first possible place prior to the max-line-length? If certain forms are going to need specific line break, that sounds like something that should be configurable (see #413).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking was hard wrap at max-line-length is going to be unsafe e.g. I will think about not being dependent on the form and make this more generic
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By "the first possible place" I mean the first possible place that doesn't change the semantic meaning. |
||
| (defn- apply-max-line-length-structural-wrap [form opts] | ||
| (let [max-len (:max-line-length opts)] | ||
| (if (pos-int? max-len) | ||
| (-> form | ||
| (wrap-ns-require-libspec-rows-to-max-length max-len) | ||
| (transform edit-all | ||
| (fn [zloc] | ||
| (and (max-len-structural-wrap-target? zloc) | ||
| (single-line-direct-children? zloc) | ||
| (not (structural-wrap-under-forbidden-reader-macro? zloc)))) | ||
| #(wrap-max-line-length-at-child-boundaries % max-len))) | ||
| form))) | ||
|
Comment on lines
+726
to
+737
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should be aiming for a function like: (defn limit-line-length [form {:keys [max-line-length]}]
(transform edit-all #(needs-line-break? % max-line-length) add-line-break))
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take this feedback and apply it for my next iteration of this PR |
||
|
|
||
| (defn- single-column-line? [zloc] | ||
| (and (let [zloc (skip-whitespace-and-commas (z/right* zloc) z/right*)] | ||
| (or (nil? zloc) (line-break? zloc))) | ||
|
|
@@ -661,8 +767,8 @@ | |
| zloc)) | ||
|
|
||
| (defn- pad-node [zloc padding] | ||
| (-> (update-space-left zloc padding) | ||
| (z/subedit-> (pad-inside-node padding)))) | ||
| (z/subedit-node (update-space-left zloc padding) | ||
| #(pad-inside-node % padding))) | ||
|
Comment on lines
+770
to
+771
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this change made?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad, this should be reset to the way it was.. Initial drafts of my change were reporting an error for this when I ran bb lint |
||
|
|
||
| (defn- count-spaces [zloc] | ||
| (if (space? zloc) (node-str-length zloc) 0)) | ||
|
|
@@ -887,8 +993,11 @@ | |
| (-> form | ||
| (cond-> (:sort-ns-references? opts) | ||
| sort-ns-references) | ||
| (cond-> (:split-keypairs-over-multiple-lines? opts) | ||
| split-keypairs-over-multiple-lines) | ||
| (cond-> (or (:split-keypairs-over-multiple-lines? opts) | ||
| (pos-int? (:max-line-length opts))) | ||
| (split-map-keypairs-for-layout opts)) | ||
| (cond-> (pos-int? (:max-line-length opts)) | ||
| (apply-max-line-length-structural-wrap opts)) | ||
| (cond-> (:remove-consecutive-blank-lines? opts) | ||
| remove-consecutive-blank-lines) | ||
| (cond-> (:remove-surrounding-whitespace? opts) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why the
split-keypairs-over-multiple-linesfunction needs to be changed in this way? I think there needs to be a very good reason to add complexity here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep the old behavior when both are on, the keypair pass always explodes maps onto many lines before the line-length pass runs. That makes :max-line-length almost meaningless for maps (it all stems from my approach being dependent on the outer form which I agree is not generic)
My intent with this change: with a positive max and keypair, breaks on maps are width-gated; split-only (no max) stays exactly the old “always split qualifying keys” behavior.