Merge lp:~widelands-dev/widelands/fix_infrastructure_return_values into lp:widelands

[kaputtnik]

While working on a fix of bug 1639514 i found that there was a return value for place_building_in_region() is documented, but it does not return anything.

https://wl.widelands.org/docs/wl/autogen_auxiliary_infrastructure/#place_building_in_region

The problem was that place_building_in_region() calls prefilled_buildings() which does not return the building created whereas this value is returned by place_buildings()

Relevant code of place_building_in_region():
https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/data/scripting/infrastructure.lua#L154

prefilled_buildings() get the building of place_buildings(), but do not return it:
https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/data/scripting/infrastructure.lua#L99

This branch provides a small change, so that both functions returns the placed building. Also some small corrections related to RST.

[kaputtnik (franku)]

Ups, that will not work...

review: Needs Fixing

[kaputtnik (franku)]

That should work now.

review: Needs Resubmitting

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3154. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/339079461.
Appveyor build 2961. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_fix_infrastructure_return_values-2961.

[Klaus Halfmann (klaus-halfmann)]

kaputtnik whats the problem then?
Is it worth, me taking a look?

[kaputtnik (franku)]

There was no real problem, but wrong documentation. Nevertheless this change will make it possible to work with the returned building, e.g. using building.flag for creating roads or query building.descr.

[GunChleoc (gunchleoc)]

Code LGTM, not tested.

review: Approve

[Klaus Halfmann (klaus-halfmann)]

OK I testplayed this for a while,
but I assume nobody activly usee this code.
So there is nothing to find?

kaputtnik: Ith er any way to actually test this, or perhpas a regression test?
If not this can go in.

review: Approve (compile, testplay)

[kaputtnik (franku)]

Klaus, this code is only used in scenarios or campaigns.

I grep'd through the data directory and currently all campaigns which use prefilled_buildings() or place_building_in_region() didn't use any return values. I would have been surprised if they do :-D

The branch linked in bug 1639514 uses the return value from prefilled_buildings():

https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1639514_fix_yellow_player/view/head:/data/campaigns/bar02.wmf/scripting/starting_conditions.lua#L97

For testing one has to write a scenario map. I could do so if it is wanted :-)

[Klaus Halfmann (klaus-halfmann)]

SO actually you next branch will use and then test it.

@bunnybot merge

review: Approve

[GunChleoc (gunchleoc)]

I started all the campaign and tutorial scenarios without problems, so it's OK.

Thanks for fixing :)

BTW we forgot to set a commit message ;)

=== modified file 'data/scripting/infrastructure.lua'
--- data/scripting/infrastructure.lua	2017-06-08 12:37:10 +0000
+++ data/scripting/infrastructure.lua	2018-02-08 16:11:51 +0000
@@ -32,6 +32,7 @@
 --    :arg create_carriers: If this is :const:`true` carriers are created for
 --       the roads. Otherwise no carriers will be created.
 --    :type create_carriers: :class:`boolean`
+
 function connected_road(p, start, cmd, g_create_carriers)
    create_carriers = true
    if g_create_carriers ~= nil then
@@ -96,7 +97,11 @@
 --          :meth:`wl.map.Warehouse.set_workers`.  Note that ProductionSites
 --          are filled with workers by default.
 --    :type b1_descr: :class:`array`
+--
+--    :returns: A table of created buildings
+
 function prefilled_buildings(p, ...)
+   local b_table = {}
    for idx,bdescr in ipairs({...}) do
       local b = p:place_building(bdescr[1], wl.Game().map:get_field(bdescr[2],bdescr[3]), false, true)
       -- Fill with workers
@@ -115,7 +120,10 @@
       -- Fill with wares if this is requested
       if bdescr.wares then b:set_wares(bdescr.wares) end
       if bdescr.inputs then b:set_inputs(bdescr.inputs) end
+
+      table.insert(b_table, b)
    end
+   return b_table
 end
 
 -- RST
@@ -132,12 +140,12 @@
 --    :type building: :class:`string`
 --    :arg region: The fields which are tested for suitability.
 --    :type region: :class:`array`
---    :arg opts:  a table with prefill information (wares, soldiers, workers,
---       see :func:`prefilled_buildings`) and the following options:
---
+--    :arg opts:  A table with prefill information (wares, soldiers, workers,
+--       see :func:`prefilled_buildings`)
 --    :type opts: :class:`table`
 --
---    :returns: the building created
+--    :returns: The building created
+
 function place_building_in_region(plr, building, fields, gargs)
    local idx
    local f
@@ -151,7 +159,7 @@
          args[1] = building
          args[2] = f.x
          args[3] = f.y
-         return prefilled_buildings(plr, args)
+         return prefilled_buildings(plr, args)[1]
       end
       table.remove(fields, idx)
    end
@@ -167,12 +175,13 @@
 -- RST
 -- .. function:: is_building(immovable)
 --
---    Checks whether an immpvable is a finished building, i.e. not
+--    Checks whether an immovable is a finished building, i.e. not
 --    a construction site.
 --
 --    :arg immovable: The immovable to test
 --
 --    :returns: true if the immovable is a building
+
 function is_building(immovable)
    return immovable.descr.type_name == "productionsite" or
       immovable.descr.type_name == "warehouse" or