widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Can you put the RST comments directly above each method? This will make the code easier to read and avoid accidentally documenting this twice in the future.

I have also added come nits and ideas.

Diff comments:

> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc	2016-10-16 09:31:42 +0000
> +++ src/scripting/lua_map.cc	2016-12-03 23:25:00 +0000
> @@ -3868,6 +3868,41 @@
>  
>     Every Headquarter or Warehouse on the Map is of this type.
>  */
> +
> +/* RST
> +   .. method:: get_ware_policies(which)
> +
> +      Gets the policies how the warehouse should handle the given wares.
> +      The method to handle is one of the strings SP_Normal, SP_Prefer, SP_DontStock, SP_Remove.
> +
> +      :arg which: behaves like for :meth:`HasWares.get_wares`.
> +
> +      :returns: :class:`string` or :class:`table`
> +*/
> +
> +/* RST
> +   .. method:: get_worker_policies(which)
> +
> +      Same as :meth:`Warehouse.get_ware_policies` but for workers.
> +*/
> +
> +/* RST
> +   .. method:: set_ware_policies(which, policy)
> +
> +      Sets the policies how the warehouse should handle the given wares.
> +
> +      :arg which: behaves like for :meth:`HasWares.get_wares`.
> +
> +      :arg policy: the policy to apply for all the wares given in `which`.
> +      :type policy: a string out of "SP_Normal", "SP_Prefer", "SP_DontStock", "SP_Remove".

Can you add a small Lua code example?

> +*/
> +
> +/* RST
> +   .. method:: set_worker_policies(which, policy)
> +
> +      Same as :meth:`Warehouse.set_ware_policies` but for workers.
> +*/
> +
>  const char LuaWarehouse::className[] = "Warehouse";
>  const MethodType<LuaWarehouse> LuaWarehouse::Methods[] = {
>     METHOD(LuaWarehouse, set_wares),
> @@ -3971,7 +4010,95 @@
>  WH_GET(ware, Ware)
>  // documented in parent class
>  WH_GET(worker, Worker)
> -#undef GET
> +#undef WH_GET
> +
> +
> +inline void wh_policy_to_string(lua_State* L, Warehouse::StockPolicy p) {
> +	switch (p) {
> +		case Warehouse::StockPolicy::SP_Normal:

I have actually renamed those to "Warehouse::StockPolicy::kNormal" and made them an enum class in another branch.

https://code.launchpad.net/~widelands-dev/widelands/compiler_warnings_201611/+merge/311329

I think it would be better not to expose such implementations to the Lua interface and just call the strings "normal", "prefer", "dontstock" and "remove".

> +			lua_pushstring(L, "SP_Normal");
> +			break;
> +		case Warehouse::StockPolicy::SP_Prefer:
> +			lua_pushstring(L, "SP_Prefer");
> +			break;
> +		case Warehouse::StockPolicy::SP_DontStock:
> +			lua_pushstring(L, "SP_DontStock");
> +			break;
> +		case Warehouse::StockPolicy::SP_Remove:
> +			lua_pushstring(L, "SP_Remove");
> +			break;
> +	}
> +}
> +
> +inline Warehouse::StockPolicy string_to_wh_policy(lua_State* L, uint32_t index) {
> +	std::string str = luaL_checkstring(L, index);
> +	if (str == "SP_Normal")
> +		return Warehouse::StockPolicy::SP_Normal;
> +	else if (str == "SP_Prefer")
> +		return Warehouse::StockPolicy::SP_Prefer;
> +	else if (str == "SP_DontStock")
> +		return Warehouse::StockPolicy::SP_DontStock;
> +	else if (str == "SP_Remove")
> +		return Warehouse::StockPolicy::SP_Remove;
> +	else
> +		report_error(L, "<%s> is no valid warehouse policy!", str.c_str());
> +}
> +
> +int LuaWarehouse::set_ware_policies(lua_State* L) {
> +	Warehouse* wh = get(L, get_egbase(L));
> +	Warehouse::StockPolicy p = string_to_wh_policy(L, -1);
> +	lua_pop(L, 1);
> +	bool return_number = false;
> +	WaresSet set = parse_get_wares_arguments(L, wh->owner().tribe(), &return_number);
> +	lua_newtable(L);
> +	for (WaresSet::iterator i = set.begin(); i != set.end(); ++i) {
> +		wh->set_ware_policy(*i, p);
> +	}
> +	return 0;
> +}
> +
> +int LuaWarehouse::set_worker_policies(lua_State* L) {
> +	Warehouse* wh = get(L, get_egbase(L));
> +	Warehouse::StockPolicy p = string_to_wh_policy(L, -1);
> +	lua_pop(L, 1);
> +	bool return_number = false;
> +	WorkersSet set = parse_get_workers_arguments(L, wh->owner().tribe(), &return_number);
> +	lua_newtable(L);
> +	const TribeDescr& tribe = wh->owner().tribe();
> +	for (WorkersSet::iterator i = set.begin(); i != set.end(); ++i) {
> +		// If the worker does not cost anything, ignore it
> +		// Otherwise, an unlimited stream of carriers might leave the warehouse
> +		if (tribe.get_worker_descr(*i)->is_buildable()
> +				&& tribe.get_worker_descr(*i)->buildcost().empty()) {
> +			continue;
> +		}
> +		wh->set_worker_policy(*i, p);
> +	}
> +	return 0;
> +}
> +
> +#define WH_GET_POLICY(type, btype)                                                                        \

Could we have just 1 function "get_warehouse_policies" here? if(ware_exists) .... else if(worker_exists) ... else error, like we do when parsing the tribes' production sites. Then we could also have 1 get/set function each in the interface.

> +	int LuaWarehouse::get_##type##_policies(lua_State* L) {                                           \
> +		Warehouse* wh = get(L, get_egbase(L));                                                    \
> +		const Tribes& tribes = get_egbase(L).tribes();                                            \
> +		bool return_number = false;                                                               \
> +		btype##sSet set = parse_get_##type##s_arguments(L, wh->owner().tribe(), &return_number);  \
> +		lua_newtable(L);                                                                          \
> +		if (return_number)                                                                        \
> +			wh_policy_to_string(L, wh->get_##type##_policy(*set.begin()));                    \
> +		else {                                                                                    \
> +			lua_newtable(L);                                                                  \
> +			for (btype##sSet::iterator i = set.begin(); i != set.end(); ++i) {                \
> +				lua_pushstring(L, tribes.get_##type##_descr(*i)->name());                 \
> +				wh_policy_to_string(L, wh->get_##type##_policy(*i));                      \
> +				lua_rawset(L, -3);                                                        \
> +			}                                                                                 \
> +		}                                                                                         \
> +		return 1;                                                                                 \
> +	}
> +WH_GET_POLICY(ware, Ware)
> +WH_GET_POLICY(worker, Worker)
> +#undef WH_GET_POLICY
>  
>  // documented in parent class
>  int LuaWarehouse::get_soldiers(lua_State* L) {
> 
> === modified file 'test/maps/lua_testsuite.wmf/scripting/warehouse.lua'
> --- test/maps/lua_testsuite.wmf/scripting/warehouse.lua	2016-02-17 22:13:21 +0000
> +++ test/maps/lua_testsuite.wmf/scripting/warehouse.lua	2016-12-03 23:25:00 +0000
> @@ -125,6 +125,114 @@
>     assert_equal(20, k.barbarians_lumberjack)
>  end
>  
> +-- ========
> +-- Policies
> +-- ========
> +function warehouse_tests:test_get_ware_policy()
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Normal")
> +   k = self.w:get_ware_policies{"log", "granite"}
> +   assert_equal("SP_Normal", k.log)
> +   assert_equal("SP_Normal", k.granite)
> +   assert_equal(nil, k.coal)
> +   k = self.w:get_ware_policies("all")
> +   assert_equal("SP_Normal", k.log)
> +   assert_equal("SP_Normal", k.granite)
> +   assert_equal("SP_Normal", k.coal)
> +   assert_equal(nil, k.barbarians_lumberjack)
> +end
> +function warehouse_tests:test_get_worker_policy()

Add a blank line between functions.

> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Normal")
> +   k = self.w:get_worker_policies{"barbarians_builder", "barbarians_lumberjack"}
> +   assert_equal("SP_Normal", k.barbarians_builder)
> +   assert_equal("SP_Normal", k.barbarians_lumberjack)
> +   assert_equal(nil, k.coal)
> +   k = self.w:get_worker_policies("all")
> +   assert_equal("SP_Normal", k.barbarians_builder)
> +   assert_equal("SP_Normal", k.barbarians_lumberjack)
> +   assert_equal(nil, k.coal)
> +end
> +function warehouse_tests:test_get_non_existing_policy()
> +   assert_error("ware policy for non existing ware", function()
> +      self.w:get_ware_policy("plastic")
> +   end)
> +   assert_error("worker policy for non existing worker", function()
> +      self.w:get_worker_policy("tree_climber")
> +   end)
> +end
> +
> +function warehouse_tests:test_set_ware_policy()
> +   -- Make sure it is the normal policy until now
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Normal")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
> +   -- Set new policy for wares
> +   self.w:set_ware_policies("log", "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
> +   self.w:set_ware_policies("log", "SP_DontStock")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_DontStock")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
> +   self.w:set_ware_policies("log", "SP_Remove")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Remove")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
> +   self.w:set_ware_policies("granite", "SP_DontStock")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Remove")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_DontStock")
> +   self.w:set_ware_policies({"log", "granite"}, "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Prefer")
> +   self.w:set_ware_policies("log", "SP_Normal")
> +   self.w:set_ware_policies("granite", "SP_Normal")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Normal")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
> +   self.w:set_ware_policies("all", "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("log"), "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("granite"), "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("meat"), "SP_Prefer")
> +   assert_equal(self.w:get_ware_policies("coal"), "SP_Prefer")
> +end
> +function warehouse_tests:test_set_worker_policy()
> +   -- Make sure it is the normal policy until now
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Normal")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
> +   -- Set new policy for workers
> +   self.w:set_worker_policies("barbarians_builder", "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
> +   self.w:set_worker_policies("barbarians_builder", "SP_DontStock")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_DontStock")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
> +   self.w:set_worker_policies("barbarians_builder", "SP_Remove")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Remove")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
> +   self.w:set_worker_policies("barbarians_lumberjack", "SP_DontStock")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Remove")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_DontStock")
> +   self.w:set_worker_policies({"barbarians_lumberjack", "barbarians_builder"}, "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Prefer")
> +   self.w:set_worker_policies("barbarians_builder", "SP_Normal")
> +   self.w:set_worker_policies("barbarians_lumberjack", "SP_Normal")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Normal")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
> +   self.w:set_worker_policies("all", "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Prefer")
> +   assert_equal(self.w:get_worker_policies("barbarians_gardener"), "SP_Prefer")
> +end
> +function warehouse_tests:test_set_non_existing_policy()
> +   assert_error("ware policy for non existing ware", function()
> +      self.w:set_ware_policies("plastic", "SP_Prefer")
> +   end)
> +   assert_error("invalid policy for ware", function()
> +      self.w:set_ware_policies("log", "SP_Burn")
> +   end)
> +   assert_error("worker policy for non existing worker", function()
> +      self.w:set_worker_policies("tree_climber", "SP_Prefer")
> +   end)
> +   assert_error("invalid policy for worker", function()
> +      self.w:set_worker_policies("barbarians_builder", "SP_Burn")
> +   end)
> +end
>  
>  -- =========
>  -- Soldiers
> 
> === added file 'test/maps/ship_transportation.wmf/scripting/test_create_request_while_worker_with_ware_is_in_transit.lua'
> --- test/maps/ship_transportation.wmf/scripting/test_create_request_while_worker_with_ware_is_in_transit.lua	1970-01-01 00:00:00 +0000
> +++ test/maps/ship_transportation.wmf/scripting/test_create_request_while_worker_with_ware_is_in_transit.lua	2016-12-03 23:25:00 +0000
> @@ -0,0 +1,70 @@
> +run(function()
> +   sleep(100)
> +
> +   game.desired_speed = 50 * 1000
> +
> +   create_southern_port()
> +   create_northern_port()
> +
> +   -- create a ready-to-work lumberjack connected to the northern port
> +   local lumberjack_hut = p1:place_building("barbarians_lumberjacks_hut", map:get_field(18, 4), false, true)
> +   if lumberjack_hut.valid_workers then lumberjack_hut:set_workers(lumberjack_hut.valid_workers) end
> +   connected_road(p1, map:get_field(18,5).immovable, "l,l|tl,tr|", true)
> +
> +   -- a long street to capture the woodcutter when his house is burned
> +   connected_road(p1, map:get_field(18,5).immovable, "r,r|r,r|r,tr|tr,tl|l,l|l,l|l,l|", true)
> +
> +   -- plant a tree inside the street to the right of the lumberjack hut
> +
> +   assert_equal(nil, map:get_field(23,3).immovable)
> +   tree = map:place_immovable("oak_summer_old", map:get_field(23,3), "world")
> +   assert_not_equal(nil, map:get_field(23,3).immovable)
> +
> +   -- set logs and woodcutters to "remove from here" in the port
> +   np = northern_port()
> +   np:set_ware_policies("log", "SP_Remove")
> +   np:set_worker_policies("barbarians_lumberjack", "SP_Remove")
> +
> +   -- aaand action!
> +
> +   -- sleep until the tree has been felled
> +   -- comparision is a hack. How do I find out, whether a MapObject (tree) is valid?

LuaImmovableDescription::has_attribute("tree") - this will only match trees that have grown enough to be felled.

> +   while (tree == map:get_field(23,3).immovable) do
> +      sleep(100)
> +   end
> +   print("tree felled")
> +   -- some more sleep since "removal of tree" and "worker has log" are not at the same time
> +   sleep(2000)
> +   -- now the worker should have the tree
> +
> +
> +   lumberjack_hut:destroy()
> +   -- worker will find street and enter port, can't stay there, enters ship
> +   while not (ship:get_workers() == 1) do
> +      sleep(100)
> +   end
> +   assert_equal(1, ship:get_workers())
> +   -- no wares since the worker is still carrying the log
> +   assert_equal(0, ship:get_wares())
> +
> +   -- start to build a lumberjack on the second port while the required log is still in transit
> +   local cons = p1:place_building("barbarians_lumberjacks_hut", map:get_field(17, 17), true, true)
> +   connected_road(p1, map:get_field(18,18).immovable, "l,tl|", true)
> +
> +   -- give the log time to reach the building site
> +   while not (ship:get_workers() == 0) do
> +      sleep(100)
> +   end
> +   sleep(3000)
> +
> +   cons:destroy()
> +   
> +   sleep(6000)
> +   -- check if both lumberjack and log come to rest in the port
> +   -- if it is possible to check for the log in the building site that would be an alternative
> +   assert_equal(1, southern_port():get_wares("log"))
> +   assert_equal(1, southern_port():get_workers("barbarians_lumberjack"))
> +
> +   print("# All Tests passed.")
> +   wl.ui.MapView():close()
> +end)


-- 
https://code.launchpad.net/~widelands-dev/widelands/feature-test-supply-crash/+merge/312422
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/feature-test-supply-crash.