Page MenuHomeWildfire Games

lobby,gamesetup: Fix remaining ESLint warnings
Needs ReviewPublic

Authored by Krinkle on Sep 5 2019, 1:14 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#5524
Summary

The following issues were found by ESLint within the gui/lobby and gui/gamesetup directories:

$ ./node_modules/.bin/eslint -c build/jenkins/lint-config/eslintrc.json binaries/data/mods/public/gui/lobby/

/0ad-git/binaries/data/mods/public/gui/lobby/lobby.js
    50:41  warning  Multiple spaces found before '"status"'     no-multi-spaces
    51:15  warning  Extra space before value for key 'away'     key-spacing
    51:41  warning  Multiple spaces found before '"status"'     no-multi-spaces
    52:15  warning  Extra space before value for key 'playing'  key-spacing
    52:41  warning  Multiple spaces found before '"status"'     no-multi-spaces
    53:15  warning  Extra space before value for key 'offline'  key-spacing
    53:41  warning  Multiple spaces found before '"status"'     no-multi-spaces
    54:15  warning  Extra space before value for key 'unknown'  key-spacing
   222:1   warning  Expected indentation of 5 tabs but found 4  indent
   223:1   warning  Expected indentation of 6 tabs but found 5  indent
   224:1   warning  Expected indentation of 7 tabs but found 6  indent
   225:1   warning  Expected indentation of 7 tabs but found 6  indent
   226:1   warning  Expected indentation of 6 tabs but found 4  indent
   227:1   warning  Expected indentation of 7 tabs but found 5  indent
   228:1   warning  Expected indentation of 8 tabs but found 6  indent
   229:1   warning  Expected indentation of 8 tabs but found 6  indent
   230:1   warning  Expected indentation of 7 tabs but found 5  indent
   231:1   warning  Expected indentation of 8 tabs but found 6  indent
   232:1   warning  Expected indentation of 8 tabs but found 6  indent
  1035:3   warning  Expected a default case                     default-case

✖ 20 problems (0 errors, 20 warnings)
  0 errors and 19 warnings potentially fixable with the `--fix` option.

$ ./node_modules/.bin/eslint -c build/jenkins/lint-config/eslintrc.json binaries/data/mods/public/gui/gamesetup

/0ad-git/binaries/data/mods/public/gui/gamesetup/gamesetup.js
    65:1   warning  Expected indentation of 1 tab but found 2   indent
    66:1   warning  Expected indentation of 1 tab but found 2   indent
    67:1   warning  Expected indentation of 0 tabs but found 1  indent
    68:1   warning  Expected indentation of 1 tab but found 2   indent
    69:1   warning  Expected indentation of 2 tabs but found 3  indent
    70:1   warning  Expected indentation of 2 tabs but found 3  indent
    71:1   warning  Expected indentation of 1 tab but found 2   indent
    72:1   warning  Expected indentation of 0 tabs but found 1  indent
    81:1   warning  Expected indentation of 1 tab but found 2   indent
    82:1   warning  Expected indentation of 1 tab but found 2   indent
    83:1   warning  Expected indentation of 1 tab but found 2   indent
    84:1   warning  Expected indentation of 1 tab but found 2   indent
    85:1   warning  Expected indentation of 0 tabs but found 1  indent
    86:1   warning  Expected indentation of 1 tab but found 2   indent
    87:1   warning  Expected indentation of 2 tabs but found 3  indent
    88:1   warning  Expected indentation of 1 tab but found 2   indent
    89:1   warning  Expected indentation of 2 tabs but found 3  indent
    90:1   warning  Expected indentation of 2 tabs but found 3  indent
    91:1   warning  Expected indentation of 2 tabs but found 3  indent
    92:1   warning  Expected indentation of 2 tabs but found 3  indent
    93:1   warning  Expected indentation of 1 tab but found 2   indent
    94:1   warning  Expected indentation of 0 tabs but found 1  indent
  1331:38  warning  Trailing spaces not allowed                 no-trailing-spaces

✖ 23 problems (0 errors, 23 warnings)
  0 errors and 23 warnings potentially fixable with the `--fix` option.

I then let ESLint auto-fix the style issues by running eslint --fix … with the same parameters.

Manual changes: lobby

The default-case in updateGameList() required manual intervention. The function sorts the games based on the currently selected column from gamesBox, but lacks a default behaviour (in case the column is unknown or not handled).

I've updated it to make sorting by the name column the default behaviour, which matches what we already do in updatePlayerList() for example.

Manual changes: gamesetup

(None.)

Test Plan

CI: Happy Coala bears.

Manual test of lobby:

  • Start game and open the multi-player lobby.
  • Ensure there is at least one game (host one if there's and use the chat icon to get back to the games list).
  • Click all the column headers for the game list.
  • Observe there are no errors in the game, nor on the command-line.

Manual test of gamesetup:

  • Start game and click single player.

Event Timeline

Krinkle created this revision.Sep 5 2019, 1:14 AM
Owners added a subscriber: Restricted Owners Package.Sep 5 2019, 1:14 AM
Vulcan added a comment.Sep 5 2019, 1:16 AM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/75/display/redirect

Krinkle edited the test plan for this revision. (Show Details)Sep 5 2019, 1:17 AM
Vulcan added a comment.Sep 5 2019, 1:18 AM

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

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

elexis added a subscriber: elexis.EditedSep 5 2019, 12:09 PM

Those are just the remaining warnings in lobby/ right? (The title of the diff should encompass all changes performed and only those changes performed) (cant read)
Since its very few changes, it seems the diff could be enlarged a bit?
Otherwise seems ok.

binaries/data/mods/public/gui/lobby/lobby.js
224

These lines are also bugged somewhere #4877.

Krinkle updated this revision to Diff 9664.Sep 7 2019, 10:38 PM
Krinkle retitled this revision from lobby: Fix remaining ESLint warnings to lobby,gamesetup: Fix remaining ESLint warnings.
Krinkle edited the summary of this revision. (Show Details)
Krinkle edited the test plan for this revision. (Show Details)

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/95/display/redirect

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

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

elexis added inline comments.Sep 12 2019, 11:45 PM
binaries/data/mods/public/gui/lobby/lobby.js
224

(fixed, was only bugged in C++)

I remember Imarok proposing this diff once.

I'm not so happy about this indentation, because previous to this diff, it leaves a nice impression of an if-elseif-elseif-else chain.

I see eslint indent has many options, there is none that we could change that makes this pass without becoming less strict for most of the rest of the code?

Other than that, it might also be nicer altogether to put this into a (for now global) JS object, somehow, so that one could pick the string using g_RoleSomething[msg.oldrole][msg.newrole].

This way one wouldnt have to disable linting, wouldnt have to make the indentation ugly, and wouldn't even have nicer code.

Something like that:

var g_RoleMessages = {
	"visitor": (me, oldrole) =>
		me ?
			translate("You have been muted.") :
			translate("%(nick)s has been muted."),
	"moderator":
		 (me, oldrole) =>
			translate("You are now a moderator.") :
			translate("%(nick)s is now a moderator.") :
	},
	"participant":
		(me, oldrole) =>
			oldrole == "visitor" ?
				translate("You have been unmuted.") :
				translate("%(nick)s has been unmuted.") :
			me ?
				translate("You are not a moderator anymore.") :
				translate("%(nick)s is not a moderator anymore.")
	}
};

But I suppose it still complains about the last object, but indenting there would be less painful than here.

elexis added inline comments.Sep 20 2019, 6:51 PM
binaries/data/mods/public/gui/lobby/lobby.js
224

Really don't like this indentation change, can this be disabled without disabling it in many other places?

1037

Perhaps we a warning as a default case would be strictly more correct, since if the case was triggered, it would be actually an error? Then if theres a warning, then the author will recognize to change the code, rather than it silently being ignored.

Also wouldn't refuse changing ' to " for the strings in the cases.

(Modders who overwrote the olist object in the XML file also have to overwrite this unless the inserted column happened to match this default case, which sounds unlikely .)

Krinkle added inline comments.Sep 22 2019, 10:08 PM
binaries/data/mods/public/gui/lobby/lobby.js
224

It can be disabled for the individual file by placing /* eslint-disable indent */ at the start of the file. This will however also disable the rule for all other types of indentation.

It can also be disabled on a per-line or per-block level by placing an inline disable and and enable comment around the block in question.

As for the style in general, are there other places where you wish this ternary style to be enforced? Maybe I'm misunderstanding you.

The style in general could be swung around the other way by setting flatTernaryExpressions": false for the indent rule in our configuration file.

https://eslint.org/docs/rules/indent#flatternaryexpressions

Krinkle added inline comments.Oct 6 2019, 1:45 AM
binaries/data/mods/public/gui/lobby/lobby.js
1037

OK. Can add a warning. What kind of warning, through what mechanism? I'm not sure what options are available/appropriate.

1037

Btw, note in the commit message I wrote I did this to match existing behaviour of updatePlayerList(). Should that be changed as well?