Page MenuHomeWildfire Games

Cleanup mod page cleanup in r20552
ClosedPublic

Authored by elexis on Nov 29 2017, 10:55 AM.

Details

Summary
  • Fixes the forgotton [0] found by leper that introduces a bug when parsing <= or => (nasty bug because it's not triggered for DE, TM, Vox Populi,and other mods and because it's not directly obvious from reading the code`.
  • Fixes the warn and forgotton regex argument of split I noticed once before and once after the commit.

The other comments in rP20552 are either disagreed upon or not a defect introduced in this commit and can be addressed by anyone in a specific patch, for instance D1093.

Test Plan

See the discussion in that commit in the affected lines.

Event Timeline

elexis created this revision.Nov 29 2017, 10:55 AM
Vulcan added a subscriber: Vulcan.Nov 29 2017, 11:00 AM

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...
Executing section Default...
Executing section Source...
Executing section JS...
elexis updated this revision to Diff 4436.Nov 29 2017, 12:14 PM

Add the removed 12% table width to the description column.

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...
Executing section Default...
Executing section Source...
Executing section JS...
leper requested changes to this revision.Nov 29 2017, 10:17 PM

See comments elsewhere.

I might agree with removing types, but since this is just mixed with unrelated code I'll not spend more time on this.

This revision now requires changes to proceed.Nov 29 2017, 10:17 PM
elexis updated this revision to Diff 4450.Nov 30 2017, 12:33 PM

Reup of the first iteration as the second one unintentionally contained the patch from D1082.

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...
Executing section Default...
Executing section Source...
Executing section JS...
elexis added a comment.Dec 1 2017, 1:41 PM

I'm most embarrassed about the missing [0], because it was an error that doesn't happen with any mods I'm aware of and it's not easily obvious from the code either.

The NaN continue isn't so bad in comparison, because we should do a separate validation in a separate function to begin with.
Since that is an addition, it should be done in a separate diff.
I could have written that yesterday already if we would focus our energy on the partial agreement instead of the partial disagreement.

binaries/data/mods/mod/gui/modmod/modmod.js
353

(The bitter irony is that this and the warn mistake wouldn't have been introduced if I hadn't lost my first version of the patch when performing or responding to an unrelated review.)

leper edited reviewers, added: Itms; removed: leper.Dec 1 2017, 10:13 PM

I could have written that yesterday already if we would focus our energy on the partial agreement instead of the partial disagreement.

Given that you went ahead and decided to commit something broken I'll take the liberty of not doing the work of fixing your breakage. Have fun.

I didn't assign you as a reviewer (and could have fixed my oversights two days ago).
Documentation, NaN and validation could be here D1093.

elexis updated this revision to Diff 4482.Dec 1 2017, 10:20 PM

Only the three oversights that weren't invalid before and do the other things missing properly in the other revisions.

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...
Executing section Default...
Executing section Source...
Executing section JS...
elexis edited the summary of this revision. (Show Details)Dec 2 2017, 2:04 PM
elexis updated this revision to Diff 4506.Dec 3 2017, 2:31 PM

Don't even fix the warn. D1093 will implement complete validation and remove the warn in this place to begin with, just like the NaN.

Vulcan added a comment.Dec 3 2017, 6:18 PM
Executing section Default...
Executing section Source...
Executing section JS...
wraitii accepted this revision.Dec 3 2017, 6:22 PM
Vulcan added a comment.Dec 3 2017, 6:41 PM

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...
Itms accepted this revision.Dec 3 2017, 7:50 PM

This looks good to me and the bug is fixed.

This revision is now accepted and ready to land.Dec 3 2017, 7:50 PM
This revision was automatically updated to reflect the committed changes.