cln_plugin Refactor ConfigOption
#7042
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We used to store all `ConfigOptions` in a `Vec`. I decided to use a `HashMap` isntead because it will be easier to implement dynamic options later on.
ErikDeSmedt
force-pushed
the
erik/refactor_config_options
branch
2 times, most recently
from
a1485de to
80836f0
Compare
Breaking changes here.
This improves the semantics of `ConfigOption` and `options::Value`
drastically.
We've been using `options::Value` for 2 purposes
1. To specify the type and default value of an option
2. Coummunicate how a user configured an option
We fall here in the pit-fall of being poor at both purposes.
I've edited the code to ensure
- `options::Value` -> To read or specify the value of an option
- `option::ValueType` -> To specify the type of an option
**Configure an option**
Let's create an string-typed option create an option named `"required-string-opt"` the
developer has to use the following code.
```rust
let option = ConfigOption::new("required-string", Value::OptString, "description");
```
The semantics of `OptString` might falsely suggest that it is optional to specify the config.
However, we use `OptString` to say we are not providing a default-value.
After this commit we can use instead
```rust
let option = ConfigOption::new_str_no_default("required-string", "description");
```
For reading a configured value the `option::Value` is somewhat
cumbersome. The old version of had 6 different types of value
```rust
let value = plugin.option("required-string");
match value {
String(s) => {}, // User has configured string value or default
Integer(i) => {}, // User has configured int value or default
Boolean(b) => {}, // User has configured bool value or default
OptString => {}, // User has not configured value and no default
OptInteger = {}, // User has not configured value and no default
OptBOolean => {}, // User has not configured value and no default
}
```
This has been changed to
```rust
let value = plugin.option("required-string");
match value {
Some(String(s)) => {},
Some(Integer(i)) => {},
Some(Boolean(b)) => {},
None => {},
}
```
This is the first part of two commits that attempts to simplify
the API that is used to retrieve configuration values.
Previously, we encouraged the following pattern to build a plugin.
```rust
let configured_plugin =
Builder::new(
tokio::io::stdin(),
tokio::io::stdout())
.option(ConfigOption::new_i64_with_default("some-option", 0, "Description"))
.configure();
let value = configured_plugion.option("some-option");
match value {
Some(Integer(i)) => {}, // Config provided
None => {}, // No config set
_ => {}, // This should never happened
}
```
This commit helps to move to the following pattern
```rust
const SOME_OPTION : ConfigOption<i64> = ConfigOption::new_i64_with_default(
"some-option",
"description");
async fn main() -> Result<()> {
let plugin = Builder::new(tokio::io::stdin(), tokio::io::stdoout())
.option(SOME_OPTION)
.configure()
.await?;
let value : i64 = plugin.option(SOME_OPTION)?;
}
```
In the old version requesting the config-value of an option
was a little bit tricky.
Let's say we want to have a plugin which uses a default
port of 1234.
```rust
let value = plugin.option("plugin-port");
match value {
Some(Value::Integer(_)) => {},
Some(Value::String(_)) => {}, // Can never happen
Some(Value::Boolean(_)) => {}, // Can never happen
None => {}, // Can never happen
}
```
Many users of the `cln_plugin` crate are overly cautious
and handle all these error scenario's. Which is completely unneeded.
Core Lightning will complain if you put a `String` where an `Integer` is
expected and will never send the value to the plug-in.
This change makes the API much more ergonomical and actually motivates
some of the changes in previous commits.
```
const MY_OPTION : ConfigOption<i64> = ConfigOption::new_i64_with_default(
"plugin-port',
1235,
"Description");
let value : Result<i64> = plugin.option(MY_OPTION);
```
The result will provide a proper error-message.
It is also safe to `unwrap` the result because it will
only be triggered if the user neglected to provide the
option to the `Builder`.
ErikDeSmedt
force-pushed
the
erik/refactor_config_options
branch
from
80836f0 to
f57beb2
Compare
cdecker
reviewed
Feb 8, 2024
plugins/src/options.rs
Outdated
Comment on lines
124
to
130
| UntypedConfigOption { | ||
| name: self.name.clone(), | ||
| value_type: self.value_type.clone(), | ||
| default: self.default.map(|i| Value::Integer(i)), | ||
| description: self.description.clone(), | ||
| value: None, | ||
| } |
There was a problem hiding this comment.
Interesting, would you say this is an example of type-erasure? I've been looking for a good example to see why that is useful :-)
cdecker
reviewed
Feb 8, 2024
Comment on lines
+10
to
+61
| //! ```no_run | ||
| //! use anyhow::Result; | ||
| //! | ||
| //! use cln_plugin::ConfiguredPlugin; | ||
| //! use cln_plugin::Builder; | ||
| //! use cln_plugin::options::{StringConfigOption, DefaultStringConfigOption}; | ||
| //! | ||
| //! const STRING_OPTION : StringConfigOption = | ||
| //! StringConfigOption::new_str_no_default( | ||
| //! "string-option", | ||
| //! "A config option of type string with no default" | ||
| //! ); | ||
| //! | ||
| //! const DEFAULT_STRING_OPTION : DefaultStringConfigOption = | ||
| //! DefaultStringConfigOption::new_str_with_default( | ||
| //! "string-option", | ||
| //! "bitcoin", | ||
| //! "A config option which uses 'bitcoin when as a default" | ||
| //! ); | ||
| //! | ||
| //! #[tokio::main] | ||
| //! async fn main() -> Result<()>{ | ||
| //! let configured_plugin = Builder::new(tokio::io::stdin(), tokio::io::stdout()) | ||
| //! .option(STRING_OPTION) | ||
| //! .option(DEFAULT_STRING_OPTION) | ||
| //! .configure() | ||
| //! .await?; | ||
| //! | ||
| //! let configured_plugin :ConfiguredPlugin<(),_,_> = match configured_plugin { | ||
| //! Some(plugin) => plugin, | ||
| //! None => return Ok(()) // Core Lightning was started with --help | ||
| //! }; | ||
| //! | ||
| //! // Note the types here. | ||
| //! // In `string_option` the developer did not specify a default and `None` | ||
| //! // will be returned if the user doesn't specify a configuration. | ||
| //! // | ||
| //! // In `default_string_option` the developer set a default-value. | ||
| //! // If the user doesn't specify a configuration the `String` `"bitcoin"` | ||
| //! // will be returned. | ||
| //! let string_option : Option<String> = configured_plugin | ||
| //! .option(&STRING_OPTION) | ||
| //! .expect("Failed to configure option"); | ||
| //! let default_string_option : String = configured_plugin | ||
| //! .option(&DEFAULT_STRING_OPTION) | ||
| //! .expect("Failed to configure option"); | ||
| //! | ||
| //! // You can start the plugin here | ||
| //! // ... | ||
| //! | ||
| //! Ok(()) | ||
| //! } |
There was a problem hiding this comment.
Very cool, this is incredibly useful, thanks ^^
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains breaking changes.
I refactored
ConfigOptionin thecln_plugincrate and added some basic documentation.It becomes easier to request the configured value of the
ConfigOption.I've marked this as draft because I still want to add some integration tests