Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "enabled" value in placeholder for devices #1372

Merged
merged 2 commits into from May 25, 2019

Conversation

@avanwinkle
Copy link
Collaborator

commented May 20, 2019

This PR reverts a behavior change in #1327 that introduced a bug on Device enabled values in placeholders. I didn't sort out exactly the issue, but I think something was double-escaping the underscored property or doubling the alias lookback. Suffice it to say, event{device.type.devicename.enabled} conditions always evaluated to false, throwing on the DevicePlaceholder not having an "enabled" property.

Changing the placeholder to be event{device.type.devicename._enabled} behaved as expected, but that feels incorrect.

This PR reverts the alias underscore of the "enabled" property for BallHold, MultiBall, and MultiballLock, and adds tests to confirm the expected behavior.

NOTE: BallRouting also includes the "_enabled" change, but I'm not familiar enough with how routing devices work to write tests for them.

@avanwinkle avanwinkle added the bug label May 20, 2019

@avanwinkle avanwinkle requested a review from jabdoa2 May 20, 2019

@@ -10,7 +10,7 @@
from mpf.core.system_wide_device import SystemWideDevice


@DeviceMonitor("balls_held", enabled="_enabled")
@DeviceMonitor("balls_held", "enabled")

This comment has been minimized.

Copy link
@jabdoa2

jabdoa2 May 20, 2019

Collaborator

This will fix evaluations for one shot placeholders. However, it will not allow subsections because the placeholder will not notice changes to the getter. That is the reason why I tried to use the internal variable. Maybe I just inverted the order? It is good that we have tests now. Maybe we can create a generic test which just queries all placeholders to make sure that they do not crash at least?

This comment has been minimized.

Copy link
@avanwinkle

avanwinkle May 20, 2019

Author Collaborator

Hey @jabdoa2 , thanks for the feedback. I tried swapping the positions to `_enabled="enabled" and my new tests still pass, is that sufficient? Could you provide tests for the situation you're describing above?

The trouble with the current bug is that it doesn't actually crash, the placeholder substitution swallows the error and returns None. But I'm all in favor of more tests!

This comment has been minimized.

Copy link
@jabdoa2

jabdoa2 May 22, 2019

Collaborator

This can be tested like this: 1239ef4#diff-e74bb5ba8adfd77096ec130b2124abf4R16. I'm pretty confident that your solution is right!

This comment has been minimized.

Copy link
@avanwinkle

avanwinkle May 22, 2019

Author Collaborator

I've pushed an update that restores the kwargs but swaps the underscore position. All the tests (including my new ones) pass, so... hurray?

@jabdoa2 jabdoa2 merged commit 60d0408 into missionpinball:dev May 25, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.