Opened 13 months ago

Last modified 3 months ago

#5524 assigned enhancement

Have all JS source code pass ESLint

Reported by: Krinkle Owned by: Krinkle
Priority: Nice to Have Milestone: Backlog
Component: Build & Packages Keywords: contributor-experience
Cc: Patch: phab:D2103, phab:D2070

Description (last modified by Krinkle)

ESLint helps in three main areas:

  • valid syntax (because syntax errors might not be found easily otherwise, given code is loaded on-demand only),
  • consistent style for the code (style rules),
  • detect and prevent common bugs by disallowing fragile or broken code (quality rules).

By default it only does syntax validation. But r20364 introduced a few style and quality rules as well.

Right now, there's a lot of violations in the source code, causing noise during code review.

It seems there may be a few rules we no longer agree with, in which case we should disable or change them to match our desired outcome.

For other cases, we should just fix them wholesale once and have a clean build going forward.

The style rules can be easily and safely auto-fixed by eslint --fix, which performs non-semantic changes only. The quality violations are best addressed in smaller changes focussing on an individual area only with understanding of local context.


contributor-experience: Having ESLint passing generally suggests (although not per se) that most of our style guides are encoded in the lint configuration, this means new contributors can automatically write code correctly and have fewer hurdles. Either because their IDE auto-matically corrects or guides them as they go, or because they find out automatically from Jenkins with wording (ESLint warnings) that are familiar to them if they have JS experience outside 0AD, or can generally learn much about on the Internet given there's lots of docs out there about JS and ESLint.

It also drains less reviewer time, and has the potential to improve code quality overall.

Change History (13)

comment:1 by Krinkle, 13 months ago

Prior work:

Recent work:

  • r22419: Fix eslint semicolon-related warnings in all components,
  • r22472: Migrate no-floating-decimal rule from jshint to eslint.

comment:2 by Krinkle, 13 months ago

Owner: set to Krinkle
Status: assignednew

comment:3 by Krinkle, 13 months ago

Patch: phab:D2103, phab:D2070
Status: newassigned

comment:4 by elexis, 13 months ago

In 22529:

Disable eslint curly rule, because none of its configurable enforcement reflects the curly brace convention of this repository, refs #5524.

Differential Revision:
Patch By: Krinkle
Reviewed By: Imarok

comment:5 by elexis, 12 months ago

In 22651:

Hide ESLint no-unmodified-loop-condition hint for a while (const) in gamesetup.js, refs #5524.

Differential Revision:
Patch By: Krinkle

comment:6 by elexis, 12 months ago

In 22654:

Duplicate clearChatMessages since that ends up being cleaner
than having a stray function in gui/common/ that refers to page specific names and logic with a try-catch block to shield against reference errors, refs #3396, rP17465, rP17467.

Incidentally fixes ESLint no-empty hint triggered for empty catch blocks, refs #5524, D2146.

comment:7 by elexis, 12 months ago

In 22655:

Fix ESLint no-multi-assign, default-case, no-empty, no-shadow warnings, refs #5524.

Differential Revision:
Based on patch By: Krinkle

comment:8 by elexis, 12 months ago

In 22803:

Remove PETRA Module and Augmentation pattern from rP14441, refs #2322, and in Petra since rP14865.

The objective of rP14441 to have only one global for multiple AIs is still achieved by not changing the resulting object structure.
The advantage of not using the pattern is to have less parental scopes, i.e. more localization and separation of concerns.
This resembles the successful simulation script component system.
The stated benefits of the Augmentation pattern were never used, because there are (luckily) no such private variables or globals passed as arguments.

Fixes an ESLint warning about using PETRA before it is defined, refs #5524, D1993.
Fixes the missing level of indentation if indentation should represent scoping proportionally.

Remove superfluous, thus misleading name PetraBot in PETRA.PetraBot assignment.

Differential Revision:
Patch By: Krinkle
Related comments by: Yves, wraitii, Philip, leper on #0ad-dev on December 2013, see #2322
Source was:

comment:9 by elexis, 11 months ago

In 22835:

Fix some Petra whitespace; with rP22803/D2103 fixes all ESLint issues in petra/, refs #5524.

Differential Revision:
Patch By: Krinkle

comment:10 by elexis, 11 months ago

In 22893:

Enable no-caller and no-irregular-whitespace in ESLint, refs #5524.

Differential Revision:
Patch By: Krinkle

comment:11 by Imarok, 8 months ago

In 23261:

Linting: Remove "no-lone-blocks" rule for ESLint

Lone blocks can help reading the code.

Patch by: Krinkle
Refs #5524
Differential Revision:

comment:12 by Freagarach, 3 months ago

In r23584: Syntax cleaning of GUI Interface and test.

comment:13 by Krinkle, 3 months ago

Description: modified (diff)
Keywords: contributor-experience added
Note: See TracTickets for help on using tickets.