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

Spike: Fail fast on missing methods in SmartAnswer::State object #2071

Closed

Conversation

Projects
None yet
2 participants
@chrisroos
Copy link
Contributor

commented Nov 11, 2015

This has been superseded by PR #2099

This is for discussion only and currently contains 4 failing tests that I've not investigated fully.

The SmartAnswer::State object is an OpenStruct which means that it responds to undefined methods with nil. This can lead to hard to find bugs in next_node blocks (particularly larger ones, e.g. this one in pay-leave-for-parents) as a simple typo will mean that the program runs but doesn't return the desired result.

The main change in this PR overrides SmartAnswer::State#method_missing so that it raises a NoMethodError exception if we try to call a method that's not previously been defined.

# Behaviour before this change
>> state = SmartAnswer::State.new(start_node = nil)
>> state.foo
=> nil
>> state.foo = 'bar'
>> state.foo
=> 'bar'

# Behaviour after this change
>> state = SmartAnswer::State.new(start_node = nil)
>> state.foo
=> NoMethodError: undefined method 'foo' for SmartAnswer::State
>> state.foo = 'bar'
>> state.foo
=> 'bar'

I've also updated all Smart Answer flows where the code was relying on the old behaviour of the State object, by explicitly setting the variables to nil that are used later in the flow.

I'm not 100% convinced about this but would like to hear what other people think.

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2015

I think this is a worthwhile change. It's not ideal that we have to introduce so many missing variables, but then that's kind of the point! Is that what you are unconvinced about?

It would be good to implement respond_to? or respond_to_missing? to match method_missing.

Ideally, I think each flow would automatically instantiate a flow-specific "model" class with the relevant methods defined on it. Then we'd get this kind of behaviour for free. But I think this is a good first step.

@chrisroos chrisroos force-pushed the spike-fail-fast-on-missing-methods-in-state-object branch from 41135bf to c617309 Nov 16, 2015

@chrisroos chrisroos force-pushed the spike-fail-fast-on-missing-methods-in-state-object branch from c617309 to deda27a Nov 16, 2015

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

It would be good to implement respond_to? or respond_to_missing? to match method_missing.

Do you think this is necessary given that we've not had it before now?

Interestingly, it would appear that OpenStruct doesn't define respond_to? or respond_to_missing? (this is against the State object before making the changes in this branch):

>> state = SmartAnswer::State.new(:start_node)
=> #<SmartAnswer::State current_node=:start_node, path=[], responses=[]>
>> state.respond_to?(:foo)
=> false
>> state.respond_to?(:foo=)
=> false
>> state.foo = 'bob'
=> "bob"
>> state.respond_to?(:foo)
=> true
>> state.respond_to?(:foo=)
=> true

chrisroos added some commits Nov 10, 2015

Update question_base_test.rb
Ensure the color variable is set on the state before trying to call it.
Update am_i_getting_minimum_wage_flow_test.rb
Ensure the accommodation_charge variable is set on the state before trying to
call it.
Add State#error and State#response
I'm planning to override `State#method_missing` so that it raises a
`NoMethodError` if you attempt to read an attribute value without first setting
it. We always want to be able to read `error` and `response` irrespective of
whether they've been set.

I initially tried using `attr_reader` but that means that `error` and `state`
are left out of the hash returned from `State#to_hash`. Setting them in this
call to `super` ensures that `#to_hash` works as expected.
Implement State#method_missing
So that it raises a NoMethodError unless the method being called is a writer
(i.e. ends in =).

This should help avoid errors in our code by failing fast if we try to access a
method that's not previously been set on the `State`.

@chrisroos chrisroos force-pushed the spike-fail-fast-on-missing-methods-in-state-object branch from deda27a to c53896a Nov 16, 2015

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

I've rebased this on master, fixed the failing tests and force-pushed.

I've also moved the commit that overrides State#method_missing to the end, so that the tests should be passing for each commit in the branch.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

@floehopper: Are you still happy for us to make this change?

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2015

I guess it's not critical to define respond_to? - I tend to try to always do it whenever I define method_missing. However, I can see here that OpenStruct is already behaving weirdly.

I'm still happy for this change to be merged.

LGTM 👍

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2015

I've tidied this branch up and opened a new pull request (PR #2099). I'm closing this and will merge from the new PR.

@chrisroos chrisroos closed this Nov 17, 2015

@chrisroos chrisroos deleted the spike-fail-fast-on-missing-methods-in-state-object branch Nov 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.