Page MenuHomeWildfire Games

gui/common: Fix ESLint code quality violations
ClosedPublic

Authored by Krinkle on Aug 4 2019, 7:59 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22655: Fix ESLint no-multi-assign, default-case, no-empty, no-shadow warnings, refs…
Trac Tickets
#5524
Summary

This fixes the subset of ESLint violations over public/mods/gui/common that are not automatically fixable by ESLint (e.g. not purely
stylistic/deterministic). Fixes for those stylistics issues I'll commit separately as they're much easier to review that way and would only add noise this diff of manually made changes.

gui/common/color.js:

  • Unexpected chained assignment. [no-multi-assign]

    At h = s = 0;.

    At r = g = b = l;.

    Fixed by assigning each one separately.
  • Expected a default case. [default-case]

    At switch (max) in rgbToHsl().

    Added an inline ignore for now, as this would require a non-trivial change to fix and might not be an issue.

    Can be revisited later in a Trac ticket, or by grepping for "eslint-disable".

gui/common/functions_utility.js:

gui/common/gamedescription.js and gui/common/settings.js:

  • Variable already declared in the upper scope. [no-shadow]

    At difficulty = ….TriggerDifficulties.find(difficulty => ….

    At victoryCondition = ….VictoryConditions.find(victoryCondition => …

    Use a different name for the inner and outer scope.

gui/common/tab_buttons.js:

  • Variable already declared in the upper scope. [no-shadow]

    At button.onPress = (category => function() { onPress(category); })(+category);

    This was a bit of a mess. It created and self-invoking arrow function that returns a regular function - all whilst inside a for-loop. Creating functions inside a loop is a dangerous anti-pattern because the block scope of the for-loop does not close over the function scope, which means most behaviours you'd expect from such a function are not actually available. All created functions would see the same single state after the loop iterations.

    Make the effectively-local variable a real local variable, and assign onPress as an arrow function instead.
Test Plan

Jenkins and Coala no longer report any ESLintBear issues in this sub tree of type default-case, no-empty, no-multi-assign, no-shadow, no-undef-init, or no-unmodified-loop-condition

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
arcpatch
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8762
Build 14363: Vulcan BuildJenkins
Build 14362: arc lint + arc unit

Event Timeline

Krinkle created this revision.Aug 4 2019, 7:59 PM
Owners added a subscriber: Restricted Owners Package.Aug 4 2019, 7:59 PM
Vulcan added a comment.Aug 4 2019, 8:01 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/305/display/redirect

Krinkle updated this revision to Diff 9250.Aug 6 2019, 10:20 PM

(Rebased)

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 153| 153| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 154| 154| 						playerDescription = translate("%(playerName)s (%(civ)s, %(state)s)");
| 155| 155| 				else
| 156|    |-					if (isActive)
|    | 156|+				if (isActive)
| 157| 157| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 158| 158| 						playerDescription = translate("%(playerName)s");
| 159| 159| 					else
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 154| 154| 						playerDescription = translate("%(playerName)s (%(civ)s, %(state)s)");
| 155| 155| 				else
| 156| 156| 					if (isActive)
| 157|    |-						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
|    | 157|+				// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 158| 158| 						playerDescription = translate("%(playerName)s");
| 159| 159| 					else
| 160| 160| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 155| 155| 				else
| 156| 156| 					if (isActive)
| 157| 157| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 158|    |-						playerDescription = translate("%(playerName)s");
|    | 158|+					playerDescription = translate("%(playerName)s");
| 159| 159| 					else
| 160| 160| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 161| 161| 						playerDescription = translate("%(playerName)s (%(state)s)");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 156| 156| 					if (isActive)
| 157| 157| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 158| 158| 						playerDescription = translate("%(playerName)s");
| 159|    |-					else
|    | 159|+				else
| 160| 160| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 161| 161| 						playerDescription = translate("%(playerName)s (%(state)s)");
| 162| 162| 			}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 157| 157| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 158| 158| 						playerDescription = translate("%(playerName)s");
| 159| 159| 					else
| 160|    |-						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
|    | 160|+				// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 161| 161| 						playerDescription = translate("%(playerName)s (%(state)s)");
| 162| 162| 			}
| 163| 163| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 158| 158| 						playerDescription = translate("%(playerName)s");
| 159| 159| 					else
| 160| 160| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 161|    |-						playerDescription = translate("%(playerName)s (%(state)s)");
|    | 161|+					playerDescription = translate("%(playerName)s (%(state)s)");
| 162| 162| 			}
| 163| 163| 		}
| 164| 164| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 348| 348| 		titles.push({
| 349| 349| 			"label": translate("Map Description"),
| 350| 350| 			"value": g_GameAttributes.settings.Description ?
| 351|    |-					translate(g_GameAttributes.settings.Description) :
|    | 351|+				translate(g_GameAttributes.settings.Description) :
| 352| 352| 					translate("Sorry, no description available.")
| 353| 353| 		});
| 354| 354| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 349| 349| 			"label": translate("Map Description"),
| 350| 350| 			"value": g_GameAttributes.settings.Description ?
| 351| 351| 					translate(g_GameAttributes.settings.Description) :
| 352|    |-					translate("Sorry, no description available.")
|    | 352|+				translate("Sorry, no description available.")
| 353| 353| 		});
| 354| 354| 	}
| 355| 355| 
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/color.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/color.js
|   1|   1| /**
|   2|   2|  * Used to highlight hotkeys in tooltip descriptions.
|   3|   3|  */
|   4|    |-var g_HotkeyTags = {"color": "255 251 131" };
|    |   4|+var g_HotkeyTags = { "color": "255 251 131" };
|   5|   5| 
|   6|   6| /**
|   7|   7|  * Concatenate integer color values to a string (for use in GUI objects)
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/327/display/redirect

elexis accepted this revision.Aug 12 2019, 4:43 PM
elexis added a subscriber: elexis.

Thanks for the patch.

I didnt run eslint to confirm that these are all matches however.

binaries/data/mods/public/gui/common/color.js
104

IMO there should never be warning-disabling comments in the code, but the code should just be compliant or the hinter should be configured.

We can spare the default: error

binaries/data/mods/public/gui/common/functions_utility.js
81

The reason there is an empty try catch block here is becaue this is untrusted input from other players, which should not trigger warnings or errors.

I'm ok with writing the reason here as aJ S comment, but we shouldnt rely on that to please ESLint.

IMO empty catch is okay to disable.

no-empty comes from rP20364. It looks like it wasn't really considered in D213?

I mean I'm ok with adding this just to please the hinter without any further benefit, and keeping the warning enabled because it may be useful in another case, sort of preemptively even with positive number of false positives.

Adding the reason here from rP21827, and an early return to make it clear that the result will be an empty array.

175

Since the reason why this exists wasnt clear until removing it and testing, I fixed it in rP22654 by erasure hard.

binaries/data/mods/public/gui/common/settings.js
428

(I came to the conclusion that there is never a reason to use abbreviations if its faster to read a variable name if it is few characters longer but a human readable word, so condition)

binaries/data/mods/public/gui/common/tab_buttons.js
47

comes from rP20684, I wonder whether the previous ones could have been ommitted too, whatever.

Tested, works as intended, whereas using +category falls apart bigtime, since thats the same reference as the iterator whereas categoryNum is a different reference each run. So the closure can be avoided indeed.

This revision is now accepted and ready to land.Aug 12 2019, 4:43 PM