Create dotter.toml - #222
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces support for a new dotter.toml settings file (plus a global settings file under the user config directory) and merges these settings into CLI options with an intended precedence order.
Changes:
- Adds
DotterSettingsdeserialization and loads settings from a global config location and./dotter.toml. - Applies config values to CLI-parsed options only when the corresponding option was not provided on the command line.
- Adds
dirsas a direct dependency to locate the platform config directory.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/args.rs |
Adds settings-file loading/merging logic and new DotterSettings model for dotter.toml. |
Cargo.toml |
Adds dirs as a direct dependency for locating the global config directory. |
Cargo.lock |
Updates lockfile to include dirs as a dependency of the crate. |
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn load_settings_file(path: &std::path::Path) -> Option<DotterSettings> { | ||
| if let Ok(content) = std::fs::read_to_string(path) { | ||
| toml::from_str(&content).ok() | ||
| } else { |
| pub fn get_options() -> Options { | ||
| let mut opt = Options::parse(); | ||
| let matches = Options::command().get_matches(); | ||
| let mut opt = Options::from_arg_matches(&matches).unwrap(); |
| let repo_settings = load_settings_file(std::path::Path::new("dotter.toml")).unwrap_or_default(); | ||
|
|
||
| let from_cli = |id: &str| { | ||
| matches.value_source(id) == Some(clap::parser::ValueSource::CommandLine) | ||
| }; |
There was a problem hiding this comment.
nah, this will be tackled later when first using dotter to clone and setup your dotfiles repo
| watchexec = { version = "8", optional = true } | ||
| watchexec-events = { version = "6.0.0", optional = true } | ||
| watchexec-filterer-globset = { version = "8", optional = true } | ||
| dirs = "4.0.0" |
There was a problem hiding this comment.
uhh it wont compile if i don't declare like that. Shellscape or smth was using it before
SuperCuber
left a comment
SuperCuber
left a comment
There was a problem hiding this comment.
How are you liking the AI review? 馃槀 I hate it at work
I like this idea! But I added a couple comments
| type ForceType = bool; | ||
| type NoConfirmType = bool; | ||
| type QuietType= bool; | ||
| type DiffContextLinesType= usize; | ||
| type VerbosityType = u8; | ||
|
|
| } | ||
|
|
||
| #[derive(Debug, Clone, serde::Deserialize, Default)] | ||
| #[serde(default)] |
There was a problem hiding this comment.
Can add deny_unknown_fields to help against typos
| //TODO: do you agree, mr. maintainer SuperCuber, with the decision of having the config file | ||
| //come from whatever `dirs` considers the platform-specific dirs? namely: https://docs.rs/dirs/latest/dirs/fn.config_dir.html | ||
| //or should we make it all be just ~/.config/dotter/dotter.toml? | ||
| //first of all, i like that path for MacOS as well, and it also works for Windows, technically. `~/` resolved everywhere. |
There was a problem hiding this comment.
I think it's weird to have a global config, are you gonna store it in your dotfiles repo? Is it going to be deployed on Dotter's first run but take no effect? We're bringing global and local and every other toml with us in the repo, the most convenient is also to bring the settings in the same place...
| .unwrap_or_default(); | ||
|
|
||
| if let Some(repo) = &global_settings.repo { | ||
| if let Err(e) = std::env::set_current_dir(repo) { |
There was a problem hiding this comment.
... however I do like this idea of Dotter finding its directory from any location. I again think it's a bit of a limited use because you're probably editing your dotfiles while in the dotfiles repo and not from a random location... So maybe a global config can literally just store the last location Dotter was run from and act as a fallback if Dotter can't find its config in the current directory? I think this is a separate feature though
| //sure what the default here would be tho | ||
|
|
||
| let from_cli = |id: &str| { | ||
| matches.value_source(id) == Some(clap::parser::ValueSource::CommandLine) |
There was a problem hiding this comment.
You probably already considered this but can't we put Option around the field type inside of Options and then, if we're dropping global settings, we can do options.myfield.unwrap_or(repo_settings.myfield)
Then in DotterSettings we can have non-option fields, with our actual defaults implemented as #[default] on the field or a impl Default for DotterSettings so that serde takes them when deserializing.
But then we want a type for Options without Option so idk maybe I didn't think it through enough
I got your comment from 2021, dude
#51 (comment)
And yeah, I made it be read from a global config, and also introduced a couple of (i thought) relevant settings that take precedence in the order
compiled defaults <
~/.config/dotter/dotter.toml(for linux) <<repo>/dotter.toml< CLI flagsIf you could come up with relevant settings as well.
And if you could, also please think if it's better to have
~/.config/dotter/dotter.tomlin general, or keep them sepparete per-OS in the way thedirscrate defines them?: https://docs.rs/dirs/latest/dirs/fn.config_dir.htmlAlso, this doesn't introduce new depedencies, dirs was already there.
Alsoo, i made these ones as well as stacked PR's to deduplicate the uglyness of merging configs , pick your liking
This (which i preffer / reccomend)
#223
First time i've written a macro lol, we're all learning
Also this one if we don't like macros:
#224
Feel free to close which one you prefer
Update: uhh idk how to declare them as stacked PRs