Page MenuHomeWildfire Games

Fix warning introduced in the mirage cleanup, rP22247
ClosedPublic

Authored by wraitii on May 8 2019, 9:16 AM.

Details

Reviewers
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22254: Fix warning introduced in the mirage cleanup, rP22247
Summary

This fixes the warning introduced by rP22247, as a for-in loop over an array will return the index as a string instead of a number, which the c++ code doesn't expect.

My suggested fix is to switch to forEach.

Test Plan

Review code. Check that warning has disappeared.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.May 8 2019, 9:16 AM
wraitii added subscribers: Stan, elexis, Itms.
wraitii added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
195 ↗(On Diff #7922)

I'm not sure where we put { in this case, @Itms, @elexis , @Stan ?

Vulcan added a comment.May 8 2019, 9:21 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/Fogging.js
| 197| »   »   »   continue;
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unsyntactic continue

binaries/data/mods/public/simulation/components/Fogging.js
| 197| »   »   »   continue;
|    | [NORMAL] JSHintBear:
|    | Unexpected 'continue'.

binaries/data/mods/public/simulation/components/Fogging.js
| 204| »   »   »   continue;
|    | [NORMAL] JSHintBear:
|    | Unexpected 'continue'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1336/display/redirect

Itms requested changes to this revision.May 8 2019, 11:05 AM

As detected by the linter, you should use return and not continue in a function. The current code doesn't even parse.

I have nothing against forEach but why changing? The original loop over player was fine... I don't see any upside to using forEach and it pushes you into changing almost all the lines in the body of the loop. 🤔

This revision now requires changes to proceed.May 8 2019, 11:05 AM
Itms added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
195 ↗(On Diff #7922)

In my experience @vladislavbelov is the most knowledgeable about CCs :D The current style lgtm if you convince me using forEach is better.

In D1864#77117, @Itms wrote:

As detected by the linter, you should use return and not continue in a function. The current code doesn't even parse.

The flaws of winging it and not running the code :p

I have nothing against forEach but why changing? The original loop over player was fine... I don't see any upside to using forEach and it pushes you into changing almost all the lines in the body of the loop. 🤔

In general I've switched to using ForEach/map/any in places of imperative-style loops. We rarely have those in modern C++ code now, so JS might follow suite, and I think only using those where that behaviour is actually necessary would be a nice statement of code intent. This runs something for each line, so "For Each".

For example if I were mapping, you would probably agree that map is better than an imperative style loop.
This also explicits the index and the value, which is nice sometimes.

Itms added a comment.May 8 2019, 6:50 PM

Yeah as long as we don't need to break out of the loop, forEach is fine. I don't see a real need for it here, but if it's more to your tastes, why not. It's just lengthening the verification of the fix a bit, because it makes it less trivial 😊

wraitii updated this revision to Diff 7939.May 8 2019, 9:13 PM

Imperative loop.

Vulcan added a comment.May 8 2019, 9:20 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1347/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.May 8 2019, 9:24 PM
This revision was automatically updated to reflect the committed changes.
Itms added a comment.May 8 2019, 9:55 PM

Ha I was testing and I can't accept now, but it's fine for me 👍

wraitii added a comment.EditedMay 8 2019, 10:09 PM

Wanted to commit before the night and it seemed harmless enough :)