Opened 2 years ago

Closed 3 weeks ago

#5262 closed change (rejected)

Loosen ESLint indentation rule

Reported by: greiner Assignee:
Priority: P3 Milestone:
Module: Automation Keywords: closed-in-favor-of-gitlab
Cc: kzar, sebastian, kvas Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

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}],

Change History (14)

comment:1 Changed 2 years ago by kzar

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

comment:2 Changed 2 years ago by 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.

comment:3 Changed 2 years ago by kzar

  • Cc sebastian kvas added

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.

comment:4 Changed 2 years ago by kvas

  • Priority changed from Unknown to P3
  • Ready set

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.

comment:5 follow-up: Changed 2 years ago by 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.

comment:6 Changed 2 years ago by sebastian

  • Ready unset

comment:7 in reply to: ↑ 5 Changed 2 years ago by 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?

comment:8 Changed 2 years ago by 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.

Last edited 2 years ago by sebastian (previous) (diff)

comment:9 Changed 15 months ago by kvas

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

comment:10 Changed 15 months ago by 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.

comment:11 Changed 15 months ago by kvas

  • Component changed from Sitescripts to Automation

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.

comment:12 follow-up: Changed 15 months ago by 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.

comment:13 in reply to: ↑ 12 Changed 15 months ago by 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).

comment:14 Changed 3 weeks ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.