Add default directory to payjoin-cli#874
Conversation
Pull Request Test Coverage Report for Build 16737406990Details
💛 - Coveralls |
7e74c05 to
c474b15
Compare
nothingmuch
left a comment
There was a problem hiding this comment.
I think ensure_config_dirs can be split into a separate commit that can be saved for a follow-up PR addressing #896
Apart from my suggestion to hold off creating the directory entirely for this PR, I think this is ready, please squash the later commits to the first one, and update the message so it's up to date (please also update the short message to be a bit clearer, maybe "payjoin-cli: search for config.toml in standard locations" or something like that, not sure that fits in 50 char suggested limit)
9ede585 to
06f1a27
Compare
nothingmuch
left a comment
There was a problem hiding this comment.
thanks for addressing all review comments
i missed a potential simplification that i think is worth doing, sorry for yet another round of feedback i should have thought of this earlier
| let file_source = config_path | ||
| .map(|path| File::from(path).format(FileFormat::Toml).required(true)) | ||
| .unwrap_or_else(|| File::new("config.toml", FileFormat::Toml).required(false)); | ||
| config = config.add_source(file_source); |
There was a problem hiding this comment.
i missed this before but this is a bit confusing...
if find_config_path returns Some(./config.toml), then this is redundant, and if it returns Some(~/.config/payjoin-cli/config.toml) this is ignored, so this could be simplified:
| let file_source = config_path | |
| .map(|path| File::from(path).format(FileFormat::Toml).required(true)) | |
| .unwrap_or_else(|| File::new("config.toml", FileFormat::Toml).required(false)); | |
| config = config.add_source(file_source); | |
| let Some(path) = config_path { | |
| config = config.add_source(File::from(path).format(FileFormat::Toml).required(true)); | |
| } |
but thinking about it more, since the config crate provides cascading support, maybe the right behavior would be to add both? ~/.config/payjoin-cli/config.toml can set global defaults, and ./config.toml can override. both can be .required(false)? i think that would simplify this PR even more, avoiding the need to check if files exist altogether since the config crate can handle that for us
@DanGould thoughts on using config this way? is there any good reason why if ./config.toml exists ~/.config/payjoin-cli/config.toml should be ignored entirely like i previously suggested? afaik the e2e test will be unaffected since it passes explicit args for everything, and that will take precedence, and for manual testing it seems more convenient to have global defaults and local overrides. we can also add --ignore-global-config, --ignore-local-config bools (and --ignore-config-files?) to the CLI options, that have the effect of skipping the config file addition
sorry for only realizing this now
There was a problem hiding this comment.
I like the idea of cascading defaults. and the simplicity it provides.
it seems more convenient to have global defaults and local override
I agree.
we can also add --ignore-global-config, --ignore-local-config bools (and --ignore-config-files?) to the CLI options, that have the effect of skipping the config file addition
yes we can, though I'd like to see that held off on for the future and not block this PR (just clarifying, i know you're not suggesting this)
There was a problem hiding this comment.
I like the idea of cascading defaults. and the simplicity it provides.
cool. so yeah @zealsham i think this means we can completely remove all the exists check, just unconditionally add both paths as sources and mark required(false)
yes we can, though I'd like to see that held off on for the future and not block this PR (just clarifying, i know you're not suggesting this)
yeah if we ever need that kind of thing for testing it's simple to add, but for now we have everything we need since CLI args are provided for all values anyway
There was a problem hiding this comment.
@nothingmuch , implemented , i'll take on config flag pr after this and defaul config goes in if we are moving in that direction .
a52f462 to
c37b6d1
Compare
enable payjoin-cli to search for config files in XDG-compliant directories and current working directory. Current directory take precedence over xdg directory for config values . if a required config value is missing in the current directory config the cli will fall back to using xdg directory config value .
c37b6d1 to
27a9cd9
Compare
|
thanks for sticking with this PR! |
SUMMARY
This PR enables payjoin-cli to automatically discover config files in standard locations following XDG Base Directory specification or use the current working directory . config values in the current working directory takes precedent over that of the xdg-compliant vlaue . If a required config value is missing in the current directory config , Payjoin-cli falls back to using values from the xdg-compliant directory config .
This pr closes #64
Changes
~/.config/payjoin/)