lightning
lightning copied to clipboard
ElementsProject
Plugin config options with no defaults
I'm making a plugin for minimint that requires a config directory path which we don't want to set a default for.
I'm currently hardcoding "default-dont-use" as default and panic later if this is still the value after initialization.
This PR makes it so I don't have to set a default.
It would be nicer if we could just make ConfigOption.default into an Option, but your serialization code for ConfigOption depends on the default field to figure out what the type of the parameter is. So it wouldn't work to just have it be None because then you wouldn't know the type.
Christian sent this message in Discord and I'm just copying it here so I don't forget about it!
We need three cases in general:
- Option with default value
- Mandatory option without which the plugin can't run
- Option without default value
I think the latter two can be combined, and have the plugin check after init if all necessary values have been set. It should definitely not panic in any case as that is not informative for the users, and will cause many pointless issues being reported. Rather provide a reason for exiting and then just exit normally.
@justinmoon we're looking to get an RC candidate cut this week; should this patch be considered as a candidate for inclusion?
@justinmoon we're looking to get an RC candidate cut this week; should this patch be considered as a candidate for inclusion?
No. It will probably take me a while to polish this off.
@justinmoon we're looking to get an RC candidate cut this week; should this patch be considered as a candidate for inclusion?
No. It will probably take me a while to polish this off.
Whoops, I missed that this is still in draft (will mark it as such) so I rebased on top of master to get it merged. Feel free to mark it as ready as soon as you feel it should be reviewed and merged.
Just out of curiosity, what's missing for it to be complete? To me it looks good to go (save name-bikeshedding ^^).
I don't remember. IIRC it works. I think you were concerned about the panic. I'm probably not going to revisit this anytime soon btw ... but would love to have it merged.
Roger that, I'll be happy to shepherd it from here ^^
Thanks @justinmoon :-)