Opened 2 years ago

Closed 3 months ago

#5449 closed change (rejected)

Clarify our JavaScript indentation rules

Reported by: kzar Assignee:
Priority: Unknown Milestone:
Module: Automation Keywords: closed-in-favor-of-gitlab
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 (11)

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

  • Cc trev removed

comment:5 Changed 13 months ago by erikvold

  • Blocking 6833 added

comment:6 Changed 13 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 13 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 13 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 13 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 13 months ago by sebastian

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

comment:11 Changed 3 months 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.