Clarify our JavaScript indentation rules

[kzar]

Background

With the upgrade to ESLint 4 the way the indent rule works has been changed, the ​indent-legacy option has been added to provide the old ESLint 3 behaviour.

With the changes for issue #5429 we've switched to use the indent-legacy rule for now, but we'd like to switch back to using indent. Additionally some discussions were raised on #5090 and #5429 about what styles of indentation should be enforced.

What to change

• Switch back to the indent rule, discussing related options such as ObjectExpression and CallExpression.
• Either clarify indentation in the coding style guide, or adjust our ESLint configuration so that it is consistent with it.
• Verify adblockpluschrome, adblockplusui and adblockpluscore all pass with those changes.

[mjethani]

In addition to ObjectExpression, we might have to adjust rules for FunctionDeclaration, FunctionExpression, CallExpression, and MemberExpression as noted in #5429.

What these various options do can be seen in the ​ESLint documentation.

[mjethani]

There is also the question of what to do with ternary expressions, because ESLint 4 imposes its own rules and is rather ​inflexible at the moment.

[sebastian]

It seems once ​ESLint bug 8594 got fixed, we can use this feature to restore the desired behavior for ternary expressions, and no (questionable) code changes should be necessary in order to make ESLint 4+ pass with the indent rule. Instead we only have to update our ESLint configuration.

[kzar]

Sure, it would be nice to fix this. But I don't think it's a blocker for the incremental filter list updates. We can add the (literally) one or two space characters there for now, without updating our ESLint configuration.

[sebastian]

I agree with Dave, this shouldn't be blocking #6833. While it would be nice if the linter would enforce more strict (sane) indentation rules, this seems to be rather about a ​disagreement whether the indentation should match parentheses. Something we have to agree on anyway before implementing any linter rules, and once we have an agreement, I don't see how the lack of linting will block #6833.

In regard to that disagreement, I again agree with Dave, that when wrapping lines with nested logic the indentation level should match the depth of the parenthesis. Otherwise, it is getting increasingly hard to follow the logic.

[erikvold]

In the long run it is faster to decide these things now so they don't come up again in other reviews. We don't want to re-debate these things, we don't want to have to manually lint ourselves (which is hard to do if you switch between source code with different styles often), and we don't want to change the code in the future just for a coding style change. All these things take time, and so does scanning files for clues about coding style.

[greiner]

I'd argue that the expression in question is too long regardless of how it's indented.

if (downloadable.softExpiration > now &&
    downloadable.hardExpiration > now &&
    (!downloadable.diffURL ||
    (downloadable.diffSoftExpiration > now &&
    downloadable.diffHardExpiration > now)))

Instead, rewriting it would make this more readable and thereby reduce the urgency of this discussion.

e.g.

let isExpired = (softExpiration > now && hardExpiration > now);
if (isExpired && diffURL)
{
  isExpired = (diffSoftExpiration > now && diffHardExpiration > now);
}

[sebastian]

Or we could rather not add any complexity to this logic in the first place as I suggested ​here.

[sebastian]

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