Page MenuHomeWildfire Games

UnitAI: Fix ESLint coding style warnings
Needs ReviewPublic

Authored by Krinkle on Sep 10 2019, 3:38 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#5524
Summary

This commit only fixes the warnings from ESLint that are about coding style and were automatically fixable by 'eslint --fix'.

The more substantial issues that might require changing meaningful code will be in a separate commit for easier review.

All the changes in this commit were automated. In addition, I also manually made the pseudo-section comments a bit different.
They previously used four slashes (////) which violated the spaced-comment rule. The auto fix is to use // //, because
ESLint treated it like a normal inline comment. I don't know if these comments are useful long-term, seems to me like these
should be grouped in a more formal way (e.g. different namespace property or separate file loaded for those), but for now I went
with a Markdown-esque way, using // ## .

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

/0ad-git/binaries/data/mods/public/simulation/components/UnitAI.js
   331:4    warning  Method 'Order.WalkToTarget' expected no return value           consistent-return
   766:5    warning  Unnecessary 'else' after 'return'                              no-else-return
   917:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
   942:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
   992:6    warning  Method 'enter' expected no return value                        consistent-return
  1033:14   warning  Missing space before value for key 'GARRISON'                  key-spacing
  1055:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1091:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1123:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1283:12   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1340:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1515:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1537:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1569:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1723:13   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1773:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1851:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  1983:9    warning  '&&' should be placed at the end of the line                   operator-linebreak
  2028:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2144:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2419:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2452:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2558:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2624:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2663:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  2710:41   warning  A space is required after '{'                                  object-curly-spacing
  2710:101  warning  A space is required before '}'                                 object-curly-spacing
  2874:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  3055:14   warning  Expected to return a value at the end of method 'enter'        consistent-return
  3223:1    warning  Expected indentation of 2 tabs but found 7                     indent
  3280:43   warning  Unnecessary use of boolean literals in conditional expression  no-unneeded-ternary
  3396:72   warning  A space is required after ','                                  comma-spacing
  3452:38   warning  A space is required after '{'                                  object-curly-spacing
  3452:76   warning  A space is required before '}'                                 object-curly-spacing
  3536:1    warning  Expected space or tab after '//' in comment                    spaced-comment
  3706:33   warning  A space is required after '{'                                  object-curly-spacing
  3706:60   warning  A space is required before '}'                                 object-curly-spacing
  3779:19   warning  'type' is already declared in the upper scope                  no-shadow
  3877:36   warning  A space is required after '{'                                  object-curly-spacing
  3877:88   warning  A space is required before '}'                                 object-curly-spacing
  3922:36   warning  A space is required after '{'                                  object-curly-spacing
  3922:80   warning  A space is required before '}'                                 object-curly-spacing
  3947:36   warning  A space is required after '{'                                  object-curly-spacing
  3947:68   warning  A space is required before '}'                                 object-curly-spacing
  3952:36   warning  A space is required after '{'                                  object-curly-spacing
  3952:80   warning  A space is required before '}'                                 object-curly-spacing
  3957:36   warning  A space is required after '{'                                  object-curly-spacing
  3957:92   warning  A space is required before '}'                                 object-curly-spacing
  3963:37   warning  A space is required after '{'                                  object-curly-spacing
  3963:75   warning  A space is required before '}'                                 object-curly-spacing
  3965:37   warning  A space is required after '{'                                  object-curly-spacing
  3965:79   warning  A space is required before '}'                                 object-curly-spacing
  3970:36   warning  A space is required after '{'                                  object-curly-spacing
  3970:81   warning  A space is required before '}'                                 object-curly-spacing
  3973:1    warning  Expected space or tab after '//' in comment                    spaced-comment
  4626:25   warning  'target' is already declared in the upper scope                no-shadow
  4641:25   warning  'target' is already declared in the upper scope                no-shadow
  4643:5    warning  '&&' should be placed at the end of the line                   operator-linebreak
  4644:5    warning  '&&' should be placed at the end of the line                   operator-linebreak
  4687:22   warning  'ent' is already declared in the upper scope                   no-shadow
  4707:20   warning  Multiple spaces found before 'Engine'                          no-multi-spaces
  4710:39   warning  'type' is already declared in the upper scope                  no-shadow
  4711:1    warning  Expected indentation of 3 tabs but found 4                     indent
  4749:20   warning  Multiple spaces found before 'Engine'                          no-multi-spaces
  4762:1    warning  Expected space or tab after '//' in comment                    spaced-comment
  4919:4    warning  Unnecessary 'else' after 'return'                              no-else-return
  5250:36   warning  Trailing spaces not allowed                                    no-trailing-spaces
  5532:9    warning  '&&' should be placed at the end of the line                   operator-linebreak
  5535:9    warning  '&&' should be placed at the end of the line                   operator-linebreak
  5558:7    warning  '&&' should be placed at the end of the line                   operator-linebreak
  5561:7    warning  '&&' should be placed at the end of the line                   operator-linebreak
  5717:22   warning  A space is required after '{'                                  object-curly-spacing
  5717:37   warning  A space is required before '}'                                 object-curly-spacing
  5744:1    warning  Expected space or tab after '//' in comment                    spaced-comment
  5948:1    warning  Expected space or tab after '//' in comment                    spaced-comment
  5954:5    warning  '&&' should be placed at the end of the line                   operator-linebreak
  5957:1    warning  Expected space or tab after '//' in comment                    spaced-comment
After
✖ 29 problems (0 errors, 29 warnings)

/0ad-git/binaries/data/mods/public/simulation/components/UnitAI.js
   331:4   warning  Method 'Order.WalkToTarget' expected no return value     consistent-return
   916:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
   941:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
   991:6   warning  Method 'enter' expected no return value                  consistent-return
  1054:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1090:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1122:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1282:12  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1339:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1514:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1536:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1568:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1722:13  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1772:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  1850:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2027:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2143:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2418:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2451:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2557:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2623:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2662:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  2873:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  3054:14  warning  Expected to return a value at the end of method 'enter'  consistent-return
  3778:19  warning  'type' is already declared in the upper scope            no-shadow
  4625:25  warning  'target' is already declared in the upper scope          no-shadow
  4640:25  warning  'target' is already declared in the upper scope          no-shadow
  4686:22  warning  'ent' is already declared in the upper scope             no-shadow
  4709:39  warning  'type' is already declared in the upper scope            no-shadow
Test Plan

Slightly less unhappy Coala bears.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
arcpatch_D2279 (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9641
Build 16168: Vulcan BuildJenkins
Build 16167: Vulcan Build (Windows)Jenkins
Build 16166: arc lint + arc unit

Event Timeline

Krinkle created this revision.Sep 10 2019, 3:38 AM
Krinkle edited the summary of 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/122/display/redirect

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/UnitAI.js
| 331| »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'Order.WalkToTarget' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
| 916| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 941| »   »   »   "enter":·function(msg)·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 991| »   »   »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'enter' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
|1054| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1090| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1122| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1282| »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1339| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1514| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1536| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1568| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1722| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1772| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1850| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2027| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2143| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2418| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2451| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2557| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2623| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2662| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2873| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3054| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3781| »   var·isWorkType·=·type·=>·type·==·"Gather"·||·type·==·"Trade"·||·type·==·"Repair"·||·type·==·"ReturnResource";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4628| »   var·target·=·ents.find(target·=>·this.CanAttack(target));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4643| »   var·target·=·ents.find(target·=>
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4689| »   var·ent·=·ents.find(ent·=>·this.CanHeal(ent));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4712| »   »   ····cmpAttack.GetAttackTypes().some(type·=>·cmpUnitAI.CheckTargetAttackRange(this.isGuardOf,·type)))
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3743| »   »   var·order·=·{·"type":·type,·"data":·data·};
|    | [NORMAL] JSHintBear:
|    | 'order' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|3812| »   for·(var·i·=·0;·i·<·this.orderQueue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5549| »   var·targets·=·this.GetTargetsFromUnit();
|    | [NORMAL] JSHintBear:
|    | 'targets' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5550| »   for·(var·targ·of·targets)
|    | [NORMAL] JSHintBear:
|    | 'targ' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5556| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(targ,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5557| »   »   »   var·targetClasses·=·this.order.data.targetClasses;
|    | [NORMAL] JSHintBear:
|    | 'targetClasses' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5637| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5640| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5645| »   »   var·cmpRanged·=·Engine.QueryInterface(this.entity,·iid);
|    | [NORMAL] JSHintBear:
|    | 'cmpRanged' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5648| »   »   var·range·=·iid·!==·IID_Attack·?·cmpRanged.GetRange()·:·cmpRanged.GetFullAttackRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5649| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5659| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5662| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.
Executing section cli...

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

elexis added a subscriber: elexis.Sep 12 2019, 11:59 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
774

Duplication should be avoided. I guess the return is not worth to speak of.
It could become if (x) doY(); else doZ(); return;, on the other hand someone could say that its just a coincidence that theres a return in both cases.
Diff is ok.

2002

yess about the &&

We had an unwritten law (as in someone had a strong opinion) on whitespace for the secnod line of if statements:

\tif (x &&
\t    z)

That's same amount of tabs and 4 spaces (i.e. same character count as if ().

I doubt one can make an ESlint rule for that.

I guess I dont care if I commit the tab or 4 spaces.

Aside, I would recommend to put every of these conjunctions (https://en.wikipedia.org/wiki/Conjunctive_normal_form) on a separate line, so it's easier to view the structure:

		if (this.order.data.attackType == "Capture" &&
			(this.GetStance().targetAttackersAlways || !this.order.data.force) &&
			this.order.data.target != msg.data.attacker &&
			this.GetBestAttackAgainst(msg.data.attacker, true) != "Capture")
3307

Unneeded parentheses (no ESlint warning about that by any chance?)

It warns about ? true : false? Nice.

I remember someone not liking the !! way (I liked it because it made the code much shorter in some places), perhaps one can use != undefined, but then one would have to make sure that only falsy values equal to undefined are passed (which probably is the case but I wouldnt want to rely on assumptions) , whatever

3565

Wheres that hash coming from? "Markdown-esque", I see.
Well, coding conventions say something wise - to do things consistenly in the file, then consitently in the folder, then consistently globally.
I never saw this before, so I'd recommend a simple // Foo.

Krinkle marked an inline comment as done.Sep 13 2019, 12:08 AM
Krinkle added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
3307

I'll remove the parenthesis for now.

As follow-up later:

  1. Look into a rule for redundant parenthesis.
  2. Analyse this property across the code base and see if we can turn it into a proper boolean, or some other strict subset of values that can be validated without needing to rely on type casting (e.g. only undefined or truthy value).
3565

Sounds good to me.

Krinkle planned changes to this revision.Sep 13 2019, 12:08 AM
Krinkle updated this revision to Diff 9749.Sep 14 2019, 6:49 AM

Removed redundant parenthesis.

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

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

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/UnitAI.js
| 331| »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'Order.WalkToTarget' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
| 916| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 941| »   »   »   "enter":·function(msg)·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 991| »   »   »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'enter' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
|1054| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1090| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1122| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1282| »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1339| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1514| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1536| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1568| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1722| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1772| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1850| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2027| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2143| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2418| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2451| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2557| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2623| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2662| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2873| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3054| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3781| »   var·isWorkType·=·type·=>·type·==·"Gather"·||·type·==·"Trade"·||·type·==·"Repair"·||·type·==·"ReturnResource";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4628| »   var·target·=·ents.find(target·=>·this.CanAttack(target));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4643| »   var·target·=·ents.find(target·=>
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4689| »   var·ent·=·ents.find(ent·=>·this.CanHeal(ent));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4712| »   »   ····cmpAttack.GetAttackTypes().some(type·=>·cmpUnitAI.CheckTargetAttackRange(this.isGuardOf,·type)))
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3743| »   »   var·order·=·{·"type":·type,·"data":·data·};
|    | [NORMAL] JSHintBear:
|    | 'order' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|3812| »   for·(var·i·=·0;·i·<·this.orderQueue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5549| »   var·targets·=·this.GetTargetsFromUnit();
|    | [NORMAL] JSHintBear:
|    | 'targets' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5550| »   for·(var·targ·of·targets)
|    | [NORMAL] JSHintBear:
|    | 'targ' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5556| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(targ,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5557| »   »   »   var·targetClasses·=·this.order.data.targetClasses;
|    | [NORMAL] JSHintBear:
|    | 'targetClasses' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5637| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5640| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5645| »   »   var·cmpRanged·=·Engine.QueryInterface(this.entity,·iid);
|    | [NORMAL] JSHintBear:
|    | 'cmpRanged' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5648| »   »   var·range·=·iid·!==·IID_Attack·?·cmpRanged.GetRange()·:·cmpRanged.GetFullAttackRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5649| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5659| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5662| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.
Executing section cli...

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

Krinkle marked an inline comment as done.Sep 14 2019, 6:57 AM
Krinkle added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
2002

(Done)

On a related note, I think this makes a good case for always including curly braces.

In the following it is not obvious at first that the last line is not part of the conditional.


"Attacked": function(msg) {
	// If we are capturing and are attacked by something that we would not capture, attack that entity instead
	if (this.order.data.attackType == "Capture" &&
		(this.GetStance().targetAttackersAlways || !this.order.data.force) &&
		this.order.data.target != msg.data.attacker &&
		this.GetBestAttackAgainst(msg.data.attacker, true) != "Capture")
		this.RespondToTargetedEntities([msg.data.attacker]);
	},
Krinkle updated this revision to Diff 9750.Sep 14 2019, 6:58 AM
Krinkle marked an inline comment as done.

Added line breaks in a multi-line conditional.

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

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

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/UnitAI.js
| 331| »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'Order.WalkToTarget' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
| 916| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 941| »   »   »   "enter":·function(msg)·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 991| »   »   »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'enter' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
|1054| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1090| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1122| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1282| »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1339| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1514| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1536| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1568| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1722| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1772| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1850| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2029| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2145| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2420| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2453| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2559| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2625| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2664| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2875| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3056| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3783| »   var·isWorkType·=·type·=>·type·==·"Gather"·||·type·==·"Trade"·||·type·==·"Repair"·||·type·==·"ReturnResource";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4630| »   var·target·=·ents.find(target·=>·this.CanAttack(target));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4645| »   var·target·=·ents.find(target·=>
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4691| »   var·ent·=·ents.find(ent·=>·this.CanHeal(ent));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4714| »   »   ····cmpAttack.GetAttackTypes().some(type·=>·cmpUnitAI.CheckTargetAttackRange(this.isGuardOf,·type)))
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3745| »   »   var·order·=·{·"type":·type,·"data":·data·};
|    | [NORMAL] JSHintBear:
|    | 'order' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|3814| »   for·(var·i·=·0;·i·<·this.orderQueue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5551| »   var·targets·=·this.GetTargetsFromUnit();
|    | [NORMAL] JSHintBear:
|    | 'targets' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5552| »   for·(var·targ·of·targets)
|    | [NORMAL] JSHintBear:
|    | 'targ' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5558| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(targ,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5559| »   »   »   var·targetClasses·=·this.order.data.targetClasses;
|    | [NORMAL] JSHintBear:
|    | 'targetClasses' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5639| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5642| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5647| »   »   var·cmpRanged·=·Engine.QueryInterface(this.entity,·iid);
|    | [NORMAL] JSHintBear:
|    | 'cmpRanged' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5650| »   »   var·range·=·iid·!==·IID_Attack·?·cmpRanged.GetRange()·:·cmpRanged.GetFullAttackRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5651| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5661| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5664| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.
Executing section cli...

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

Stan added a subscriber: Stan.Sep 14 2019, 12:12 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
3307

Natural behaviour is an optionnal a choice tag.

Which means it can either be present or not

<UnitAI>
  <NaturalBehaviour/>
</UnitAI>

Here


this.template.NaturalBehaviour === undefined //true

"NaturalBehaviour" in this.template == true // true

I think there is no check for whether text in there is correct.

<UnitAI>
   <NaturalBehaviour>defensive<NaturalBehaviour/>
</UnitAI>

this.template.NaturalBehaviour === undefined //false

this.template.NaturalBehaviour.length === "defensive".length // true

"NaturalBehaviour" in this.template == true // true
elexis added inline comments.Sep 14 2019, 2:21 PM
binaries/data/mods/public/simulation/components/UnitAI.js
2002

I suppose anyone who really wants to learn the content of the line will discover it very quickly, since every if statement has a consequent, so it can only be the last one.

For these cases it certainly isnt controversial to add the curly braces.
But that example doesnt compare to the many conditions that have only one or two lines.

2730–2733

(I'm a fan of having each property on a separate line if it's more than one property, so that one can see all property names without having to read the values for instance.)

3307

!== undefined might more reflect the intended meaning.

We can make sure that its never false, null or 0.
Its defined in
m_ScriptInterface.SetProperty(m_Instance, "template", paramNode, true, false);
ending up in
ScriptInterface::ToJSVal<CParamNode>
then
CParamNode::ToJSVal
then
CParamNode::ConstructJSVal.

So as far as I see the object is a string or undefined, but never 0, null, or false.

Add to that the circumstancial evidence of all those .template == "false" and related boolean checks, for example
this.template.Unhealable == "true"
and the number conversions with + such as:
halfSize = Math.max(+template.get("Footprint/Square/@depth"), +template.get("Footprint/Square/@width")) / 2;
and undefined checks such as
if (this.template.Distance.MinDistance !== undefined)
So it would be even more consistent to use the undefined check isntead of !!, at least for simulation templates.

3565

ping

3909–3913

(same)

3958

(same)

5756

(I suppose this should be a vector some time)

Stan added inline comments.Sep 14 2019, 6:54 PM
binaries/data/mods/public/simulation/components/UnitAI.js
5756

Would that be a 3D one with a useless y component or a 2D with y used as z ?

Krinkle marked an inline comment as done.Sep 14 2019, 7:02 PM
Krinkle added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
3307

Those are nominal changes. I'm quite interested in making the XML to JS translation of some of these values more strict and easier to use (in particular around boolean values), but would be okay to handle that separately?

Krinkle marked 5 inline comments as done.Sep 14 2019, 7:57 PM
Krinkle added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
2730–2733

Done. I changed it for this line, and also for a few other ones in the same file where an object used more than ~ 100 chars and had multiple keys.

Krinkle updated this revision to Diff 9766.Sep 14 2019, 7:57 PM
Krinkle marked an inline comment as done.
Krinkle marked an inline comment as done.

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

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

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

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

elexis added inline comments.Sep 20 2019, 6:43 PM
binaries/data/mods/public/simulation/components/UnitAI.js
2000

Does the linter allow us to make the last tab for those 3 lines 4 spaces?

3307

Using` !== undefined` instead of !! in this line would be more consistent with the rest of the template parsing, no?

4685

That ); on a separate line looks wrong to me, doesn't it for you?
I understand why scopes, objects and arrays are on a separate line, but not these ones?
(I suppose there is no linter rule for that?)

Krinkle added inline comments.Sep 22 2019, 10:04 PM
binaries/data/mods/public/simulation/components/UnitAI.js
2000

No, it does not. Outside strings and comments, the only place where spaces are allowed for indentation is for subsequent lines of a block comment:

\t /**
\t _ * …
\t _ * …
\t _ */

I do personally dislike that the conditional body and conditions align directly. There's a number of different styles to avoid that, though. The easiest one would be to insert the braces which helps set the two apart.

Before
	"Attacked": function(msg) {
		if (this.order.data.attackType == "Capture" &&
			(this.GetStance()..force) &&
			this.order.data.target != msg.data.attacker &&
			this.GetBestAttackAgainst(msg,) != "Capture")
			this.RespondToTargetedEntities([msg.data.attacker]);
	},
After
	"Attacked": function(msg) {
		if (this.order.data.attackType == "Capture" &&
			(this.GetStance()..force) &&
			this.order.data.target != msg.data.attacker &&
			this.GetBestAttackAgainst(msg.) != "Capture")
		{
			this.RespondToTargetedEntities([msg.data.attacker]);
		}
	},
3307

It casts to a boolean on this line in trunk, and this patch currently does not change that.

Is it okay if I audit those in a separate patch so that we can isolate those and the testing required for them?

4685

I generally follow a rule that closing symbols must align in indentation with opening symbols. This makes it possible to read large amounts of code faster and/or with greater accuracy due to there being less visual ambiguity.

In this case these three lines together make up the body of the lambda passed to Array#find.

Removing the line break would make it hard to distinguish whether between find() && … ( || ) and find( => … && … ( || ) ).

The idea of aligning with the opening symbol was already present here before my change, and indeed to my knowledge there is no rule available to require or disallow that. It's a judgement call.

elexis added inline comments.Sep 25 2019, 12:38 PM
binaries/data/mods/public/simulation/components/UnitAI.js
2000

It's more that the conditions of the if align with the if.

I suppose the linter policy derives from our coding conventions, and the code derives from our coding conventions, not the coding conventions from the linter.

This was objected in several instances before, http://irclogs.wildfiregames.com/

2016/2016-01/2016-01-02-QuakeNet-#0ad-dev.log:17:47 <@leper> use spaces to align the condition for that while if you have to
2016/2016-01/2016-01-19-QuakeNet-#0ad-dev.log:19:06 <@leper> elexis: one tab and align with spaces
2016/2016-03/2016-03-15-QuakeNet-#0ad-dev.log:18:03 <@leper> 1 tab, spaces to align
2016/2016-04/2016-04-12-QuakeNet-#0ad-dev.log:15:50 <@leper> elexis: the second line of the if should use spaces to align with the first line
2016/2016-05/2016-05-17-QuakeNet-#0ad-dev.log:20:49 < elexis> leper: there are so many spaces that the second line of the if statement has more indentation than the return, so it doesnt look like its aligned

Ive discussed it with Philip in 2016 too and then became convinced that it's okay to align the conditions of the if statement with spaces.
So I don't really see why I'd change my mind just because some linter software doesn't support this.

If we decide to drop that style, then we can probably identify all cases and change all cases in one step.

I find 319 matches in JS files for (1 tab + 4 space) and 416 for 1 tab + 3 space.
There are less than 200 in cpp files.

So we should decide one way or the other, but what the linter supports sounds like something that shouldn't be decisive, the linter should adapt to whatever we come up with.

3307

return this.template.NaturalBehaviour !== undefined; also returns a boolean.
The last line is what should be here, so it should be changed to that, and Im not making two commits for this style change.
So if we are uncertain that the proposed change is correct, then we should work on that.
I've posted the steps to reproduce that information above already.
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/simulation2/system/ParamNode.cpp

4685

I understand the idea, I committed some of them too some day ago, but I've changed my mind, because those aren't objects, aren't arrays, aren't function scopes. It doesn't make it any less readable if the closing parentheses is the same line as the function call.

It would also allow for var x = func(\n\tbar\n);`, but it's not a space that is opened like it is with objects, arrays or function scopes where one can insert arbitrary elements. meh

Krinkle marked 2 inline comments as done and an inline comment as not done.Sep 25 2019, 8:56 PM
Krinkle added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
2000

I agree about code and convention first, linter second.

I'm bringing it up because it's not entirely arbitrary. The vast majority of JavaScript code in the world, as developed professionally by people who work on it daily and whom might be interested in contributing here, they generally interpret the language and stylistically prefer one of the ways provided by the linter. Because they too held code first and linter second, and implemented the rule in question.

At this point it's a trade off to be made:

  • Cost and impact of not having an indentation rule (sloppy code, wasted review efforts on telling people to line up code in a for JS-wise unusual manner, and inability for 0AD contributors to have code automatically formatted in their IDE or text editor through ESLint plugin).
  • Loss in value over our belief of readability for this particular change (it's not about all rules and all conventions, it's about this particular convention, how much value it brings to us and how bad/good the alternative is).
  • Loss in value over having different conventions in CPP and JS. Personally, I think it's manageable to diverge a little bit, they are after all very different languages. Being too strict on requiring similarity will result in illusions taking form that make code look like it behaves a certain way when ti doesn't to please a similarity with an unrelated programming language. Might not be the case here, but something to keep in mind more generally.
  • Alternatively - Cost of implementing and maintaining our own ESLint plugin. Not likely to be very rewarding if done locally, but perhaps something upstream is interested in accepting.

What is your recommendation for the short-term, to get us to a point where ESLint is warning-free? Disable the rule in this file, directory or block? Change indentation?

3307

I don't mind changing it to !== undefined if you're comfortable with that and confident that this change in behaviour is not a problem. Boolean casting to false accepts many more values than merely undefined. I currently do not know this code base well enough to confidently say the difference won't be observable and cause bugs. Hence I would lean towards doing that code change separate from a style change.

But if we know where it's used, okay with spending time on testing it as part of review, I will update it then.

I optimise my patches for needing very little testing from you because my short-term goal is not to change the code behaviour, but to make it pass the linter, so that it can start providing useful feedback about real changes in behaviour and slowly raising our bar of standard quality. This is only the first phase to clear out the noise. But I am getting used to now that you either don't trust that style changes won't cause bugs and want to test extensively regardless, or that you prefer to do it all in one go within a given area of code. That's not how I usually work, but it's no problem, I'll adapt :)

Krinkle marked 4 inline comments as done.Sep 29 2019, 1:34 AM
Krinkle added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
3307

I've done a full audit and indeed NaturalBehaviour is only ever assigned a string, and thus yields the undefined value is accessed otherwise.

3311

This IsAnimal() check appears redundant. Would it be okay to remove as part of this change?

Krinkle updated this revision to Diff 9998.Sep 29 2019, 1:35 AM

Updated IsAnimal to use stricter !== undefined check instead of bool cast.

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

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

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/UnitAI.js
| 331| »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'Order.WalkToTarget' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
| 928| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 953| »   »   »   "enter":·function(msg)·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1003| »   »   »   »   »   return·true;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'enter' expected no return value.

binaries/data/mods/public/simulation/components/UnitAI.js
|1066| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1102| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1134| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1298| »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1355| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1530| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1552| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1584| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1738| »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1788| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|1866| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2045| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2161| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2439| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2472| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2578| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2644| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2683| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|2897| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3078| »   »   »   »   "enter":·function()·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of method 'enter'.

binaries/data/mods/public/simulation/components/UnitAI.js
|3811| »   var·isWorkType·=·type·=>·type·==·"Gather"·||·type·==·"Trade"·||·type·==·"Repair"·||·type·==·"ReturnResource";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4666| »   var·target·=·ents.find(target·=>·this.CanAttack(target));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4681| »   var·target·=·ents.find(target·=>
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'target' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4727| »   var·ent·=·ents.find(ent·=>·this.CanHeal(ent));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|4750| »   »   ····cmpAttack.GetAttackTypes().some(type·=>·cmpUnitAI.CheckTargetAttackRange(this.isGuardOf,·type)))
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'type' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/UnitAI.js
|3773| »   »   var·order·=·{·"type":·type,·"data":·data·};
|    | [NORMAL] JSHintBear:
|    | 'order' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|3842| »   for·(var·i·=·0;·i·<·this.orderQueue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5587| »   var·targets·=·this.GetTargetsFromUnit();
|    | [NORMAL] JSHintBear:
|    | 'targets' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5588| »   for·(var·targ·of·targets)
|    | [NORMAL] JSHintBear:
|    | 'targ' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5594| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(targ,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5595| »   »   »   var·targetClasses·=·this.order.data.targetClasses;
|    | [NORMAL] JSHintBear:
|    | 'targetClasses' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5675| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5678| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5683| »   »   var·cmpRanged·=·Engine.QueryInterface(this.entity,·iid);
|    | [NORMAL] JSHintBear:
|    | 'cmpRanged' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5686| »   »   var·range·=·iid·!==·IID_Attack·?·cmpRanged.GetRange()·:·cmpRanged.GetFullAttackRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5687| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5697| »   »   var·cmpVision·=·Engine.QueryInterface(this.entity,·IID_Vision);
|    | [NORMAL] JSHintBear:
|    | 'cmpVision' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|5700| »   »   var·range·=·cmpVision.GetRange();
|    | [NORMAL] JSHintBear:
|    | 'range' is already defined.
Executing section cli...

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