While wanting to do something with the structree (using D1108 to remove JSON loading),
I noticed this function and some others are copied. Why? Did the reviewer(s) not notice either? Why did I spend my time on this again? :/
Details
Details
- Reviewers
s0600204 - Commits
- rP21022: Unify structree production queue loading duplication.
There is more to clean, but one thing at a time. Notice small differences, one of the four copies uses "technology" instead of "techs". Some use undefined while others an empty array.
Diff Detail
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
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
Comment Actions
Could have done without the passive-aggressive summary and test-plan.
However, the intent of the proposed change is good.
Unfortunately, implementation is flawed - there is a problem with the line indicated. Once fixed, the "Unit Trainers" section on the right-side of the Structure Tree should reappear and start working again.
| binaries/data/mods/public/gui/reference/common/load.js | ||
|---|---|---|
| 112 ↗ | (On Diff #5069) | This line is not correct. |
Comment Actions
Thanks for the review and finding the bug.
I was just pissed that I spent almost an entire development day on this (had removed more duplication but didn't get it to work and started from scratch).
The most crucial questions are:
- Does it limit the template viewer patch (since I saw you changed the affected code)?
- Does it break mods somewhere? (I don't see a reason to however)
| binaries/data/mods/public/gui/reference/common/load.js | ||
|---|---|---|
| 112 ↗ | (On Diff #5069) | Thanks |
Comment Actions
Hmm. Remind me, what's the policy on cleanup patches and the necessity of getting them reviewed? I ask because I do happen to have several cleanup patches for the structree (and files in the gui/reference folder generally) that are currently sitting on my hard-drive that I haven't uploaded because (a) they date from before I was a staff member, and (b) I have concerns about how long I'd have wait to see them even glanced at let alone reviewed.
The most crucial questions are:
- Does it limit the template viewer patch (since I saw you changed the affected code)?
Nope.
- Does it break mods somewhere? (I don't see a reason to however)
It shouldn't. That said, only time (and modder imagination) will tell.
Comment Actions
Thanks for the review!
Hmm. Remind me, what's the policy on cleanup patches and the necessity of getting them reviewed? I ask because I do happen to have several cleanup patches for the structree (and files in the gui/reference folder generally) that are currently sitting on my hard-drive that I haven't uploaded because (a) they date from before I was a staff member, and (b) I have concerns about how long I'd have wait to see them even glanced at let alone reviewed.
In general code quality should be the decisive factor, not the formalities in my opinion. A patch should be an improvement and regressions should be fixed before the release.
There is also a big difference in politeness if you give the possibility to comment on a diff before the commit. (I'm impolite.)
But speaking about actual area of code discussed (gui/reference/), you are the department leader, the author of all the code, so you are the biggest stakeholder there and be able to judge.
I had seen a some things I didn't like (code duplication, using Object.keys too often when an array might have been nicer, fallback and default values, TLDR comments), so if you ask for review for one day before committing I could try to respond.
But I certainly don't have time to perform full reviews (i.e. proofs for code correctness) everywhere. So IMO we need more realistic processes. Try n error.
I can't decide that you can commit things without review, you have to decide yourself if you don't want to ask for the policy makers for permission.
Comment Actions
I intended to replace the manual Aura and Tech JSON file loading in the structree with a call to LoadModificationTemplates from ModificationTemplates.js. In order to do that cleanly, I intended to remove the duplication prior to that (this was the first patch thereof). Did you plan anything similar? Then I'll let you perform instead.