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

Wrong indentation in multi-arity forms #239

Closed
myguidingstar-zz opened this issue Jun 28, 2014 · 23 comments
Closed

Wrong indentation in multi-arity forms #239

myguidingstar-zz opened this issue Jun 28, 2014 · 23 comments

Comments

@myguidingstar-zz
Copy link
Contributor

Things are currently auto-indented like this:

(defn a-function
  ([x]
     x)
  ([x y]
     (+ x y)))

and

(fn a-function
  ([x]
     x)
  ([x y]
     (+ x y)))
@expez
Copy link
Member

expez commented Jun 28, 2014

As a reference, here's how it usually looks.

(defn a-function
  [x y]
  (+ x y))

@bbatsov
Copy link
Member

bbatsov commented Jul 14, 2014

Yep, that's definitely a bug.

@wjlroe
Copy link

wjlroe commented Nov 14, 2014

What is the desired behaviour here? Both vim's Clojure indentation and LightTable's indent thus:

(defn foo
  ([x] (bar x))
  ([x y]
   (if (predicate? x)
     (bar x)
     (baz x))))

(which looks weird to me, but I think I'm used to the way clojure-mode has been indenting these for a while now). I can't find any advice in any clojure style guides (including the referenced Scheme guide) about multiple-arity clojure indentation.

@expez
Copy link
Member

expez commented Nov 14, 2014

What you just posted is the desired behavior. clojure-mode now indents the first form, of multi-arity function bodies with two spaces more than it should.

@zane
Copy link

zane commented Nov 15, 2014

I see this has the "help-needed" label. What would be useful?

@bbatsov
Copy link
Member

bbatsov commented Nov 15, 2014

It means that I don't have the time to deal with this soon. Basically, the problem is caused by us copying (inheriting) too much things from lisp-mode. We should reduce our reliance on lisp-mode.

@expez
Copy link
Member

expez commented Nov 15, 2014

Should probably rename that label, @bbatsov might want some help, but he never needs it :p

@bbatsov
Copy link
Member

bbatsov commented Nov 15, 2014

@expez LOL

@vmfhrmfoaj
Copy link
Contributor

Hi all,

Is it still a bug? already, some codes has used 3 space indent. (e.g. core.async/src/main/clojure/core/async.clj:78)
Anyway, it is related to the following code:

;; clojure-mode.el:718:clojure-indent-function:
(if (and (eq (char-after (point)) ?\[)
         (eq (char-after (elt state 1)) ?\())
    (+ (current-column) 2) ;; this is probably inside a defn
  (current-column))

and commits:

@expez
Copy link
Member

expez commented Nov 20, 2014

@vmfhrmfoaj yes, this is still a bug. That code, in core.async, is probably written using emacs, and thus it's our fault it looks like that.

@zane
Copy link

zane commented Nov 21, 2014

Well that wound up being much simpler than expected. Thanks so much for the quick response!

@tsdh
Copy link
Contributor

tsdh commented Dec 1, 2014

Phew, I was just writing a bug report that the indentation of multi-arity functions isn't as nice anymore as it used to be...

@bbatsov
Copy link
Member

bbatsov commented Dec 2, 2014

@tsdh Can't please everyone I guess. I have no idea why the indentation was implemented as it was originally. We can make this configurable, but this is not something I'm going to pursue myself.

@tsdh
Copy link
Contributor

tsdh commented Dec 2, 2014

@bbatsov Please don't make it customizable. It's good for a language and collaboration if there's one canonical style of formatting and not everybody has his own style. I'll just adapt to the new style.

@expez
Copy link
Member

expez commented Dec 2, 2014

👍

@alexander-yakushev
Copy link
Member

I'd also say I like the previous one better. Also, most of the Clojure codebase is indented the old way. Can we bring this up again and discuss a little more?

Arguments pro: in the old way the implementations of different arities are easier distinguished:

(defn foo
  ([x] 
     (bar x))
  ([x y]
     (if (predicate? x)
       (bar x)
       (baz x))))

vs

(defn foo
  ([x] 
   (bar x))
  ([x y]
   (if (predicate? x)
     (bar x)
     (baz x))))

@bbatsov
Copy link
Member

bbatsov commented Dec 3, 2014

I'd also say I like the previous one better. Also, most of the Clojure codebase is indented the old way. Can we bring this up again and discuss a little more?

Perhaps. But the argument that nobody else does the indentation like clojure-mode did is a very strong cons of the old style. I might write a blog post about this to get some more feedback.

@tsdh
Copy link
Contributor

tsdh commented Dec 3, 2014

@alexander-yakushev While I also liked the old way better, I don't buy your argument. I just checked clojure/core.clj, and there you'll find about half of the time the new indentation style, and the other half the old one. Aside from that, Clojure itself is a poor resource when it comes to deciding what's a good formatting style. It's full of trailing whitespace and far from consistent. I guess that's caused by people working with different tools on it: Emacs, CCW, LightTable, whatnot.

To me the most important aspect should be how the other editors format multi-arity fns. I just tested LightTable, and that indents like clojure-mode does now, too. Someone should check CCW and others with reasonable market share. @wjlroe already told us that Vim also indents that way.

So to summarize, all Clojure editor devs should try to use the same indentation/formatting by default. It's just frustrating if you get dozen-line patches just because you edited one line in a function that someone with some other editor wrote and some auto-formatting jumped in. (Yes, I know the relevant diff options but I prefer "just works").

@alexander-yakushev
Copy link
Member

I totally agree that it is better to have a single formatting style. I just don't consider Vim and LightTable enough authority to state that clojure-mode does it the wrong way. It could also be them who are wrong. Besides we have this https://github.com/bbatsov/clojure-style-guide which has more authority than accidental implementations in any of the editors. Let's figure out what is better, phrase it unambiguously in the style guide, and then it will be the responsibility of all editors to conform to the guide, not clojure-mode to follow other editors.

@alexander-yakushev
Copy link
Member

@bbatsov I'm 99% sure those decisions in other editors were accidental, not considered. Say, in clojure-mode and in Vim people arrived at two different styles. Then, LightTable developers randomly followed Vim style rather than in clojure-mode (but it could be vice versa). I think this doesn't qualify as an argument until there is a proof that this behaviour was intended and not just mimicked.

I say we can bring those other editor implementors into the discussion and decide upon the correct way once and for all.

@expez
Copy link
Member

expez commented Dec 3, 2014

Pretty sure our indentation was the accidental one. We were inheriting behavior from lisp-mode with no special handling of multi-arity forms. Am I right, @bbatsov?

@bbatsov
Copy link
Member

bbatsov commented Dec 3, 2014

Pretty sure our indentation was the accidental one.

There was a commit that introduced it way back in 2008 (7c3a604), but I have no idea what was the reasoning behind this. I think this was just a preference of the original clojure-mode developer. I always considered this behaviour a bug as it didn't make sense to me to treat those forms specially.

Anyways, I'm fine with starting a broader discussion on the topic.

@wjlroe
Copy link

wjlroe commented Dec 3, 2014

I submit that we move any discussion of the platonic ideal of Clojure code relating to multiple arities to the style guide. I've opened a pull request accordingly. If and when that issue gets resolved then all our tools can refer to that.

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

No branches or pull requests

8 participants