Opened 5 years ago

Last modified 3 years 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 (14)

comment:1 by Krinkle, 5 years 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, 5 years ago

Owner: set to Krinkle
Status: assignednew

comment:3 by Krinkle, 5 years ago

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

comment:4 by elexis, 5 years 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: https://code.wildfiregames.com/D2070
Patch By: Krinkle
Reviewed By: Imarok

comment:5 by elexis, 5 years ago

In 22651:

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

Differential Revision: https://code.wildfiregames.com/D2147
Patch By: Krinkle

comment:6 by elexis, 5 years 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, 5 years ago

In 22655:

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

Differential Revision: https://code.wildfiregames.com/D2146
Based on patch By: Krinkle

comment:8 by elexis, 5 years 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: https://code.wildfiregames.com/D2103
Patch By: Krinkle
Related comments by: Yves, wraitii, Philip, leper on #0ad-dev on December 2013, see #2322
Source was: http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html

comment:9 by elexis, 5 years ago

In 22835:

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

Differential Revision: https://code.wildfiregames.com/D1993
Patch By: Krinkle

comment:10 by elexis, 5 years ago

In 22893:

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

https://eslint.org/docs/rules/no-caller
https://eslint.org/docs/rules/no-irregular-whitespace

Differential Revision: https://code.wildfiregames.com/D2237
Patch By: Krinkle

comment:11 by Imarok, 4 years 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: https://code.wildfiregames.com/D2452

comment:12 by Freagarach, 4 years ago

In r23584: Syntax cleaning of GUI Interface and test.

comment:13 by Krinkle, 4 years ago

Description: modified (diff)
Keywords: contributor-experience added

comment:14 by Freagarach, 3 years ago

In 25087:

Fix cosmetic ESLint warnings in JS components (and their tests).

Which were autofixable, with slight modifications.
Not done are the slightly more complex changes.

Refs. #5524
Patch by: @Krinkle
Differential revision: D2279
Comments by: @elexis, @Stan
Reviewed by: @wraitii

Note: See TracTickets for help on using tickets.