1530221 - make calendar use 2-space indent

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

The 4 space indent that calendar/ uses is not common elsewhere and Mozilla code is converging to 2 space indention (in JavaScript).

As I've been looking at some calendar patches lately, this really stands out for lightning code.
I asked Philipp some time ago and he said he'd be ok moving to 2 space indention.

It's probably best to do eslint --fix and get it all converted in one go.

•    "indent-legacy": [2, 4, { SwitchCase: 1, }],
  

•    "indent": [2, 2, { SwitchCase: 1, }],
  

But require 2 after that, or just leave it at recommended at 2?
For global { } in a file, as for custom elements if you don't want to leak the class into global scope, it would be preferable not to be required to indent at all. If we leave it at required to be 2 space, that's not possible I think. Toolkit doesn't have a requirement for indention - that is why they get away with it.

Comment 1

[[:MakeMyDay]]

I'm strongly in favour to wontfix this. I never liked the 2 case intentation - it's less readable. The ruleset defined for calendar is well balanced and we had a look at each rule when we introduced linting. Linter rules are per component, so what other components use doesn't make a (strong) argument here.

And in any event, letting the linter fix a changed rule is not a good idea if you cannot exclude aplying any other rule from that fix (and to my understanding, you cannot) - the result produces also not wanted reformattings.

Comment 2

[Magnus Melin [:mkmelin]]

Of course it's a question of taste. Sure, linting can be per component, but that is not to encourage different styling, but to be able to move towards unified styling piece-by-piece. I looked through the other rules calendar has, and didn't yet notice much other that's controversial It's mostly more strict rules than elsewhere, which doesn't really hurt.

--fix won't change anything except the indent, since everything else is already according to the rules. It's not perfect of course, so there some manual checking is needed. It does take care of most cases.

In case you didn't hear, Mozilla central ran the fixing over most part of their c++ code recently.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Just automating this is going to be difficult. The new indent rule does all sorts of unexpected things, such as misaligning lines:

var foo = bar.QueryInterface(Ci.nsIInterface)
             .baz;

which turns into:

var foo = bar.QueryInterface(Ci.nsIInterface)
  .baz;

Personally I've moved to 2 space indent for a lot of my projects and have grown accustomed to it. It does help with these cases:

if (a.condition &&
    that.goes.over["multiple"].lines) {
    this_code_is_already_the_body();
}

vs:

if (a.condition &&
    that.goes.over["multiple"].lines) {
  this_code_is_already_the_body();
}

I'd be open to entertaining the thought, there is definitively some bikeshedding involved though.

Comment 4

[Paul Morris [:pmorris]]

We should decide this before the auto-formatting of JS code happens (in bug 1572047). If all goes well, I'd like to land that bug right before the next TB beta release (the one corresponding to FF 70, which is released on Tues Sept. 3rd). That will make it easy to backport changes to the beta release. That's roughly three weeks from now.

The changes in that bug answer the automation question(s).

As someone who works on both TB and Calendar code, I'd be in favor of standardizing on 2 space indent for both. Among other things it makes the limited number of characters per line go further. I'm accustomed to 2 space indent and I've found the readability is fine for me.

Comment 5

[Paul Morris [:pmorris]]

Philipp, Any chance to reach a decision on this before the Prettier auto-formatting on Thurs Aug 29?

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Let's move to 2 space indent. Readability takes a short while getting used to, but in the end you grow used to it. Sorry MakeMyDay!

We do need to be careful with auto-reformatting though, as per comment 3.

Comment 7

[Magnus Melin [:mkmelin]]

Prettier is going to auto-format things the way it sees things best (which we may not always agree with). There's quite little to configure there.

Comment 8

[Paul Morris [:pmorris]]

Resolved by bug 1577606.