Add option to generate a default config file, fixes #870 #885
Conversation
src/bin/bat/config.rs
Outdated
| if config_file.exists() { | ||
| println!("A config file already exists at: {}", config_file.to_string_lossy()); | ||
|
|
||
| print!("Overwrite? (y/n): "); |
There was a problem hiding this comment.
Maybe make it clear that "no" is the default here:
Overwrite? (y/N):
src/bin/bat/config.rs
Outdated
| println!("A config file already exists at: {}", config_file.to_string_lossy()); | ||
|
|
||
| print!("Overwrite? (y/n): "); | ||
| let _ = io::stdout().flush(); |
There was a problem hiding this comment.
If you change this function to return Result<()>, you can use
io::stdout().flush()?;here. Another alternative would be io::stdout().flush().ok().
src/bin/bat/config.rs
Outdated
| print!("Overwrite? (y/n): "); | ||
| let _ = io::stdout().flush(); | ||
| let mut decision = String::new(); | ||
| io::stdin().read_line(&mut decision).expect("Failed to read input"); |
There was a problem hiding this comment.
If this function returns a Result<…>, you can simply use .read_line(&mut decision)? instead of .expect(…) which would kill the process in case of an error.
src/bin/bat/config.rs
Outdated
| io::stdin().read_line(&mut decision).expect("Failed to read input"); | ||
|
|
||
| if !decision.trim().eq_ignore_ascii_case("Y") { | ||
| return; |
There was a problem hiding this comment.
This would need to be a return Ok(());. Similarly, you need to use Ok(()) in the last line of this function.
src/bin/bat/config.rs
Outdated
| return; | ||
| } | ||
| } else { | ||
| let config_dir = config_file.parent().unwrap(); |
There was a problem hiding this comment.
.unwrap() also kills the process if an error occurs. It's better to use .parent()?.
src/bin/bat/config.rs
Outdated
| } | ||
| } else { | ||
| let config_dir = config_file.parent().unwrap(); | ||
| if !config_dir.exists() { |
There was a problem hiding this comment.
Doing a check like this before creating a directory or file is generally discouraged because it can lead to race conditions. Even if that is unlikely to be a problem here, the alternative is much simpler: simply create the directory and ignore the fact that it could already exist.
Now fs::create_dir would return an error in case the path already exists, but you can (and should) use fs::create_dir_all anyway. create_dir would fail if the parent directory would not exist (~/.config on Unix systems).
src/bin/bat/config.rs
Outdated
| } | ||
| } | ||
|
|
||
| let default_config = "# Specify desired theme (e.g. \"TwoDark\") |
There was a problem hiding this comment.
To prevent having to escape all quotes in this default config, you can use a raw string:
let default_config = r#"…
…"#;
src/bin/bat/config.rs
Outdated
| } | ||
|
|
||
| let default_config = "# Specify desired theme (e.g. \"TwoDark\") | ||
| #--theme=\"TwoDark\" |
There was a problem hiding this comment.
We could maybe mention bat --list-themes here in the comment.
src/bin/bat/config.rs
Outdated
| #--map-syntax \".ignore:Git Ignore\" | ||
| "; | ||
|
|
||
| fs::write(&config_file, default_config).expect("Error writing config file!"); |
There was a problem hiding this comment.
Similar here, please replace .expect(…) by a ?.
|
Thanks @sharkdp! I'll be adapting accordingly. |
I took a shot at implementing the
--generate-config-filefeature request per #870.This is my 1st foray into Rust so I'm open to feedback/suggestions for improvement. Also... thoughts on if the default config file content is sufficient or should be modified?
Thanks!