Remove blank lines in forms

[camsaul]

Resolves #374

[weavejester]

Thanks for the PR. Is there any reason to limit this to definition forms? What if we instead had options:

• :remove-blank-lines-in-forms?
• :forms-allowed-blank-lines

The latter should include cond, comment and let.

Additionally, what if we narrowed down where the blank lines were allowed, as in let the pairs would only be in the first argument.

{let     #{0}
 cond    :all
 comment :all}

[camsaul]

Is there any reason to limit this to definition forms?

I guess it's not really limited to definition forms, it removes blank lines anywhere except the top level, I just wasn't sure what to call this so I tried to match the naming of the rule in the Community Style Guide https://guide.clojure.style/#no-blank-lines-within-def-forms . But that's not quite the same as what this does so I'll change it

what if we narrowed down where the blank lines were allowed

For binding vectors this builds on the recently-added :[extra-]binding-forms stuff, so for let currently just the binding vector can keep extra newlines, and for stuff like cond it's limited to the top-level of the form instead (nested forms inside cond still get considered separately). I think there's some advantage to this because you don't need to specify the binding forms for everything twice if you're also using :align-binding-columns?. Does that work? I can tweak it if you still want something more fine-grained

[camsaul]

Updated the name of this to make it clearer that it works on all forms and not just def and friends

[weavejester]

Ideally we want to follow the least complex solution. Having both :binding-forms and :pairwise-forms affect which forms are allowed blank lines is more complex than having a single option to decide that. Additionally, there are forms like comment where one reasonably might want blank lines, yet this is neither a binding form nor a pairwise form.

In fact, this has led me to reconsider :binding-forms for the same reason. I'm thinking I'll change those options to:

• :align-form-columns?
• :aligned-forms
• :extra-aligned-forms

And for this feature, I think it would be best to have:

• :remove-blank-lines-in-forms?
• :blank-line-forms
• :extra-blank-line-forms

This way the options are isolated and therefore more predictable.

[camsaul]

Makes sense. I'll rework this PR later today

[camsaul]

Alright @weavejester I think I made all the changes you suggested, please take another look

[weavejester]

Thanks for the PR. I've reviewed the code and have some suggestions/improvements.

[weavejester]

This could be a little more concise:

(defn remove-blank-lines-in-forms [form blank-line-forms alias-map]
  (let [ns-name     (find-namespace (z/of-node form))
        context     {:alias-map alias-map, :ns-name ns-name}
        blank-line? #(blank-line-in-form? % blank-line-forms context)]
    (transform form edit-all blank-line? replace-blank-lines-in-forms)))

[weavejester]

Use form-matches-key? here. Something more like:

(defn- blank-lines-allowed-in-form? [zloc blank-line-forms context]
  (and (z/list? (z/up zloc))
       (some (fn [[k indexes]]
               (and (form-matches-key? zloc k context)
                    (or (= :all indexes)
                        (contains? (set indexes) (dec (index-of zloc)))))
             blank-line-forms)))

In fact, we could use the same logic for both forms with blank lines and forms we need to align. So let's do something like:

(defn- matching-form? [zloc forms context]
  (and (z/list? (z/up zloc))
       (some (fn [[k indexes]]
               (and (form-matches-key? zloc k context)
                    (or (= :all indexes)
                        (contains? (set indexes) (dec (index-of zloc)))))
             forms)))

This replaces aligned-form?.

[weavejester]

This doesn't seem like it'll work for an arbitrary nesting depth. What we want to do is determine whether the zloc represents multiple newlines, and then iterate up the tree until we hit root (nil), checking if any containing form is also a matching form.

[weavejester]

A more accurate name for the function is replace-with-single-newline, as that's what it does, rather than what it's for.

[weavejester]

Keep lines to 80 characters or less, even in tests.

[weavejester]

I think this comment can be removed.

[weavejester]

Use an external URL to this link, like [community style recommendation][no-blank-lines].