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

feat(config): toml deserialization errors #41

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

snormore
Copy link
Member

@snormore snormore commented Oct 4, 2024

This PR updates the TOML config provider to error when deserialization fails, where previously it would just return the default of the config.

@snormore snormore force-pushed the s/config-deser-errors branch 2 times, most recently from eee8ad2 to f32056f Compare October 4, 2024 21:31
@ozwaldorf
Copy link
Member

ozwaldorf commented Oct 4, 2024

Does struct level #[serde(default)] as implemented on most config types (to reuse old fields while still defaulting new ones) silently defeat this?

@ozwaldorf
Copy link
Member

ozwaldorf commented Oct 4, 2024

We also somewhat rely on the defaulting behavior in lightning-node print-config (ie, to update a config, just lightning-node print-config > ~/.lightning/config.toml)

But maybe that command is not relevant anymore if we do want to require config to always be correct (ie, print config = cat)

@snormore
Copy link
Member Author

snormore commented Oct 4, 2024

It returns the default config if the key isn't found, so print-config still works, and there are a bunch of tests that depend on it too (I tried erroring to see what would break). This only changes the behavior when deserialization of something existing fails.

@ozwaldorf
Copy link
Member

Sounds good 👍

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.

2 participants