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

Clean up PrepareOptions semantics #10387

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mrambacher
Copy link
Contributor

Clean up PrepareOptions semantics:

  • If an option has not changed (via Configure), do not run Prepare on it

Clean up PrepareOptions semantics:
- If an option has not changed (via Configure), do not run Prepare on it
PrepareOptions is now called under fewer circumnstances.

- If invoke_prepare_options = false, it is not called during the configuration process
- If invoke_prepare_options = true, PrepareOptions will only be called for the base object (the one invoking Configure) and on objects whose configuration was changed by this invocation.  In other words, if the configuration of an object has not changed, it will not be prepared again.
-f invoke_prepare_options = true and the option itself is marked as "do not prepare", it will not be prepared even if its configuration has changed.
@mrambacher mrambacher changed the title WIP: Clean up PrepareOptions semantics Clean up PrepareOptions semantics Jul 29, 2022
@mrambacher mrambacher requested review from pdillinger and ajkr July 29, 2022 16:29
@mrambacher
Copy link
Contributor Author

I ran this against #10375, and reverted #10378, and do not see the issues reported.

There is one potential "extra" PreparedOptions case still to clean up -- if passed in a "bad option" and we attempt to revert to the original, that I will attempt to fix in an upcoming PR.

@mrambacher
Copy link
Contributor Author

@pdillinger @ajkr Any chance of a review? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants