-
Notifications
You must be signed in to change notification settings - Fork 49
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
Modify and improve config change callback system #74
Conversation
@@ -171,18 +171,26 @@ Configuration specific behaviour can be managed by registering a callback to | |||
fire when a given configuration variable is set on the cli. The callback runs | |||
*after* the corresponding environment variables are set. The callback function | |||
is a 3-arity function that gets called with the original key (as a list of |
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 should now be 2-arity?
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.
Nice catch! Thanks!
Good work, I will wait for a rebase to squash some of these commits before going ahead with the merge. |
beab01a
to
3fa2acc
Compare
We no longer pass the flags through to the config change callback, and the callback is executed directly on whichever node(s) are receiving the config change. Additionally, the return format of the config callbacks has changed so that we can display either a text status message for a standard "set", or a table of results when we use the --all flag.
And fix some dialyzer warnings too while we're at it.
3fa2acc
to
ff4591c
Compare
Regardless of whether the callback displays any output, it's better to tell the user exactly what has been done and confirm that the variable has been modified.
Credit to Ian for pointing this out in review comments
ff4591c
to
14e01b0
Compare
@ian-mi Alright, after much git wrangling, I think that's about as clean and tidy as I'm going to get it without having to start manually resolving merge conflicts. GitHub seems to have gotten a bit confused by all my rebases and force pushes, because it's showing the commits out of order now, so be careful to get the correct commit hash when you +1 it for Bors. (Currently 14e01b0). |
looks good, +1 14e01b0 |
I think Bors needs the +1 to be the first thing in the comment and on its own line. Let me try one more time... |
👍 14e01b0 |
Modify and improve config change callback system Reviewed-by: nickelization
@borshop merge |
These changes break backwards compatibility with the old config callback API, but the new system is much easier to use in that it automatically deals with use of the --node and --all flags and calls the callbacks in the correct places. It also allows the callbacks to add output messages that can be displayed back to the user. In the case of the --all flag, all the results will now be displayed back to the user in a table.
This PR also updates the documentation appropriately, and adds extensive unit tests for the clique "set" command.