(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]);
	},

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.

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.

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).

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

!== 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.

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?

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

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?

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

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 :)

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

Sounds good to me.

ping