Fix issue where yield_to can return too early #455
Conversation
d54a13d to
ea96ad1
Compare
There was a problem hiding this comment.
Good find! I had a bit of trouble following what the issue was, but the test helped. You have some syntax issues and one future proofing issue. Address those and I'll take another look.
P.S. Try running the tests before opening a PR ;). If you are having any issues getting setup to the GUT test suite, let me know. It's all a bit gross using the tool to test the tool.
Thanks for the PR
addons/gut/gut.gd
Outdated
| @@ -61,6 +61,7 @@ const LOG_LEVEL_ALL_ASSERTS = 2 | |||
| const WAITING_MESSAGE = '/# waiting #/' | |||
| const PAUSE_MESSAGE = '/# Pausing. Press continue button...#/' | |||
| const COMPLETED = 'completed' | |||
| const YIELD_FROM_OBJ = {} | |||
There was a problem hiding this comment.
Since 4.0 compares dictionaries by value this should be something else so the bug does not show back up when being ported to 4.0. A Node instance would work if we make this a var instead...which is fine with me. Keep the all-caps so we know it should be constant.
var YIELD_FROM_OBJ = Node.new()
There was a problem hiding this comment.
I guess we don't care about leaking the node since gut is a singleton?
There was a problem hiding this comment.
I figured out you can use a class as a sentinel and then it can be used in places that require a constant expression. I'm not sure there is a better way to create a const sentinal with a unique identity
If the signal emits the value 'false' as the first argument this will assume that it's the bound 'false' value which signifies a timeout.. This is because bound arguments appear at the end of the callback parameter list, not a the beginning. Instead of using true/false, this change binds a sentinel. We can definitively know whether the callback came from the bound signal or a timeout. See also bitwes#327
39eb8b1 to
882e625
Compare
|
This looks good. Thanks for the contribution! |
If the signal emits the value 'false' as the first argument this will assume that it's the bound 'false' value which signifies a timeout.. This is because bound arguments appear at the end of the callback parameter list, not a the beginning.
Instead of using true/false, this change binds a sentinel. We can definitively know whether the callback came from the bound signal or a timeout.
See also #327