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

Proposed by kaputtnik
Status: Merged
Merged at revision: 8583
Proposed branch: lp:~widelands-dev/widelands/fix_infrastructure_return_values
Merge into: lp:widelands
Diff against target: 75 lines (+15/-6)
1 file modified
data/scripting/infrastructure.lua (+15/-6)
To merge this branch: bzr merge lp:~widelands-dev/widelands/fix_infrastructure_return_values
Reviewer Review Type Date Requested Status
Klaus Halfmann Approve
GunChleoc Approve
kaputtnik (community) Needs Resubmitting
Review via email: mp+337364@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
kaputtnik (franku) wrote :

Ups, that will not work...

review: Needs Fixing
Revision history for this message
kaputtnik (franku) wrote :

That should work now.

review: Needs Resubmitting
Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

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

Revision history for this message
kaputtnik (franku) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LGTM, not tested.

review: Approve
Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

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)
Revision history for this message
kaputtnik (franku) wrote :

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 :-)

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

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

@bunnybot merge

review: Approve
Revision history for this message
GunChleoc (gunchleoc) wrote :

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 ;)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/scripting/infrastructure.lua'
2--- data/scripting/infrastructure.lua 2017-06-08 12:37:10 +0000
3+++ data/scripting/infrastructure.lua 2018-02-08 16:11:51 +0000
4@@ -32,6 +32,7 @@
5 -- :arg create_carriers: If this is :const:`true` carriers are created for
6 -- the roads. Otherwise no carriers will be created.
7 -- :type create_carriers: :class:`boolean`
8+
9 function connected_road(p, start, cmd, g_create_carriers)
10 create_carriers = true
11 if g_create_carriers ~= nil then
12@@ -96,7 +97,11 @@
13 -- :meth:`wl.map.Warehouse.set_workers`. Note that ProductionSites
14 -- are filled with workers by default.
15 -- :type b1_descr: :class:`array`
16+--
17+-- :returns: A table of created buildings
18+
19 function prefilled_buildings(p, ...)
20+ local b_table = {}
21 for idx,bdescr in ipairs({...}) do
22 local b = p:place_building(bdescr[1], wl.Game().map:get_field(bdescr[2],bdescr[3]), false, true)
23 -- Fill with workers
24@@ -115,7 +120,10 @@
25 -- Fill with wares if this is requested
26 if bdescr.wares then b:set_wares(bdescr.wares) end
27 if bdescr.inputs then b:set_inputs(bdescr.inputs) end
28+
29+ table.insert(b_table, b)
30 end
31+ return b_table
32 end
33
34 -- RST
35@@ -132,12 +140,12 @@
36 -- :type building: :class:`string`
37 -- :arg region: The fields which are tested for suitability.
38 -- :type region: :class:`array`
39--- :arg opts: a table with prefill information (wares, soldiers, workers,
40--- see :func:`prefilled_buildings`) and the following options:
41---
42+-- :arg opts: A table with prefill information (wares, soldiers, workers,
43+-- see :func:`prefilled_buildings`)
44 -- :type opts: :class:`table`
45 --
46--- :returns: the building created
47+-- :returns: The building created
48+
49 function place_building_in_region(plr, building, fields, gargs)
50 local idx
51 local f
52@@ -151,7 +159,7 @@
53 args[1] = building
54 args[2] = f.x
55 args[3] = f.y
56- return prefilled_buildings(plr, args)
57+ return prefilled_buildings(plr, args)[1]
58 end
59 table.remove(fields, idx)
60 end
61@@ -167,12 +175,13 @@
62 -- RST
63 -- .. function:: is_building(immovable)
64 --
65--- Checks whether an immpvable is a finished building, i.e. not
66+-- Checks whether an immovable is a finished building, i.e. not
67 -- a construction site.
68 --
69 -- :arg immovable: The immovable to test
70 --
71 -- :returns: true if the immovable is a building
72+
73 function is_building(immovable)
74 return immovable.descr.type_name == "productionsite" or
75 immovable.descr.type_name == "warehouse" or

Subscribers

People subscribed via source and target branches

to status/vote changes: