Review: Approve
You missed one rename :). I did it for you and merged.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
Review: Needs Fixing
Spoke to early. The fix is flawed - it also changes initial_res_amount in a
game. That leads to wrong results if a scenario changes a fields resource
amount. I added a failing test.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team
all right to have common design for both, I the new design will be: change the
initial_res_amount to res_amount only if initial_res_amount would be lower
then new value of res_amount
Agreed?
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands
What happens when amount initial_amount - I dont think is it good aproach
to allow such situation and in addition, there is no set_initial_res_amount
available via LUA - but I will change it, no problem...
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your
@SirVer - I changed it. Test works as well.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
___
Mailing list:
The proposal to merge lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
has been updated.
Status: Needs review = Merged
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
--
Your team Widelands Developers is subscribed to branch
lp
Review: Approve
Looks good now. Merged.
What happens when amount initial_amount - I dont think is it good aproach
to allow such situation and in addition, there is no set_initial_res_amount
available via LUA - but I will change it, no problem...
I understand what you say. If a scripter
@SirVer - I changed the test as you asked
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
___
Mailing list:
great. only the rename from starting_ to initial_ and this is good to go.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
renaming done. I did recursive renaming with sed so quite a lot files has been
changed.
BTW: look at the 'Description of the Change' now, funny :) Or terrible overhead
I can say :)
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers
Review: Needs Fixing
You missed some :). See inline comments.
Diff comments:
=== modified file 'src/editor/map_generator.cc'
--- src/editor/map_generator.cc 2014-09-20 09:37:47 +
+++ src/editor/map_generator.cc 2014-12-01 21:50:51 +
@@ -138,7 +138,7 @@
Do the rename here I'd say.
I dont understand a meaning of this comment. Why should we care that
resource_amount!=starting_resource_amount here. In fact they will probably be
equal. Depending on a map used for testing.
I would then suggest that you test one more value and also test the
@SirVer
so should I remove
:see also: :attr:`resource`
line? There are more such/similar lines in the lua_map.cc though.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
so should I remove :see also: :attr:`resource`
I thought it would not work. Could you doublecheck that the current occurences
are working in https://wl.widelands.org/docs/wl/ ? If so, then leave it in. If
not, please take it out.
--
It works. It is visible in wiki and link also works
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
___
Mailing list:
Review: Approve
LGTM, a couple of nits. Could you fix them.
Also starting_res_amount is a bit cumbersome. What do you guys think of
renaming it to initial_resource_amount throughout the codebase (i.e. also in
cpp?).
Diff comments:
=== modified file 'src/scripting/lua_map.cc'
---
+1 to renaming. Do we want to do that here or in a separate bug? I think doing
it here might be a good idea, so we will have it in before we expose it to the
Lua interface.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is
no problem for me to rename
@SirVer - see answer to one of your comments below in diff.
Diff comments:
=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc 2014-10-27 10:14:10 +
+++ src/scripting/lua_map.cc 2014-11-26 20:51:03 +
@@ -3501,6 +3501,7 @@
details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
The fix is very trivial - one line was added
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1281823 into lp:widelands.
=== modified file
/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1281823 into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev
20 matches
Mail list logo
