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

Plugin config options with no defaults #5369

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Plugin config options with no defaults #5369

merged 2 commits into from
Nov 3, 2022

Conversation

justinmoon
Copy link
Contributor

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.

@justinmoon
Copy link
Contributor Author

justinmoon commented Jul 10, 2022

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.

@niftynei
Copy link
Collaborator

@justinmoon we're looking to get an RC candidate cut this week; should this patch be considered as a candidate for inclusion?

@justinmoon
Copy link
Contributor Author

@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.

@niftynei niftynei added this to the v22.10 milestone Jul 11, 2022
@cdecker cdecker self-assigned this Sep 26, 2022
@cdecker
Copy link
Member

cdecker commented Sep 26, 2022

@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 ^^).

@cdecker cdecker marked this pull request as draft September 26, 2022 13:36
@cdecker cdecker removed this from the v22.10 milestone Sep 26, 2022
@justinmoon
Copy link
Contributor Author

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.

@cdecker
Copy link
Member

cdecker commented Sep 30, 2022

Roger that, I'll be happy to shepherd it from here ^^

Thanks @justinmoon :-)

@cdecker cdecker added this to the v22.11 milestone Sep 30, 2022
@cdecker cdecker marked this pull request as ready for review October 25, 2022 12:23
justinmoon and others added 2 commits November 1, 2022 14:53
Changelog-Added: cln-plugin: Options are no longer required to have a default value
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