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.

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

Support for associative syntax allignment #36

Open
timvisher opened this issue Mar 24, 2015 · 31 comments · May be fixed by #77
Open

Support for associative syntax allignment #36

timvisher opened this issue Mar 24, 2015 · 31 comments · May be fixed by #77

Comments

@timvisher
Copy link

Are there plans to support associative syntax alignment?

(let [n    :charnock
      name :ohai]
  …)

{:n    :charnock
 :name :ohai}

(defproject example
  :n    :charnock
  :name :ohai)

(ns example
  (:require [[req         "1000.0.1" :exclusions [blah]]
             [another-req "0.0.1"]]))
@weavejester
Copy link
Owner

There are. I was also going to add in a configurable "tolerance", so it can be set to align only if the spacing won't exceed a certain value. e.g.

(let [x   :foo
      ab  :bar
      baz :quz
      something-very-long :bang]
  …)

@timvisher
Copy link
Author

FWIW, my team would advocate for doing the following:

(let [x :foo
      ab :bar
      baz :quz
      something-very-long :bang]
  …)

(let [x   
      :foo

      ab
      :bar

      baz
      :quz

      something-very-long
      :bang]
  …)

Which could be another configuration toggle.

@pcarlisle
Copy link
Contributor

If this is going to be an option I'd love to have a setting where it removes any such alignment.

@ivarref
Copy link

ivarref commented May 27, 2016

+1

@jmorris0x0
Copy link

jmorris0x0 commented Jun 19, 2016

I've started work on this and have made some good progress.

Not sure yet but this may involve #39 as well.

@gdphllps
Copy link

+1

@jmorris0x0 jmorris0x0 linked a pull request Jul 25, 2016 that will close this issue
lread added a commit to lread/cljfmt that referenced this issue Mar 7, 2019
lread added a commit to lread/cljfmt that referenced this issue Mar 7, 2019
lread added a commit to lread/cljfmt that referenced this issue Mar 7, 2019
lread added a commit to lread/cljfmt that referenced this issue Mar 7, 2019
lread added a commit to lread/cljfmt that referenced this issue Mar 8, 2019
@lread
Copy link
Contributor

lread commented Mar 18, 2019

I'm plugging away at this one (some discussion in #77) but have taken a pause to see if I can update rewrite-cljs to include rewrite-clj support I'd like to use to support my change.

Another good use case for alignment I just noticed is the args of the clojure.test/are macro.

@lread
Copy link
Contributor

lread commented Mar 30, 2019

Other candidates for align might cond case and condp clauses.

@lread
Copy link
Contributor

lread commented Mar 15, 2021

I can take a crack at this one again after an upgrade to rewrite-clj v1.

@practicalli-johnny
Copy link

Scope of alignment

I would be happy with a simple on/off all or nothing mechanism for aligning. However, there isn't universal agreement on aligning, so a mechanism to specify which things to align seems useful. I assume a similar approach to clj-kondo that you can include or exclude specific patterns (allowing users to add their own in a config file - which would make the change smaller perhaps)

Aligning for these functions would be great

let, let-fn, loop, for, cond, case, condp, require, ...

Aligning of hash-map data structure

{:dbtype   "postgresql"
 :dbname   (System/getenv "DATASTORE_DB_NAME")
 :host     (System/getenv "DATASTORE_URL")
 :port     (System/getenv "DATASTORE_PORT")
 :user     (System/getenv "DATASTORE_USER")
 :password (System/getenv "DATASTORE_PASSWORD")}

Long argument alignment

I would be happy aligning long arguments in line with other arguments.
If the argument is very long, then there is a chance that it would be put on the next line, spliting the pair. With pairs split across 2 lines, it would create a separation between alignment in the pairs above and and pairs following. Any pairs following would form an alignment group of their own.
Or a blank line can be inserted before a long pair (see next section)

Blank lines should break alignment in form

Using Clojure mode clojure-indent-style 'align-arguments the alignment does not cross over blank line boundaries. This provides a very simple way to align groups of arguments

(ns practicalli.request-handler
  (:require
   ;; Web Application
   [ring.util.response :refer [response]]
   [hiccup.core        :refer [html]]
   [hiccup.page        :refer [html5 include-js include-css]]
   [hiccup.element     :refer [link-to]]

   ;; Data access
   [practicalli.hanlder-helpers :as helper]))

@lread
Copy link
Contributor

lread commented May 11, 2021

Thanks @practicalli-john, I think the blank line boundary idea is pretty clever!

@lread
Copy link
Contributor

lread commented May 19, 2021

I spent some time today refreshing my brain on where I was at with this feature.

It looks like I got as far as handling maps and binding args. When associative alignment is enabled, all maps are aligned and bindings args for forms with specific rules are also aligned. Cljfmt would include reasonable default alignment rules.

So @practicalli-john, this means my current strategy can handle let letfn loop for etc, but not cond case condp etc.

The current rule for let, as an example, is: {let [0]}. This says align within arg 0 of let. The current strategy will skip alignment if specified arg is not a vector. I think I'll have to revisit the rule syntax if we are to support more alignment variations.

For cond and friends, a rule that allows us to say args starting at position n should be treated as an alignment block could work. I don't know that we'd need to specify any arg exclusions, like start at arg n but exclude the last x args. But I am certainly interested in examples that would require more sophisticated rules.

I have yet to think about require and :require. This is a different pattern where we have a list of vectors whose items should be aligned with one another.

Just brainstorming, but I expect folks might be interested in aligning a collection of maps as well?

[{:a 1234 :b 2    :c 44}
 {:a 7    :b 8212 :c 8}]

This is a good point to decide what we'd like this feature to do.
After that we can decide what we'll do for the first cut.
And we'll shoot for an extensible syntax for alignment rules.

@ericdallo
Copy link

ericdallo commented May 20, 2021

@lread looks good to me!

IMO user should be able to choose what forms/macros will use this new feature, for example users that want that on most forms but not on require ones

@lread
Copy link
Contributor

lread commented May 20, 2021

IMO user should be able to choose what forms/macros will use this new feature, for example users that want that on most forms but not on require ones

Thanks for chiming in @ericdallo!
In my WIP, the current level of config is:

  • alignment is on or off
  • when on:
    • all maps are aligned
    • only forms that match specific rules are aligned
      • cljfmt will include default rules that the user can override

WIP config currently does NOT include:

  • the ability to override/specify alignment inline.
    I think this would be inspired by clj-kondo inline config, maybe #_{:cljfmt/align <inline rule here>} to apply/override an alignment rule to a form, and maybe #_{:cljfmt/no-align} (or #{:cljfmt/align false}) to suppress alignment rules for a form.

@timvisher
Copy link
Author

Just brainstorming, but I expect folks might be interested in aligning a collection of maps as well?

[{:a 1234 :b 2    :c 44}
 {:a 7    :b 8212 :c 8}]

This is a good point to decide what we'd like this feature to do.

Yeah! I dig that quite a bit. :)

@lread
Copy link
Contributor

lread commented May 21, 2021

Ok, my WIP now includes initial support for tabular alignment of argument subsets, to support forms like cond, case, condp etc.
Subset is currently always arg n to end of args.

Still don't have:

  • inline overrides
  • aligning of collections of like things (to support formatting of require :require, and to be used perhaps as an inline alignment for arbitrary collections of like things)

Also thinking of naming for this feature. Is "associative syntax alignment" maybe easier to digest if described as "tabular alignment"?

At some point we'll have to pester @weavejester to see if we are on a track that makes sense to him.

@ericdallo
Copy link

Also thinking of naming for this feature. Is "associative syntax alignment" maybe easier to digest if described as "tabular alignment"?

I already saw some people referring to it as "vertical alignment", but your suggestion LGTM as well

@lread
Copy link
Contributor

lread commented May 21, 2021

Also thinking of naming for this feature. Is "associative syntax alignment" maybe easier to digest if described as "tabular alignment"?

I already saw some people referring to it as "vertical alignment", but your suggestion LGTM as well

Thanks, much appreciated! I was thinking that regular indentation can also, kind of, be a form of vertical alignment, and that tabular might be more descriptive?

@practicalli-johnny
Copy link

Reviewing my code it seems alignment for hash-map and binding forms (defn, let and :require in an ns form) would cover 99% of all my coding.

Especially if a blank line separated groups of alignments

There are lots of other good ideas discussed here. Thanks for working on this.

@timvisher
Copy link
Author

Also thinking of naming for this feature. Is "associative syntax alignment" maybe easier to digest if described as "tabular alignment"?

When I originally suggested this feature I had mostly associate forms in mind (except for that ns example… 🤔), hence the name. Now that we're talking about implementing cond, case, condp, etc. I think it makes less sense.

At the same time 'tabular alignment' or 'vertical alignment' doesn't feel self-documenting to me. Once I know what's being referred to then I feel like I would get why it was named that way. But not knowing what it refers to I think I'd be scratching my head whereas 'associative syntax' at least gives me a clue as to what part of the syntax we're talking about. But I'm obviously biased on that point and I don't have another suggestion.

¯\_(ツ)_/¯ Naming's hard. Let's just go with a8b203a4-a431-459f-90d0-07ee7396e4d2? xD

@lread
Copy link
Contributor

lread commented May 21, 2021

@timvisher yeah, I get what you mean by biases, my wee brain is glued on "tabular" now.

¯_(ツ)_/¯ Naming's hard. Let's just go with a8b203a4-a431-459f-90d0-07ee7396e4d2? xD

😄

@lread
Copy link
Contributor

lread commented May 21, 2021

Especially if a blank line separated groups of alignments

@practicalli-john I too think this is a nice convention. I'll underline this one on my todo list.

@PEZ
Copy link
Contributor

PEZ commented May 23, 2021

Linking my input on how to treat ”groups” from another thread #77 (comment)

@lread
Copy link
Contributor

lread commented May 27, 2021

Thanks @PEZ, I'll paste it here for easy referencing:

Following up on the discussion about how to align N elements. How about a rule something like:

If there are more than one consecutive line with the same number of elements, they get aligned as a group, otherwise no alignment happens.

If we give @weavejester's example to this it would align the first two lines as group, then the third line would be left unaligned, and then line 4 and 5 would be aligned as a group. Like so:

[:one   :two
 :three :four
 :five
 :six  :seven :eight
 :nine :ten   :eleven
 
 :one-hundred-fifty   :ten    :fifteen
 :five-hundred        :twenty :twentyfive
 :one-thousand-fiifty :thirty :thirtyfive
 :foo :bar
 :a   :b]

Also in this example, an empty line would effectively reset the the alignment grouping. So line 7, 8, 9 would get different alignment than line 4 and 5, even though they have the same N elements. And then line 10 and 11 would get their alignment as a group.

@lread
Copy link
Contributor

lread commented May 27, 2021

As a reference, an example project that seems to have wholly embraced tabular alignment is re-com.

@timvisher
Copy link
Author

I like the newline separation of groups but I don't particularly like the ungrouping between lines of differing numbers of elements. I think if the elements are all in a contiguous series of lines it makes more sense to treat them all as a group. It's interesting in this particular example you've got going that it suggests to me at least that the data structure is wrong. I can't understand why we'd have a single vector of that many elements all with different groupings. I'd want something more like a vector of vectors, and then what would the alignment be?

[[:one   :two]
 [:three :four]
 [:five]
 [:six   :seven :eight]
 [:nine  :ten]

 [:one-hundred-fifty  :ten    :fifteen]
 [:five-hundred       :twenty :twentyfive]
 [:one-thousand-fifty :thirty :thirtyfive]
 [:foo                :bar]
 [:a                  :b]]

or

[
  [:one   :two         ]
  [:three :four        ]
  [:five               ]
  [:six   :seven :eight]
  [:nine  :ten         ]

  [:one-hundred-fifty  :ten    :fifteen   ]
  [:five-hundred       :twenty :twentyfive]
  [:one-thousand-fifty :thirty :thirtyfive]
  [:foo                :bar               ]
  [:a                  :b                 ]
]

?

This all feels a little too clever to me, TBH. Of course I think I'm still stuck on the 'associative' aspect of this. A vector is only potentially associative if its pairs. If it's anything else then it's just a list of tuples. But since we seem to want to extend this to the notion of tabular alignment maybe that makes sense. ¯_(ツ)_/¯

@lread
Copy link
Contributor

lread commented May 29, 2021

@timvisher, thanks for the observation. The newlines separation of groups seems uncontroversial to me as well.
I agree that any other group separation rules will have to be carefully considered.

For your vector of vector example, is the only differences the placement of [ and ]?
Remembering that we are still in early stages, but: I have no current plans to have tabular alignment move [ and ] for your examples. Existing indentation config/behaviour will affect the opening [, and the closing ] can be affected by existing :remove-surrounding-whitespace? config.

While we are on the topic of clever behaviour, while poking around re-com sources, I see things like this:

(def alert-box-args-desc
  (when include-args-desc?
    [{:name :id         :required false                 :type "anything"                                              :description [:span "a unique identifier, usually an integer or string."]}
     {:name :alert-type :required false :default :info  :type "keyword"         :validate-fn alert-type?              :description [:span "one of " alert-types-list]}
     {:name :heading    :required false                 :type "string | hiccup" :validate-fn string-or-hiccup?        :description [:span "displayed as a larger heading. One of " [:code ":heading"] " or " [:code ":body"] " should be provided"]}
     {:name :body       :required false                 :type "string | hiccup" :validate-fn string-or-hiccup?        :description "displayed within the body of the alert"}
     {:name :padding    :required false :default "15px" :type "string"          :validate-fn string?                  :description "padding surounding the alert"}
     {:name :closeable? :required false :default false  :type "boolean"                                               :description [:span "if true, render a close button. " [:code ":on-close"] " should be supplied"]}
     {:name :on-close   :required false                 :type ":id -> nil"      :validate-fn fn?                      :description [:span "called when the user clicks the close 'X' button. Passed the " [:code ":id"] " of the alert to close"]}
     {:name :class      :required false                 :type "string"          :validate-fn string?                  :description "CSS class names, space separated (applies to the outer container)"}
     {:name :style      :required false                 :type "CSS style map"   :validate-fn css-style?               :description "CSS styles to add or override (applies to the outer container)"}
     {:name :attr       :required false                 :type "HTML attr map"   :validate-fn html-attr?               :description [:span "HTML attributes, like " [:code ":on-mouse-move"] [:br] "No " [:code ":class"] " or " [:code ":style"] "allowed (applies to the outer container)"]}
     {:name :parts      :required false                 :type "map"             :validate-fn (parts? alert-box-parts) :description "See Parts section below."}
     {:name :src        :required false                 :type "map"             :validate-fn map?                     :description [:span "Used in dev builds to assist with debugging. Source code coordinates map containing keys" [:code ":file"] "and" [:code ":line"]  ". See 'Debugging'."]}
     {:name :debug-as   :required false                 :type "map"             :validate-fn map?                     :description [:span "Used in dev builds to assist with debugging, when one component is used implement another component, and we want the implementation component to masquerade as the original component in debug output, such as component stacks. A map optionally containing keys" [:code ":component"] "and" [:code ":args"] "."]}]))

In this vector of maps, the alignment understands that some keyword-value pairs can be missing.
Tabular alignment will never re-order source code, but it may be smart about aligning collections of maps as above, if I can find a meaningful general strategy.

Also a note: cljfmt does not currently format comments and neither will tabular indentation.
We'll address comments separately when we address existing open issues on formatting comments.

@p-himik
Copy link

p-himik commented Aug 14, 2021

I've been asked to share my 2c here, so here we go.

In my, admittedly not so humble, opinion, this particular issue #36 is a perfect example of feature creep. It's been created more than 6 years ago. And yet, the discussion about what to include is still ongoing. Now there's even some confusion about the naming - simply because the old "associative syntax alignment" name cannot represent all the features that have crept in.

In my, this time even less humble, opinion, these are the things that need to be done to bring it to fruition, at least in some sense. To make it useful for as many users and as early as possible, and not wait till we have a specification and a configuration grammar for all the corner cases that are used by a handful of projects (people behind which might not be interested in cljfmt at all):

  1. Do not clump together anything that looks remotely associative, isolate separate cases. Map literals are distinct from let et al
  2. After isolating all such cases, follow the Pareto principle and decide how to spend those ~20% of effort to get the most bang for the buck. My guess is that it would be handling map literals and let, and in the simplest way possible - just aligning all the values at the same column, nothing more. My second guess is that it has actually been implemented already in Add associative syntax alignment, WIP #77 and is relatively easy to deliver now and not in some distant future
  3. People that are interested in having anything that goes beyond what's been decided in the step 2 should create separate issues, one for each extra scenario. I'm not talking about "nice 'what if's", I'm talking about actual people with actual preferences. So, in case those don't end up important enough for step 2, "align map literals with a configurable tolerance" is completely separate from "align map literals in a special way when there's a new line between keys and values" is completely separate from that guess in step 2, and so on
  4. For the cases that were deemed important in the step 2, make sure that they're still kept separate in the configuration. Map literals are still not let forms - they don't have an obligation to follow the same rules
  5. Existing most popular editors should be examined in their ability to align the relevant forms. The defaults for cljfmt should then be set in accordance to those abilities. There's no need to create formatting schism just because a PR author uses a particular tool
  6. Finally, implement what has been decided on and deliver it. All the other issues created in step 3 will be handled separately, possibly by different people

@PEZ
Copy link
Contributor

PEZ commented Aug 14, 2021

Good points, @p-himik. Some context about #77:

  1. It is what Calva uses to implement the Format and Align command (and that not-recommended setting to enable it always).
  2. It is quite bug ridden.
  3. I have tried to merge it onto master, but failed. That doesn't mean it is hard, just that I failed in the time I spent on it (a bit too much time, given that I didn't even get to reap the fruits...).

I agree the scope for it is fine, might need some little tweaking. E.g. separate flags for let and maps. Possibly some more, it was a while since I looked at what it actually is supposed to do.

@lread
Copy link
Contributor

lread commented Aug 14, 2021

Thanks for chiming in @p-himik! I think you make some good points.

One important thing to remember is that this is open source work where contributors are all volunteers with varying amounts of free time, interest and patience. So while it might seem that some team is taking 6 years to deliver a feature, there really has been no focused team working on this. From time to time some person has picked up the torch and taken a stab at it. Perhaps you will be the next person to do so.

@reutsharabani
Copy link

I took a stab at it: #299

Trivial attempt. Comments welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.