-
Notifications
You must be signed in to change notification settings - Fork 227
Add support for callbacks on config change #912
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
Conversation
💚 Build SucceededExpand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
|
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.
Looks like a clean solution!
I was wondering if it would be possible to use validators for the callback functionality, but this is much better.
Also added a _dict_key_lookup to _ConfigBase to allow for easy access to _ConfigValue instances via dict_key
@beniwohli Something I realized while trying to figure out how to test this today -- callbacks will also be called on initial Config creation, for any non-default values. I think this is probably ok and expected behavior? But it's something to note, and I wondered if you saw any issues with that. I feel like any operation you want to perform on config change probably needs to be performed on initialization as well, if the config value is different than the default value, but I'm not certain. |
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.
Awesome work! Thanks for taking the time to write those docs!
* Add support for callbacks on config value change * Fix VersionedConfig.reset() Also added a _dict_key_lookup to _ConfigBase to allow for easy access to _ConfigValue instances via dict_key * Remove unused variable, and use compat.iteritems * Use default values for old_value comparison in callbacks * Add remote config callback test * Add to changelog * Workaround for no nonlocal keyword in py2 * Add nonlocal note * Add a reset() callback test, plus a typo fix
* Add support for callbacks on config value change * Fix VersionedConfig.reset() Also added a _dict_key_lookup to _ConfigBase to allow for easy access to _ConfigValue instances via dict_key * Remove unused variable, and use compat.iteritems * Use default values for old_value comparison in callbacks * Add remote config callback test * Add to changelog * Workaround for no nonlocal keyword in py2 * Add nonlocal note * Add a reset() callback test, plus a typo fix
* Add support for callbacks on config value change * Fix VersionedConfig.reset() Also added a _dict_key_lookup to _ConfigBase to allow for easy access to _ConfigValue instances via dict_key * Remove unused variable, and use compat.iteritems * Use default values for old_value comparison in callbacks * Add remote config callback test * Add to changelog * Workaround for no nonlocal keyword in py2 * Add nonlocal note * Add a reset() callback test, plus a typo fix
What does this pull request do?
Adds the ability to specify one or more "callback" functions which will be called when a
_ConfigValue
is changed.Related issues
closes #908