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

Prevent duplicate permitted next nodes #2360

Merged
merged 4 commits into from
Mar 15, 2016

Conversation

floehopper
Copy link
Contributor

Manually specified permitted next nodes

It's possible for a developer to accidentally include the same node key in the list of permitted next nodes multiple times. However, from a visualisation point-of-view, we don't want the same node to appear more than once. There were a couple of duplicates in the existing flows and so I've included commits to remove them.

Automatically detected permitted next nodes

Likewise, when auto-detecting permitted next nodes, it's legitimate for a next_node block to have multiple execution paths which return the same next node key. But we still don't want the same node to appear more than once in the visualisation. I've been working on converting all the existing flows to use the auto-detect mechanism and I know that (without this fix) some of them would indeed generate duplicates like this.

Expected changes

None

@floehopper
Copy link
Contributor Author

It turns out there are a couple of duplicates in the existing flows - I'm going to add a couple of commits to this PR to remove them.

@floehopper
Copy link
Contributor Author

I've now pushed the two extra commits to remove the duplicates and updated the PR description to explain the removal of the duplicates.

@chrisroos
Copy link
Contributor

As far as I can tell, the GraphPresenter already handles the case where there are duplicate next nodes specified in the list of permitted next nodes, i.e. the same node won't appear more than once in the visualisation. I checked this by looking at the visualisation of the two flows updated in this branch:

Having said all that, it certainly does seems more correct for #permitted_next_nodes to return a unique set of nodes. So, this all looks good to me.

@chrisroos chrisroos added the LGTM label Mar 15, 2016
@floehopper
Copy link
Contributor Author

@chrisroos: Thanks. You're right that the GraphPresenter does seem to cope - I think it must be this conditional. However, that behaviour doesn't seem to be tested and the code is pretty hard to follow. And, as you say, I think it makes more sense for #permitted_next_nodes to return a unique set of nodes.

@floehopper
Copy link
Contributor Author

Rebasing against master and force-pushing in preparation for merging.

I think this makes the intent of the tests clearer.

Also I'm planning on adding more tests for `permitted_next_nodes` in
subsequent commits and I think this will be easier with these nested
contexts in place.
* It's possible for a developer to accidentally include the same node key in
the list of permitted next nodes multiple times. However, from a visualisation
point-of-view, we don't want the same node to appear more than once.

* Likewise, when auto-detecting permitted next nodes, it's legitimate for a
`next_node` block to have multiple paths which return the same next node key.
But we still don't want the same node to appear more than once in the
visualisation.
@floehopper floehopper force-pushed the prevent-duplicate-permitted-next-nodes branch from 3f394e2 to b37118d Compare March 15, 2016 10:18
floehopper added a commit that referenced this pull request Mar 15, 2016
…xt-nodes

Prevent duplicate permitted next nodes
@floehopper floehopper merged commit ed2f0f8 into master Mar 15, 2016
@floehopper floehopper deleted the prevent-duplicate-permitted-next-nodes branch March 15, 2016 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants