-
Notifications
You must be signed in to change notification settings - Fork 777
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
Delegation #145
Delegation #145
Conversation
Unit tests are currently failing after an upstream change on the cli. The root cause is that |
@@ -71,6 +71,10 @@ def __init__(self, session=None, interaction_loader=None, | |||
self._interaction_loader = interaction_loader | |||
if interaction_loader is None: | |||
self._interaction_loader = InteractionLoader() | |||
if delegation_loader is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little confusing here especially since a delegation loader could be itself. The docstring may be needed to be updated here. I am not super familiar with the architecture but I am assuming you add the delegation loader if you delegate to a different wizard so if None is provided wouldn't that mean there is no wizard to delegate to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the delegation_loader
an init parameter to facilitate unit testing. Initially, the loader that was passed was always self
which meant that if a wizard would delegate it would always attempt to load the wizard from disk. This allows me to pass a stub/mock for the delegation loader. Using self
as the delegation_loader
is intended to be the default behavior. I can improve the docstring for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Usually it is not the best practice to be adding an extra parameter solely for unit testing. Can you see the parameter ever being used in the future? If not, is there any other way of testing this like could just use the Stage
interface directly since the wizard loader gets injected there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this up. Rather than inject a WizardLoader, I simply register a custom data loader on the session and inject that session instead.
Looks good. Just had a question on the interface. |
Cool. Interface looks much better. 🚢 |
Implemented Wizard Delegation. Wizard specifications can now specify a Retrieval step to delegate like so:
A stage's resolution is no longer a void operation, and the data that would be stored in the environment is now also returned at the end of the stage. Upon executing the final stage the return value from it is returned as the final result of executing the wizard.
This PR also has a couple fixes to old unit tests to increase coverage.