Opened 19 months ago

Last modified 4 months ago

#5449 new change

Clarify our JavaScript indentation rules

Reported by: kzar Assignee:
Priority: Unknown Milestone:
Module: Automation Keywords:
Cc: mjethani, sebastian, fhd, greiner, hfiguiere, kvas Blocked By: #5429
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

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.

Change History (10)

comment:1 Changed 19 months ago by 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.

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

comment:3 Changed 18 months ago by 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.

comment:4 Changed 14 months ago by fhd

  • Cc trev removed

comment:5 Changed 4 months ago by erikvold

  • Blocking 6833 added

comment:6 Changed 4 months ago by kzar

  • Blocking 6833 removed
  • Component changed from Sitescripts to Automation

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.

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

comment:8 Changed 4 months ago by 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.

comment:9 Changed 4 months ago by 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);
}

comment:10 Changed 4 months ago by sebastian

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

Note: See TracTickets for help on using tickets.