Page MenuHomeWildfire Games

Make Promotion.js use the common Transform helper

Authored by wraitii on Jun 29 2019, 5:32 PM.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
rP22753: Make Promotion.js use the common Transform helper, add resource gatherer and…
Trac Tickets

I don't see a valid reason that Promotion should be an exception, but I could have missed it.

Test Plan

I anybody knows of said good reason, please comment. Otherwise this is relatively straightforward.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.Jun 29 2019, 5:32 PM

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

Link to build:

elexis updated the Trac tickets for this revision.Jun 29 2019, 5:34 PM
bb added a subscriber: bb.Jul 1 2019, 12:49 PM

Transform.js got added in rP18467 to enable upgrading, as transform.js is meant to be generic and should be able to perform any change of template, this change makes should make things more clean and easier to handle.

Notice that we now also carry over the capturable data, which is a good thing (it didn't harm anything up to now since there is no unit that can promote and be captured)

65–67 ↗(On Diff #8645)

that seems to be missing in tranform.js

69–75 ↗(On Diff #8645)

same here

87 ↗(On Diff #8645)

we still need that call

Stan added a subscriber: Stan.Jul 1 2019, 1:24 PM

Would be nice to have a test maybe ?

Thanks for giving this a look, indeed we should validate that all checks done here are also done in Transform, and perhaps check which checks done in Transform aren't done here.
The experience value might not need to be passed here depending on how IncreaseXP does things, so we'll need to be careful.

Adding tests for Promotion sounds good. We should also add static checking for serialisation.

wraitii updated this revision to Diff 9381.Aug 18 2019, 4:47 PM

Add Promotion and ResourceGatherer to Transform.js, and rewrite the Promote test to be about promotion, not about Transform.js.

One ought to add integration tests to Transform someday perhaps.

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

Linter detected issues:
Executing section Source...
Executing section JS...

|  47| »   "SetInterval":·(ent,·iid,·funcname,·time,·repeattime,·data)·=>·{·timerActivated·=·true;·return·7;·}
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

| 208| »   »   »   »   for·(let·ent·of·cmpNewObstruction.GetEntitiesDeletedUponConstruction())
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.
Executing section cli...

Link to build:

Stan added inline comments.Aug 18 2019, 6:09 PM
38 ↗(On Diff #9381)

Capital S.

6 ↗(On Diff #9381)

Any reason for using this syntax ?

8 ↗(On Diff #9381)

Block missing indent ?

79 ↗(On Diff #9381)

missing space

87 ↗(On Diff #9381)

typo in resources :) can probably be inlined.

wraitii added inline comments.Aug 22 2019, 7:20 PM
6 ↗(On Diff #9381)

I like it more than writing a function and calling it, but no otherwise.

8 ↗(On Diff #9381)

linter actually complains if I indent.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 22 2019, 7:26 PM
This revision was automatically updated to reflect the committed changes.