Loosen ESLint indentation rule

[greiner]

Background

eslint-config-eyeo includes the following line

"indent": ["error", 2, {SwitchCase: 1, ArrayExpression: "first"}],

and while it does make sense to have consistent indentation, it is questionable whether we should dictate how the indentation is supposed to be made consistent - especially given that this is not prohibited by ​our coding style either. By using ArrayExpression: "first" instead of ArrayExpression: 1, however, we do exactly that.

Some examples:

Without line break

let arr = ["abc", "def", "ghi"];

Valid with ArrayExpression: "first"

let arr = ["abc", "def",
           "ghi"];

let arr = [
  "abc",
  "def",
  "ghi"
];

Invalid with ArrayExpression: "first"

let arr = ["abc", "def",
  "ghi"];

Conclusion

While some may want to avoid expressions such as

let arr = ["abc",
  "def",
  "ghi"

it is not ideal to enforce code such as

if (longObjectName.longMethodName(["a",
                                   "a",
                                   "a",
                                   "a",
                                   "a",
                                   "a",
                                   "a",
                                   "a"]))
{
  ...
}

or

let strings = [
  "a",
  "a",
  "a",
  "a",
  "a",
  "a",
  "a",
  "a"
];
if (longObjectName.longMethodName(strings))
{
  ...
}

since it could also be written as

let longStringNames = ["a", "a", "a", "a", "a",
  "a", "a", "a"];
if (longObjectName.longMethodName(longStringNames))
{
  ...
}

instead.

What to change

In ​https://hg.adblockplus.org/codingtools/file/tip/eslint-config-eyeo/index.js replace the line

"indent": ["error", 2, {SwitchCase: 1, ArrayExpression: "first"}],

with

"indent": ["error", 2, {SwitchCase: 1, ArrayExpression: 1}],

[kzar]

Did you test this change against adblockplusui, adblockpluscore and adblockpluschrome?

[greiner]

I must admit that I haven't but if any change needs to be tested against certain repositories, it'd be great if we could add that to the README. Note that the Flattr extension code is also affected by changes to eslint-config-eyeo but we may not want to require testing changes against that repository.

[kzar]

Well it's just that before knowing if a rule (or a change to one) is a good idea we need to test it against our existing code bases. Quite often I found that rules that sounded like a good idea were a problem in practice. I don't know if that belongs in the README.

[kvas]

ArrayExpression: "first" style is more or less consistent with how we indent Python expressions. In case of Python we don't enforce it programmatically or in the style guide, so this change would make the policy for JavaScript and Python sort of consistent. I'm not sure it's the best approach, but programmatically disallowing something that the style guide permits certainly sounds wrong.

[sebastian]

I generally agree with kzar, that when we change our linter configuration, we should be aware of the changes this implies on our code base. However, if I understand correctly, the change suggested here is about relaxing an existing rule, and therefore shouldn't require any changes to existing code?

Anyway, I personally disagree with the coding practice of wrapping code like suggested here. I find code easier to read if arguments are aligned consistently.

[kvas]

I had some personal conversations with Sebastian and Thomas to not pollute this thread too much and the agreement seems to be (correct me if I'm wrong) that we should either implement this ticket or adjust the style guide to be aligned with the ESLint configuration. Opinions differ about which one of this options is preferable so perhaps we could discuss it here and decide something.

The arguments for the stricter style guide (aligned with the config) that I've heard are:

• The code is more readable this way,
• It's consistent with our Python practice (which is also not in styleguide, but Python developers seem to agree on it),
• All ABP code already complies.

and the arguments for implementing this issue are:

• The difference in readability is not significant,
• ArrayExpression: "first" is too restrictive,
• Flattr code would need to be reformatted if we keep the config as is.

Does anybody want to add something for either side? What do people feel about readability? Whom else should we invite into this discussion?

[sebastian]

FWIW, we did plenty of changes to cleanup the Adblock Plus code, when we introduced ESLint, and only removed rules we disagreed to for good reasons, not just to avoid changes to existing code. So that we might have to do the same now for Flattr, doesn't strike me as sufficient reason to further relax our rules.

Moreover, the examples given above are quite bad. You wouldn't do something like this:

let strings = [
  "a",
  "a",
  "a",
  "a",
  "a",
  "a",
  "a",
  "a"
];

But rather:

let strings = ["a", "a", "a", "a",
               "a", "a", "a", "a"];

Or:

let strings = [
  "a", "a", "a", "a", "a", "a", "a", "a"
];

Which are both allowed by the current ESLint configuration. Same when the array literal is inlined in a function call. So all this is about is how subsequent lines with array items should be indented, in which case I think the current configuration enforces a more readable and consistent style.

However, it is true that this is one of a bunch of rules which we use, but aren't explicitly outline in our coding practices. Then again, the coding practices refer to our ESLint configuration and instruct you to use it. Not sure how large a difference it would make if we'd document stuff like this redundantly in the coding practices itself. It might not hurt, but then we'd probably should do the same for a bunch of other rules.

[kvas]

Hi Thomas, is this issue still relevant? (If so, we should probably move it to automation module)

[greiner]

From what I read, none of the two sides has any real arguments. In those situations I'd always go for removing unnecessary rules if there's no reason to keep them.

[kvas]

I agree in general, but I'm not a JS developer and don't have a strong opinion (or strong arguments) about the details of this particular change. Since Sebastian (who's the module owner of Automation, where this ticket logically belongs) seems to disagree with the ticket, I'm not sure how we can proceed with it. We had discussions about using module owner power to enforce decisions like this one on other projects and we seemed to agree that this is not the right way to go. But at the end of the day, if ABP developers want the linter config to remain and Flattr developers want it to change, is there really a way to make everyone happy -- could forking the config be the least of all evils?

Whatever the outcome will be, this ticket belongs to Automation so I will make it so.

[greiner]

Note that there's anything Flattr-specific about this request. This was just something I mentioned on the side.

If we can resolve the underlying question of "should we technically enforce something that's not in the coding style", this ticket's resolution will follow automatically.

[kvas]

Replying to greiner:

Note that there's anything Flattr-specific about this request. This was just something I mentioned on the side.

If we can resolve the underlying question of "should we technically enforce something that's not in the coding style", this ticket's resolution will follow automatically.

My answer to this would be "no" (with a possible exception of some rules that everyone agrees on).

[sebastian]

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the ​new issue tracker.