Make auto-detecting permitted next nodes the default behaviour #2374
Conversation
|
I'm going to rebase this against master to resolve the conflicts that have been introduced by recent changes in master and then force-push it. |
floehopper
force-pushed
the
make-autodetecting-permitted-next-nodes-the-default
branch
from
ddf43bf to
6105b1e
Compare
|
Rebasing & force-pushing done. Conflicts resolved. Ready for review again. |
| @@ -8,7 +8,7 @@ class QuestionBaseTest < ActiveSupport::TestCase | |||
| context '#next_node' do | |||
| should 'raise exception if called with block but no permitted next nodes' do | |||
| e = assert_raises(ArgumentError) do | |||
| @question.next_node do | |||
| @question.next_node(permitted: []) do | |||
| :done | |||
There was a problem hiding this comment.
should this line be?
outcome :doneThere was a problem hiding this comment.
No, although I see why you ask. This is still testing the "old" mechanism which doesn't require the wrapping of the result with question or outcome. I hope to remove this functionality soon.
|
Even though this code doesn't change the outward appearance of any page, it might be useful to add a before and after of what a |
|
I've edited the PR description to explain the internal changes to the flow DSL. |
| end | ||
| assert_equal [:done, :another_question], @question.permitted_next_nodes | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Minor: In the context description, when I read "not permitted option specified" I'm looking for the 'not permitted' option, but I guess it means without a permitted option.
Minor: In the should description, I'm not sure I'm aware of what syntactic sugar methods relates to. I think a more obvious expectation would be that it expects to return a list of nodes from the outcomes and questions defined within the block.
There was a problem hiding this comment.
Regarding your first comment - that was a typo - well spotted! Fixup commit: 30d92cc.
There was a problem hiding this comment.
Regarding your second comment - I've tried to improve the test name. Fixup commit: 7c9ea42.
|
@erikse: Thanks for reviewing. I've addressed your comments. Are you happy for me to merge this? |
|
Yep, this looks really good |
|
Thanks. I'll get it rebased, force-pushed & merged. |
floehopper
force-pushed
the
make-autodetecting-permitted-next-nodes-the-default
branch
from
7c9ea42 to
e19a506
Compare
…ext-nodes-the-default Make auto-detecting permitted next nodes the default behaviour
floehopper
deleted the
make-autodetecting-permitted-next-nodes-the-default
branch
In #2374 automatic detection of permitted next nodes was made the default and all the places where `next_node` was called with explicitly specified permitted next nodes were converted to use this syntax. This commit removes the `permitted` option altogether which should prevent any regressions from creeping in.
In #2368 & #2370, all occurrences of
next_nodewith a block were converted to use the automatic detection of permitted next nodes instead of explicitly supplying them. So we can now make the code less verbose by makingpermitted: :autothe default arguments fornext_nodewith a block.Note that the old mechanism is still available, it's just no longer the default behaviour.
I've updated the documentation to reflect this change, noting that the old approach is now deprecated.
Motivation
This is another small step towards simplifying the flow DSL and in particular the code in
SmartAnswer::Question::Base.Internal changes
Before
After
The
permitted: :autois no longer necessary:External changes
This is just a refactoring - there should be no changes in externally observable behaviour.