var -> let
This file has only one additional opened revision (D2106) and it is by me, so noone other than me has merge conflicts.
Details
Details
- Reviewers
elexis - Commits
- rP22523: Trader linting, refs D2106.
Check nothing is broken by that change.
Diff Detail
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
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/166/display/redirect
Comment Actions
Out of curiosity, what's the difference between var and let? And if it should be let, then why do so many files have var? (Try running grep -cr 'var ' in the ai, components, and helpers folders.)
Comment Actions
var is global declaration of variable
if you do var once in some function, that variable exists outside the scope
you can do multiple var with the same name and variable gets redeclared
let is scope variable, it stops exists after scope ends (if, while, function) and if you do multiple let in one scope you get error
I am not sure why, mostly it is old code
We are aware of that, thats why me and others (some of them) inline var->let if someone has diff changes around that variable
And I am planing to clear that on files that have no merge conflicts one by one to keep it small
Also it is CC https://trac.wildfiregames.com/wiki/Coding_Conventions#JavaScript
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
Executing section JS...
| | [NORMAL] ESLintBear (curly):
| | Unnecessary { after 'if' condition.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| 93| 93| cmpTargetMarket.AddTrader(this.entity);
| 94| 94| }
| 95| 95| else if (this.markets.length == 1)
| 96| |- {
| | 96|+
| 97| 97| // If we have only one market and target is different from it,
| 98| 98| // set the target as second one
| 99| 99| if (target == this.markets[0])
| 105| 105| cmpTargetMarket.AddTrader(this.entity);
| 106| 106| this.goods.amount = this.CalculateGain(this.markets[0], this.markets[1]);
| 107| 107| }
| 108| |- }
| | 108|+
| 109| 109| else
| 110| 110| {
| 111| 111| // Else we don't have target markets at all,
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| 98| 98| // set the target as second one
| 99| 99| if (target == this.markets[0])
| 100| 100| return false;
| 101| |- else
| 102| |- {
| | 101|+
| 103| 102| this.index = 0;
| 104| 103| this.markets.push(target);
| 105| 104| cmpTargetMarket.AddTrader(this.entity);
| 106| 105| this.goods.amount = this.CalculateGain(this.markets[0], this.markets[1]);
| 107| |- }
| | 106|+
| 108| 107| }
| 109| 108| else
| 110| 109| {
| | [NORMAL] ESLintBear (object-curly-spacing):
| | A space is required before '}'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| 274| 274| let max = 1;
| 275| 275| if (cmpObstruction)
| 276| 276| max += cmpObstruction.GetUnitRadius()*1.5;
| 277| |- return { "min": 0, "max": max};
| | 277|+ return { "min": 0, "max": max };
| 278| 278| };
| 279| 279|
| 280| 280| Trader.prototype.OnGarrisonedUnitsChanged = function()
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/172/display/redirect
Comment Actions
What `let' is:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let
As mentioned the benefit is that the variable is only declared in the innermost scope, whereas var is global to the function.
So sometimes there were two scopes using the same var and overwriting or reusing values without being aware of that, whereas with let that's impossible.
Or var defined in a local scope but used outside of the scope, so that the reader was mislead to believe that the var was used only in that scope when it wasn't.
Also var can be read from before it was defined, while for let it bugs out.
Why let only occurs in new code:
Initial definition:
ECMAScript 2015 (6th Edition, ECMA-262)
| binaries/data/mods/public/simulation/components/Trader.js | ||
|---|---|---|
| 49 ↗ | (On Diff #9034) | ++x while at it |
| 277 ↗ | (On Diff #9034) | +\ as the bot pointed out |
Comment Actions
That's an oversimplification.
function test()
{
var a = 6;
console.log(a);
}
test();
console.log(a);would produce a ref error.
Comment Actions
++, space, also removes not needed else branching because if before use return, and inlines one variable
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/177/display/redirect
Comment Actions
Patch achieves the objective of silencing the linter in this file (except for one controversial brace, the rule will probably be nuked D2070).
Tested quickly with self-trading works.
(With "global to the function" I meant referable to in the entire scope of the function, specs phrase it as "locally to an entire function regardless of block scope"; at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/statements/var its defined as "execution context, which is either the enclosing function or, for variables declared outside any function, global".)
| binaries/data/mods/public/simulation/components/Trader.js | ||
|---|---|---|
| 106 ↗ | (On Diff #9043) | Good |
| 159 ↗ | (On Diff #9043) | Okay with this inlining since it's a number. (There is always some debate whether one should check for QueryOwnerInterface returning an object in cases where one expects it always to do that, so inlining the other variables might already be controversial) |