WIP: Config file for theming/customization #790
Conversation
There was a problem hiding this comment.
I want to ensure that if the user-supplied config is malformed,
onefetchprints a warning about that and uses the default config values.
Just an FYI I did exactly that in another project 🙂
https://github.com/spenserblack/repofetch/blob/d87c577bc1987f0a7bb7434dfeb5a5ac444d9584/src/main.rs#L97-L108
https://github.com/spenserblack/repofetch/blob/d87c577bc1987f0a7bb7434dfeb5a5ac444d9584/src/configuration/mod.rs#L85-L119
Though, in hindsight, the way I did it can hide the reason why the config is malformed, confusing users. I think it's important to show the user exactly why the config couldn't be read. IMO either a warning or an error would both be OK as long as the necessary information for the user to fix the malformed config is accessible.
I'm thinking of moving config-specific code to a separate file, but that's most of
cli.rs. What would be a good way to separate them?
I think the config logic can be moved to a configuration.rs file, with cli.rs focused on both creating the CLI arguments, and using the CLI arguments to build that config, if that makes sense. I.e. configuration.rs returns the config struct, cli.rs mutates that struct.
Which of the config value sources has more priority, the config file or the command line arguments?
I think CLI arguments should always override config file values. Yes, it can be annoying if options like exclude aren't merged, but I think it can be confusing if CLI arguments don't all have the same behavior. IMO using a CLI arg has always meant "use this instead". There can be a new CLI arg, extra-exclude, to "merge" this value if it's needed. As an example, pip has both --index-url and --extra-index-url args -- one overwrites, one appends.
| pub r#type: Vec<LanguageType>, | ||
| /// Specify a custom path to a config file. | ||
| /// Default config is located at ${HOME}/.config/onefetch/config.conf. |
There was a problem hiding this comment.
Just an FYI that this is only the default path on Linux if you're using dirs, AFAIK.
| fn get_default_config() -> Config { | ||
| Config { | ||
| input: PathBuf::from("."), | ||
| ascii_input: Default::default(), | ||
| ascii_language: Default::default(), | ||
| ascii_colors: Default::default(), | ||
| disabled_fields: Default::default(), | ||
| image: Default::default(), | ||
| image_protocol: Default::default(), | ||
| color_resolution: 16, | ||
| no_bold: Default::default(), | ||
| no_merges: Default::default(), | ||
| no_color_palette: Default::default(), | ||
| number_of_authors: 3, | ||
| exclude: Default::default(), | ||
| no_bots: Default::default(), | ||
| languages: Default::default(), | ||
| package_managers: Default::default(), | ||
| output: Default::default(), | ||
| true_color: When::Auto, | ||
| show_logo: When::Always, | ||
| text_colors: Default::default(), | ||
| iso_time: Default::default(), | ||
| email: Default::default(), | ||
| include_hidden: Default::default(), | ||
| r#type: vec![LanguageType::Programming, LanguageType::Markup], | ||
| completion: Default::default(), | ||
| } | ||
| } |
There was a problem hiding this comment.
A wise choice to move this into a Default implementation 👍
| let toml = toml::to_string(&Config::default()).expect("Config should be serializable"); | ||
| fs::write(default_path, toml).expect("Should be able to write to config dir"); |
There was a problem hiding this comment.
Honestly, maybe these should just be unwraps to show the underlying error. For example, fs::write might fail if there's no more space on the disk. If that happens, the user should probably see that, and only that.
Resolves #745.
General
Bugs / Improvements
onefetchsubdirectory. I'm still figuring out how to best ensure that a parent dictionary is created if necessary.onefetchprints a warning about that and uses the default config values. Or would it be better to fail completely instead of showing the info with default options?Other questions
cli.rsgot a lot longer. I'm thinking of moving config-specific code to a separate file, but that's most ofcli.rs. What would be a good way to separate them?excludepaths from both sources counted. Otherwise, the user would need to write out all the excluded paths in the config if they want to include an extra path.boolvalues are merged basically as an "or" (if it'struein the config file, then the command-line argument doesn't matter), and all other values are completely overwritten. This is obviously not ideal, and I plan to change it, but I'd like to hear your perspective on what I should change it to.MyRegexinstead of the trait implementation I wrote, but I couldn't think of one.I'm very new to Rust, so if something I wrote is not idiomatic, I'll be happy to change that if you point it out.