Page MenuHomeWildfire Games

Handle 0 case for template parsing
ClosedPublic

Authored by smiley on Wed, Sep 16, 5:13 PM.

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

smiley created this revision.Wed, Sep 16, 5:13 PM

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

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

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

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

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1550/display/redirect

smiley requested review of this revision.Wed, Sep 16, 5:16 PM
Angen accepted this revision.Wed, Sep 16, 5:54 PM

thank you

binaries/data/mods/public/simulation/ai/common-api/entity.js
36 ↗(On Diff #13491)
This revision is now accepted and ready to land.Wed, Sep 16, 5:54 PM
smiley added inline comments.Wed, Sep 16, 7:50 PM
binaries/data/mods/public/simulation/ai/common-api/entity.js
36 ↗(On Diff #13491)

That doesn't quite preserve the behavior and I get cargo cult programming vibes from it.

First of all, 0 is the only possible falsy value here. Assuming otherwise implies template validation is broken.

Second of all, while null != undefined returns false, null !== undefined returns true, which means that this is legitimizing the existence of an obvious bug by choosing to accept the value null in the cost information and not treating the key as undefined.

I guess this boils down to whether "metal": null should be accepted or rejected. The template schema seems to suggest it should be rejected.

smiley added inline comments.Wed, Sep 16, 7:57 PM
binaries/data/mods/public/simulation/ai/common-api/entity.js
36 ↗(On Diff #13491)

Forgot to put this part in,

What's the direction here? It might seem trivial, but the last question is IMO a pretty important distinction.

Angen added inline comments.Wed, Sep 16, 7:58 PM
binaries/data/mods/public/simulation/ai/common-api/entity.js
36 ↗(On Diff #13491)

For the record, false is also possible. But I undestand your point. Also null should be threated as undefined.

Angen added a comment.Wed, Sep 16, 8:18 PM

I do not see ai getting any valueable information knowing that some was defined as null instead of undefined.
metal: null equals to not including that line at all from simulation point of view so should be the same as `undefined'.

It is not only about cost information. This is generic function. If one would want to fix just cost, one would fix line in cost function as following:
ret[type] = +(this.get("Cost/Resources/" + type) || 0) as other "defaults" are set.

I am also not sure what you mean by direction.

smiley added a comment.EditedWed, Sep 16, 10:16 PM

Right, hence not having null be distinguished from null.

For the focusing of costs, that's just because the subject matter. But thinking in generic terms, there isn't much use having this be a strict check because 0 and false is the only case where this will be issue, where both of them need to be preserved. But, undefined vs null are the same as you just said. I just don't see the point of not treating null as undefined.

This revision was landed with ongoing or failed builds.Sun, Sep 20, 11:49 AM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sun, Sep 20, 11:49 AM