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

quick fix: a blank plugin option should be true, not false #3586

Merged
merged 3 commits into from
Mar 21, 2020

Conversation

niftynei
Copy link
Collaborator

Fix a syntactic mistake i made in #3582

Also fix the option interop test so that it actually verifies the log messages.

@niftynei niftynei added this to the 0.8.2 milestone Mar 11, 2020
@niftynei niftynei requested a review from cdecker as a code owner March 11, 2020 20:57
Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Didn't test yet, but we stil use opt_register_arg() to register plugin options. With an empty string passed is it equivalent to opt_unregister_noarg() ?

@rustyrussell
Copy link
Contributor

rustyrussell commented Mar 11, 2020

Yes, as @darosior notes, pretty sure this doesn't work, since --foo will not get a callback with opt_register_arg.

We'd almost need a new type to mean a simple flag argument, say flag? It would use opt_register_noarg instead.

We deliberately don't allow optional arguments, since they're ambigious (is --foo --bar to be interpreted as --foo=--bar or two options?).

a few things. one is that `is_in_log` returns a result rather than
enforcing a condition. so these lines all need asserts

two is that with the 'allow_deprecated_apis' option on, the python json
parser overwrites the now typed input with the later-added string
version, so the only option value present in the option key-value set is
the last, string one. the check for this has been updated to only verify
that the string version is included (i manually verified that both are
printed to the JSON message)
Updates the plugin docs to include more detailed info about how options
work.

Changelog-Added: Plugins: 'flag'-type option now available.
@niftynei
Copy link
Collaborator Author

Didn't test yet, but we stil use opt_register_arg() to register plugin options. With an empty string passed is it equivalent to opt_unregister_noarg() ?

Hm so the tests seem to pass, but I'm not sure that the command line arguments will work the same as the python mediated options? I removed it entirely, I think it's fine to force specification.

We'd almost need a new type to mean a simple flag argument, say flag? It would use opt_register_noarg instead.

This seemed like a fun thing to add, so I went ahead and added this as a type.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack d0aa618

@rustyrussell rustyrussell merged commit 563b468 into ElementsProject:master Mar 21, 2020
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.

3 participants