align associative (Attempt to solve issue #36)

[reutsharabani]

Partially solves #36 .

I know the issue has a lot of opinions on how it should be done, this is my (trivial?) attempt to solve it.

I don't plan on supporting many features, just trivial alignment of bindings and maps.

I know there is a PR already open but I didn't see any recent updates. Hopefully it's not rude to open another one. I had to learn how to use clj-rewrite since I never used it before. I did copy some of it (I will go over comments there as well when I feel I'm done).

Comments welcome.

[reutsharabani]

@weavejester Thank you for your time reviewing this.

I just found out about the draft option so I try and remember to use it after you review again.

I would like to try and run this on a big project (I have a bunch of candidates). Or even this project.

Is there an easy way to do it? (Right now I will install the lein plugin locally and run it, but maybe there is something simpler I don't know)

[weavejester]

This is a brief initial review. I think this is one of those PRs where a complete review is going to be fairly involved.

Overall, I think the code needs to use existing functions in the namespace, rather than reinventing what's already there. The code needs some explanation, as a lot of it isn't obvious at first glance, and I've added some of my questions as comments. Function names need to be a little more detailed and accurate to aid understanding in general.

I think we need to pull out the functions that the indentation system uses to find configuration, so we can use them for finding alignment rules as well. I'll get back to you on this.

[weavejester]

I've given this a little more thought, and I think we're approaching this in the wrong way.

First, I think it makes sense to narrow the scope of the problem and focus only on aligning maps to begin with. This means we don't initially need to worry about working out what a valid binding is.

Second, I think we've been focused (this and the previous PR) on keys and values, when I think it's more universally applicable to consider columns. Consider something like:

[first second third fourth
 fifth sixth seventh
 eighth]

The column of each item in the vector is its position along the line. So in this case:

[0 1 2 3
 0 1 2
 0]

We can find the maximum width of each column by measuring the width of each element:

{0 6, 1 6, 2 7, 3 6}

Then use this to adjust the spacing between each element (padding = max - width):

[first  second third   fourth
 fifth  sixth  seventh
 eighth]

The advantage of doing it this way is that it works for a wider variety of situations. If there is only one line, there is no change to the spacing. If there are triples instead of pairs, it will align them correctly as well.

As a rough outline, I can see the following functions being used:

(defn- column-index [zloc] ...)
(defn- node-width [zloc] ...)
(defn- max-column-widths [zloc] ...)
(defn- pad-node [zloc n] ...)

I'll take a crack at this myself over the next few days, as I think I have a reasonable idea of how to approach this.

[weavejester]

I committed some code to a branch (a9876d7) to align maps along columns. I haven't added tests yet.

[reutsharabani]

I committed some code to a branch (a9876d7) to align maps along columns. I haven't added tests yet.

I think you can close this MR.

I plan on using my code for now but if you add it yourself I think this MR is pointless (and I will eventually use your code).

If I can help in any way lmk

[iterati]

Thanks for starting this ball rolling @reutsharabani. And thanks for everything @weavejester.

[weavejester]

I'll keep this PR open a little longer, as there's still part of it that may be useful, if that's okay by you.

[reutsharabani]

@weavejester just checking if there's anything I can do.

Do you think this MR can be promoted and its' core replaced later? The default behavior is that new features are off so it shouldn't break anything.

If we fix the wrapper and deal with the implementation later (maybe mark it as experimental somehow?) do you think it can make it to an RC?

[weavejester]

Merging this and replacing it later would ultimately be more work for me, so I'm afraid I'm going to take the option that saves me time, even if it means that the feature will be delayed.

I'd also like to get the code working with all feasible edge cases before releasing. This is somewhat more tricky than it seems. Consider this case, for example:

(def m {{:a 1
:b 2} [x
y]
:d [z]})

This is somewhat difficult, as the width of the key depends on whether indentation is supplied, but usually we want to apply indentation after the keys have been aligned.

[reutsharabani]

I would consider releasing a wrapper that is pretty much expected not to change (api, config) and stating that implementation may change, handle more cases, handle known cases differently etc.

I know I'd use it in many professional projects with multiple team members even if it only aligned maps and lets trivially.

Dealing with edge cases seems to me to be what delayed this on the first place as it was a never-ending discussion useful for maybe 1% of users (I can tell you that I'm working professionally on multiple code-bases and I assume non or very few places have examples like the one you provided above, but this is an anecdote of course).

If the API and config are agreed upon (or are at least extensible in a smart way) I think not dealing with all cases (even only dealing with the most trivial cases) can have significant value. If this default is for these features to be switched off - I don't see any harm.

But ultimately this is your decision of course, I'm just trying to see if there is a way to do this iteratively in smaller pieces, providing significant value by handling the common cases first, without making you work harder.

If there isn't a way, I'll be patient :) but feel free to ping me at any time.

Thanks for all the work so far. I use a lot of the code you produced for the community daily. Much appreciated!

[weavejester]

I think it's smart to iterate: that's why I want to get the simpler case of aligning maps to work, before looking into handling binding forms as well.

With regard to the idea of implementing this as an experimental feature, I understand the sentiment, but again, this creates extra work for me that takes time away from other open source projects. I don't want to have to deal with issues that might be reported as part of this feature, and while I could add a disclaimer, there are plenty of people who wouldn't read that.

I'm also not sure where I'd put this feature? On the master branch? On an experimental branch? Both have disadvantages I'd rather not deal with.

This feature was first proposed back in 2015. I'm sure people can wait a little longer for the feature to be implemented.

[reutsharabani]

I committed some code to a branch (a9876d7) to align maps along columns. I haven't added tests yet.

Based on your work I created a new branch and PR:
reutsharabani#2

(I meant to open to open it in my fork but I'm not fluent enough in github so I also opened and closed a PR to your repo by mistake)

I renamed a few things (align-bindings-args->aligns) to be more consistent and tried to keep it as idiomatic as I can to existing code.

I also tried to take a sane approach regarding commas (after running cljfmt on cljfmt):

1. keep commas (don't remove / add) - removing may be an easier approach but intrusive IMO
2. align them to the left
3. Treat them as node-width (meaning, add 1 to column max width if there is a comma after any member of that column)

I also made an attempt at grouping by adding cardinality (of group) to the column widths map.

I ran it using:

lein install
lein update-in :plugins conj "[lein-cljfmt \"0.9.2\"]" -- update-in : assoc :cljfmt "{:align-maps? true :align-forms? true}" -- cljfmt check

There is also another issue that was obvious from the run: should there be a max-align-width?

Also there is a failing test from my suite I need to handle regarding cljs map values.

If you think it's usable I can reset this branch to the branch that's based on your code (or take another approach - not sure what the manners are) to keep the discussion here.

[weavejester]

Sure, I don't mind you resetting this branch to the new code. The new branch looks a lot better, though as I mentioned previously, I'd like to get the simpler case of maps alignment working correctly first. I should have time this week to work on that.

[reutsharabani]

Sure, I don't mind you resetting this branch to the new code. The new branch looks a lot better, though as I mentioned previously, I'd like to get the simpler case of maps alignment working correctly first. I should have time this week to work on that.

+

I will split it into two PRs (master <- align maps <- align forms) when I have time.

[reutsharabani]

I've split the new PR to 2 PRs.

I've reset this as the first, only adding support to align maps.

The following which adds support for other forms if this one gets through is here for now:

reutsharabani#3

[weavejester]

Thanks. As I mentioned, I'm going to be taking a look at it this week, as I suspect that getting the core of the algorithm correct may require a degree of experience with the source code and rewrite-clj. You're welcome to take a look yourself, of course, but you also may not want to replicate the work I'll be doing.

[gianfun]

Hi there!
Does it make sense to also add a test where one of the values is longer than the other?
ex:

    (testing "preserves comments"
      (is (reformats-to?
           ["{:a 1 ;; comment"
            " :longer 200}"]
           ["{:a      1 ;; comment"
            " :longer 200}"]
           {:align-maps? true}))))

or

    (testing "preserves comments"
      (is (reformats-to?
           ["{:a 1 ;; comment"
            " :longer 200}"]
           ["{:a      1   ;; comment"
            " :longer 200}"]
           {:align-maps? true}))))

(I'm actually unsure of what would be the correct expected case, but would guess case1)

[bpringe]

Just want to say thanks for working on this, to all involved! 💜

[ericdallo]

Gentle ping @weavejester if you have any news about this (clojure-lsp users are looking forward for this feature)

[weavejester]

I've been focusing on Ring 1.11.0 currently, but this is high up on my todo list.

[Brunoporto2702]

Hello folks!! Is there any way I can help with this feature? Looking forward to it! hahaha
Cheers!!

[weavejester]

Hello folks!! Is there any way I can help with this feature? Looking forward to it! hahaha Cheers!!

I have a WIP branch align-maps, if you wanted to take a look. Obviously this is a hard problem to solve, and the edge cases are problematic. I do plan on coming back to this to try and push it over the line, but currently I haven't had the time to do so.

[Brunoporto2702]

Thanks for the answer @weavejester !
One last question, do you consider more apropriate to colaborate on this PR we are discussing now or on the branch you mentioned?
I see both of them work on the same issue.
Thanks again!

[weavejester]

The align-maps branch is the most up to date code.

[Brunoporto2702]

Hello @weavejester, hope your doing well!

Do you see any problem if I start a new branch from master to get the most up to date modifications and "pass" the work you have done so far? I tried merging with master (100 commits behind) but have been having some troubles....

Thanks!

[weavejester]

Go right ahead.