Merge lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands

Proposed by Notabilis
Status: Rejected
Rejected by: Notabilis
Proposed branch: lp:~widelands-dev/widelands/bug-1809838-blackroot-target
Merge into: lp:widelands
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1809838-blackroot-target
Reviewer Review Type Date Requested Status
Toni Förster Disapprove
Review via email: mp+361290@code.launchpad.net

Commit message

Adding missing check whether blackroot flour is required.

Description of the change

Checking in the production program whether blackroot flour is required before grinding blackroot, so the economic target is respected.

It might be that this isn't checked on purpose, since different from corn there is no other use for blackroot. If the check shouldn't be in the production program, the economy target for blackroot flour should be removed instead.

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

Continuous integration builds have changed state:

Travis build 4369. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/472649794.
Appveyor build 4162. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1809838_blackroot_target-4162.

Revision history for this message
Notabilis (notabilis27) wrote :
Revision history for this message
Toni Förster (stonerl) wrote :

LGTM.

The target should stay in I'd say, since it is an second tier ware. And AFAIK currently all second tier wares have economy targets. So why make an exception here?

review: Approve
Revision history for this message
Toni Förster (stonerl) wrote :

I had a conversation with WorldSavior, where he pointed out that Wine also does not have a target setting.

So removing blackroot-flour from the Economy Settings is the appropriate way to fix this.

review: Disapprove

Subscribers

People subscribed via source and target branches

to status/vote changes: