I don't see a valid reason that Promotion should be an exception, but I could have missed it.
Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22753: Make Promotion.js use the common Transform helper, add resource gatherer and…
- Trac Tickets
- #4334
I anybody knows of said good reason, please comment. Otherwise this is relatively straightforward.
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
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/1843/display/redirect
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)
binaries/data/mods/public/simulation/components/Promotion.js | ||
---|---|---|
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 |
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.
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... binaries/data/mods/public/simulation/components/tests/test_Pack.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. binaries/data/mods/public/simulation/helpers/Transform.js | 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: https://jenkins.wildfiregames.com/job/docker-differential/414/display/redirect
binaries/data/mods/public/simulation/components/Promotion.js | ||
---|---|---|
38 ↗ | (On Diff #9381) | Capital S. |
binaries/data/mods/public/simulation/components/tests/test_Promotion.js | ||
6 ↗ | (On Diff #9381) | Any reason for using this syntax ? |
8 ↗ | (On Diff #9381) | Block missing indent ? |
binaries/data/mods/public/simulation/helpers/Transform.js | ||
79 ↗ | (On Diff #9381) | missing space |
87 ↗ | (On Diff #9381) | typo in resources :) can probably be inlined. |