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

proposal: vertically align binding values in `let` #10

Open
uvtc opened this Issue Jan 4, 2013 · 24 comments

Comments

Projects
None yet
@uvtc
Contributor

uvtc commented Jan 4, 2013

Here's a preference I have that I think might be good for the style guide:

;; good
(let [thing       "stuff"
      other-thing "other stuff"]
  ...)

;; ok
(let [thing "stuff"
      other-thing "other stuff"]
  ...)
@drcode

This comment has been minimized.

drcode commented Jan 4, 2013

I agree this is nice in principle, but it is too laborious in text editors that don't support elastic tab stops. (http://en.wikipedia.org/wiki/Elastic_tabstop)

And, given that not even Emacs supports this, it is a feature that's almost non-existent in text editors.

@samaaron

This comment has been minimized.

samaaron commented Jan 5, 2013

Of course Emacs supports this!

In Emacs Live it's bound to C-M-z which calls the fn align-cljlet. The source is here: https://github.com/gstamp/align-cljlet

@crimeminister

This comment has been minimized.

crimeminister commented Jan 5, 2013

I'm not sure I agree that this should be canonical. Even with editor support it still requires effort to tweak alignment in this way, especially for the addition of a new let binding with a longer name requiring all other vars to be realigned. The gain in readability is (IMHO) marginal compared with the benefit of separate syntax colouring of the binding name.

That said, if it is adopted as canonical style then it would be nice to see this align-cljlet functionality added to clojure-mode.

@uvtc

This comment has been minimized.

Contributor

uvtc commented Jan 5, 2013

Created a clojure-mode issue regarding this: clojure-emacs/clojure-mode#126

Thanks, samaaron, for the heads-up regarding align-cljlet!

@uvtc

This comment has been minimized.

Contributor

uvtc commented Jan 6, 2013

Hm. My guess is that there's likely a substantial number of folks who prefer the readability of lining them up (even though it takes a little more typing), but also that it's probably too much to ask to have everyone do it that way.

What do you think of changing the existing let alignment example from:

;; good
(let [thing1 "some stuff"
      thing2 "other stuff"]
  {:thing1 thing1
   :thing2 thing2})

;; bad
(let [thing1 "some stuff"
  thing2 "other stuff"]
  {:thing1 thing1
  :thing2 thing2})

to

;; good
(let [thing "some stuff"
      other-thing "other stuff"]
  {:thing thing
   :other-thing other-thing})

;; fine as well
(let [thing       "some stuff"
      other-thing "other stuff"]
  {:thing       thing
   :other-thing other-thing})

;; bad
(let [thing "some stuff"
  other-thing "other stuff"]
  {:thing thing
  :other-thing other-thing})

?

That just lets folks know, "yeah, you won't get yelled at for lining things up, whether they're values in a let binding, entries in a map, or whatever --- but it's not required".

@bbatsov

This comment has been minimized.

Owner

bbatsov commented Jan 8, 2013

What I dislike about alignment such as the proposed one is the impact on diffs each time you a longer name in the let that would require pushing all of the others as well thus creating noise in the diffs. This also requires additional effort from team members using editors and IDEs not supporting such alignment. We might mention it in the guide as an example of good style, but I think we shouldn't recommend it.

@xumingming

This comment has been minimized.

xumingming commented Jan 8, 2013

Agree with @bbatsov that vertically alignment will cause some unneeded change when add a longer name in the let.

@uvtc

This comment has been minimized.

Contributor

uvtc commented Jan 8, 2013

the impact on diffs each time you a longer name in the let that would require pushing all of the others as well thus creating noise in the diffs.

Good point.

We might mention it in the guide as an example of good style, but I think we shouldn't recommend it.

No need to explicitly mention it --- maybe just imply it somewhere ... for example, perhaps change

;; good and arguably a bit more readable
{:name "Bruce Wayne"
 :alter-ego "Batman"}

to

;; good and arguably a bit more readable
{:name      "Bruce Wayne"
 :alter-ego "Batman"}
@ska2342

This comment has been minimized.

ska2342 commented Mar 30, 2013

I'd also recommend considering vertical alignment in let, ns, literal maps and vectors and maybe other place for improved readability while keeping in mind that it may complicate the code writing process if no editor support is available.

@AnneTheAgile

This comment has been minimized.

AnneTheAgile commented Apr 18, 2014

@bbatsov made an important comment;

What I dislike about alignment such as the proposed one is the impact on diffs each time you a longer name in the let that would require pushing all of the others as well thus creating noise in the diffs.

Don't all the relevant tools support diff'ing modulo whitespace? This is a vital characteristic, imho, for a comparison tool.

@yatesco

This comment has been minimized.

yatesco commented Sep 2, 2015

I know this is old, but +1 for right aligning the values. It is so much more readable... If we wanted to go this way are there any recommended plugins to support this style of indentation?

@eigenhombre

This comment has been minimized.

eigenhombre commented Sep 2, 2015

@yatesco the package https://github.com/gstamp/align-cljlet was mentioned above, if you're an Emacs-er. I agree with @bbatsov though about the cost for noise in diffs which makes this questionable as a standard (yes, I know there is an option for some diff tools to ignore whitespace, but they have to be turned on in e.g. Magit or other places where diff is called implicitly). I guess I'd be OK w/ showing this as a valid alternative, but not as the One True Way.

@yatesco

This comment has been minimized.

yatesco commented Sep 2, 2015

Thanks John, that looks great. Ironically in my situation this will make
integration easier as it a mixed Cursive and emacs camp.
On 2 Sep 2015 13:09, "John Jacobsen" notifications@github.com wrote:

@yatesco https://github.com/yatesco the package
https://github.com/gstamp/align-cljlet was mentioned above, if you're an
Emacs-er. I agree with @bbatsov https://github.com/bbatsov though about
the cost for noise in diffs which makes this questionable as a standard
(yes, I know there is an option for some diff tools to ignore whitespace,
but they have to be turned on in e.g. Magit or other places where diff is
called implicitly). I guess I'd be OK w/ showing this as a valid
alternative, but not as the One True Way.


Reply to this email directly or view it on GitHub
#10 (comment)
.

@mars0i

This comment has been minimized.

mars0i commented Dec 5, 2015

I'm sceptical of adding this as a style guideline because while I think that aligning values is a fine thing to do in certain situations, in many situations I would prefer not aligning them--for the sake of code readability.

Often the rhs of a let binding is a complex expression. If it's significantly complex, then I usually spread it over multiple lines, with proper indentation inside the expression. If it's only slightly complex, I often want to keep it all on one line, which may be preferable because then the code uses less vertical space, and the bound variables appear one per line. Either way, the result is that I sometimes want the rhs to be pushed as far to the left as possible so that it's less likely extend beyond the right margin of the editor window.

Note that when you have lets that are inside of a function definition, or maybe inside a for or another let, etc. (sometimes multiple binding expressions can't easily be combined), normal indentation pushes everything to the right, so it becomes even more valuable to move complex let binding values as far to the left as possible.

@edvorg

This comment has been minimized.

edvorg commented Apr 29, 2016

I don't know what's your experience guys. We currently use vertical alignment of values and it only gives us much more conflicts when working with git. This is caused because when you change code adding or removing bindings you typically realign let forms and it causes huge whitespace changes. Now imagine several branches making changes in different places of let bindings. It would be ok if not let binding values alignment and of course we will get conflicts here. So I personally doubt this proposal is good idea to be canonical clojure code.

@yatesco

This comment has been minimized.

yatesco commented Apr 29, 2016

I personally find this increases readability significantly and religiously use this capability from clj-refactor. To be blunt, I don't buy the 'git conflicts' issue - if you use it everywhere then the only conflicts you are going to run into is if multiple people change the same let binding concurrently, which, if frequent, I would consider a smell itself.

@edvorg

This comment has been minimized.

edvorg commented Apr 29, 2016

Well if take into account that let is one of most frequent forms used in code - yes conflicts happen quite often. When developing several unrelated to each other features changing same parts of our code at time, I'm puzzled to say what other choice do we have..

@edvorg

This comment has been minimized.

edvorg commented Apr 29, 2016

If people interested about readability of let forms, maybe it would be better to go other way and make editors only display let bindings aligned but preserve unaligned code under source control to minimize unneeded changes. What do you think, guys?

@yatesco

This comment has been minimized.

yatesco commented Apr 29, 2016

This risks getting off topic and arguing about subjective opinions but the only time I would expect a conflict is if there were multiple updates to the same let binding, which might happen occasionally but I would expect them to be rare. If this is happening all the time then I might start questioning:

  • how big are the lets? Small focussed functions should reduce this
  • how stable are my published APIs? Is the code updated because two people wanted different things from the same thing?
  • how good are my abstractions (same as previous point really)
    and so on. In other words, if this kept happening I would be concerned about the granularity of my fns/name spaces/branches etc..

I wouldn't be as concerned if multiple people updated the same namespace but in different places but that wouldn't cause any conflicts. The only problem is concurrent updating of the same let binding which I wouldn't want to be general practice anyway.

@mars0i

This comment has been minimized.

mars0i commented Apr 29, 2016

Since these are style guidelines, anyone can violate them freely, but they do have a normative effect. People reformat others' code on stackoverflow, newbies ask about conventions, etc. This case seems like one where there shouldn't be a single rule. Or rather, the rule could be something like, "Prefer aligning values other things being equal, if there's enough space on the right, etc., but in some situations you may want to do XYZ."

edvorg's proposal to let the editor do the alignment is interesting. I suppose it should be configurable though. I might want to reindent a block of code without having to go back and delete unwanted spaces that have been inserted in the let bindings (as in my example above of long multiline bindings).

@arrdem

This comment has been minimized.

Contributor

arrdem commented Apr 29, 2016

After what three years of debating this back and forth purely on anecdotal grounds it seems clear to me that there is not a consensus on "good style" which we can reasonably commend and that this thread should be closed.

Because this is an anecdotal thread, I've long had my editors (first vim now emacs) set up to automagically "repair" whitespace and enforce alignment. I've had coworkers complain about it, but I use it everywhere and for everything. I'd love it if CIDER/clojure-mode could provide vertical alignment of bindings as a view rather than as a change in the text (rant about programs as data/asts nonsense of worrying about the text formatting should work on renders of asts vc the ast not the rendering goes here), but failing that I personally find editor assisted alignment to be sane and I look forwards to cljfmt growing support for it.

@cloojure

This comment has been minimized.

Contributor

cloojure commented Apr 29, 2016

I agree with Reid & Colin that vertical alignment nearly always helps
readability tremendously. And, it is always nice if the editor can
automatically do most of the work for you.

However, a bigger issue is if whitespace is causing problems in your Git
integrations. I would STRONGLY RECOMMEND always choosing "ignore
whitespace" and "ignore blank lines" as the defaults for your Git settings.
This is an easy fix that avoids all kinds of problems and arguments. Both
GitHub and BitBucket support this. At the command line, I always use this
alias:

alias gitdw='git diff --ignore-all-space --ignore-blank-lines'

Cheers!
Alan

On Fri, Apr 29, 2016 at 11:18 AM, Reid D McKenzie notifications@github.com
wrote:

After what three years of debating this back and forth purely on anecdotal
grounds it seems clear to me that there is not a consensus on "good style"
which we can reasonably commend and that this thread should be closed.

Because this is an anecdotal thread, I've long had my editors (first vim
now emacs) set up to automagically "repair" whitespace and enforce
alignment. I've had coworkers complain about it, but I use it everywhere
and for everything. I'd love it if CIDER/clojure-mode could provide
vertical alignment of bindings as a view rather than as a change in the
text (rant about programs as data/asts nonsense of worrying about the text
formatting should work on renders of asts vc the ast not the rendering goes
here), but failing that I personally find editor assisted alignment to be
sane and I look forwards to cljfmt growing support for it.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#10 (comment)

@edvorg

This comment has been minimized.

edvorg commented Jun 8, 2016

I would argue with the thing that conflicts in map literals and cond expressions are such a rare case. We have maps everywhere and we use constructs like cond to dispatch app flow control at some higher levels of logic. For example let's take a look how we typically describe http routes in our projects. We use something like bidi describing web app routes as maps. Even if develop totally unrelated features in separate branches we would get conflicts putting these features together as both of them are likely modifying routes and likely realigning whole route maps. The same can be said about project.clj. No matter how unrelated features are and how decoupled code is, we are still having piles of conflicts everywhere.

@edvorg

This comment has been minimized.

edvorg commented Jun 8, 2016

Maybe let is quite rare but problem is still present as well

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