Page MenuHomeWildfire Games

Handle 0 case for template parsing
ClosedPublic

Authored by lyv on Sep 16 2020, 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

lyv created this revision.Sep 16 2020, 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

lyv requested review of this revision.Sep 16 2020, 5:16 PM
Silier accepted this revision.Sep 16 2020, 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.Sep 16 2020, 5:54 PM
lyv added inline comments.Sep 16 2020, 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.

lyv added inline comments.Sep 16 2020, 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.

Silier added inline comments.Sep 16 2020, 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.

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.

lyv added a comment.EditedSep 16 2020, 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.Sep 20 2020, 11:49 AM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 20 2020, 11:49 AM