widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #01189
Re: [Merge] lp:~widelands-dev/widelands/autosave_on_objectives into lp:widelands
Review: Needs Fixing
I pushed my review of this branch in r6613. You can find all my comments by grep-ing for #cghislai. I changed some nits around (see the diff) and added a short comment when I found it appropriate - feel free to just delete them again, they are for your eyes only :). I did not touch anything that would affect the logic because you are more suited to do this as you know your code better.
I think the code looks very reasonable already. The biggest issues is that we now have wl.Game():save and wl.game.save_game - the first one is only used in the persistence.wmf test (see tests/lua/persistence.wmf/scripting/init.lua). I think you should move your logic to the method that is already there and make sure that its new features are exercised in the persistence tests - comments are in the source code as well.
--
https://code.launchpad.net/~widelands-dev/widelands/autosave_on_objectives/+merge/174546
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/autosave_on_objectives.
References