Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-in'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/StatisticsTracker.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/StatisticsTracker.js
| 577| 577| {
| 578| 578| 	if (typeof fromData == "object")
| 579| 579| 		for (let prop in fromData)
| 580|    |-		{
|    | 580|+		
| 581| 581| 			if (typeof toData[prop] != "object")
| 582| 582| 				toData[prop] = [fromData[prop]];
| 583| 583| 			else
| 584| 584| 				this.PushValue(fromData[prop], toData[prop]);
| 585|    |-		}
|    | 585|+		
| 586| 586| 	else
| 587| 587| 		toData.push(fromData);
| 588| 588| };

Link to build: https://jenkins.wildfiregames.com/job/phabricator_lint/1022/display/redirect

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

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

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

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

CMessageTerritoriesChanged is only sent upon MT_Update, i.e. once per turn, whereas PlayerDefeated can be sent in the middle of a turn.
If I understand correctly, the problem is that the autostart subscribes to the playerdefeat event, prints the outdated statistics, but after that the statistics are corrected?
Then we could add a comment stating that it's relevant for that one turn.

New code looks much simpler and less error prone

But is it correct and needed? Was the problem analyzed correctly?

the elexis comment seems simple enough => accept

The whitespace is not so important to me than the open question, otherwise I had accepted this before.

The patch is attractive and I would really like to accept it, but I'm a bit eager to commit a solution without understanding the problem well.

This i guess (i've not checked that part of the code) is because the PlayerDefeated message is treated before the TerritoryChanged one, and thus the peak percentage is not updated for the final gain.

So it is only relevant if the player or the autostart thing ends the game in the middle of a turn, after PlayerDefeated but before TerritoryChanged?
If so, we should add a code comment informing the reader.

In D1294#59071, @bb wrote:

New code looks much simpler and less error prone

the elexis comment seems simple enough => accept

thanks

In D1294#59078, @elexis wrote:

New code looks much simpler and less error prone

But is it correct and needed? Was the problem analyzed correctly?

the elexis comment seems simple enough => accept

The whitespace is not so important to me than the open question, otherwise I had accepted this before.

The patch is attractive and I would really like to accept it, but I'm a bit eager to commit a solution without understanding the problem well.

This i guess (i've not checked that part of the code) is because the PlayerDefeated message is treated before the TerritoryChanged one, and thus the peak percentage is not updated for the final gain.

So it is only relevant if the player or the autostart thing ends the game in the middle of a turn, after PlayerDefeated but before TerritoryChanged?
If so, we should add a code comment informing the reader.

Do you now have analysed it correctly? then you'd realize the comment is not needed. The goal of the patch is to make the result independent of the message orders, independently of all the details you are discussing. Of course (and you can do it if that problem really bothers you), you could change the way autostart games listen to messages so that it is always done at the end of a turn, but that would add complexity and make it less robust imo.

As mentioned I suspect the player can end the game or open the summary screen in the middle of a turn too, so it's not only for autostarted games.
Still think that the first question that a reader of the code has is why it's needed, but I guess he can use archeology too or do some code digging and guessing.
(Otherwise thanks for having implemented my proposals)