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 )
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 , 5 years ago
comment:2 by , 5 years ago
Owner: | set to |
---|---|
Status: | assigned → new |
comment:3 by , 5 years ago
Patch: | → phab:D2103, phab:D2070 |
---|---|
Status: | new → assigned |
comment:13 by , 4 years ago
Description: | modified (diff) |
---|---|
Keywords: | contributor-experience added |
Prior work:
Recent work: