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

Add generic conditional container plugin. #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnfzdz
Copy link

@gnfzdz gnfzdz commented Oct 1, 2022

See #325 for explanation

Not intended as merge worthy, but just something to help visualize the idea. Let me know if it's worth refining.

return True

def _execute_tasks(self, data):
# TODO improve handling of defaults either by reusing context/dispatcher -OR- prepend defaults & extract at end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this currently has the same issue as the similar third-party plugins (scoping defaults to the child tasks). I think the best approach would be to update dispatcher constructor to allow passing in (and therefore reusing) a context.

Other options considered:

  • Reusing the parent dispatcher by exposing it through context
  • Modify the task list by prepending a default directive. After completion, get the child context defaults and call set_defaults on parent context.

In either case, I also think it's worth adding a configuration option to this plugin to control whether defaults are scoped to the child tasks or not.

@kurtmckee
Copy link
Contributor

This is a great start, @gnfzdz! I'd like to assist on implementing a number of initial condition providers, writing up documentation, and adding unit tests. I can pick this up after #313 is reviewed and merged, as it will dramatically affect how the unit tests are implemented (and will make it possible to test the condition providers across all supported operating systems).

@gnfzdz
Copy link
Author

gnfzdz commented Nov 29, 2022

@kurtmckee Thanks for the positive feedback. I can definitely see how this would work better with cross platform support. Thanks for the offer to help?

@anishathalye Have you had a chance to review the proposal? I did release this as a separate plugin, but it would feel a lot cleaner if at least the abstract base Condition was available in the core Dotbot project.

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