Toml impl #1548
Toml impl #1548
Conversation
Did you forget to commit your changes to the Rust code? I'm only seeing changes related to flake. I'm not familiar with flake at all, and only played around with Nix a little bit, but if the addition of a |
|
Yes I added flake for devShell (and .envrc for direnv so better keep it) only, I had to commit it as Nix read only committed files; this also a wip shell im gonna work with, will open separate PR. About rust code, yes it seems like I simply forgot to push it lol. I will push asap |
OK, no problem! I think it might be helpful to take a test-driven development approach for this one. Write the tests for decoding the config (and possibly combining it with the CLI options) before the actual implementation. So it would look vaguely like this: fn decode_from_toml(toml: &str) -> Config {
todo!() // This panics, but lets the code compile so tests can run
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_decode_from_toml() {
let config = decode_from_toml("types = [\"language\"]");
assert_eq!(config.types, ["language"]);
}This is really messy, but I hope this at least gives you a general idea of how you can write this PR in a test-driven way. |
|
@spenserblack I pushed changes. Code is compileable, don't look at CI complaints I forgot to remove few imports So, I took previous pull request as base, this is a really raw draft I have work a lot on. Any advice and criticism is appreciated |
Cargo.toml
Outdated
| @@ -52,6 +53,7 @@ gix-features = { version = "0.40.0", features = ["zlib-ng"] } | |||
| globset = "0.4.15" | |||
| human-panic = "2.0.2" | |||
| image.workspace = true | |||
| merge = "0.1.0" | |||
There was a problem hiding this comment.
I kind of feel that merging values can be manually implemented pretty easily, and we don't need this dep.
src/cli.rs
Outdated
| #[merge(skip)] | ||
| pub input: PathBuf, | ||
| /// Specify custom path for config file. Default is $XDG_CONFIG_HOME/onefetch/config.toml | ||
| #[arg(long, value_hint = ValueHint::FilePath)] | ||
| #[merge(skip)] | ||
| pub config_path: Option<PathBuf>, |
There was a problem hiding this comment.
input should probably be excluded from any serialization or deserialization, because it doesn't really make sense for being part of the configuration. I think that everything else is OK, though.
🤔 Not sure right now about the best way to do that, but maybe something vaguely like
#[derive(Parser)]
pub struct CliOptions {
// non-configurable option
pub input: PathBuf,
/// Options that are configurable
#[command(flatten)]
pub config: Config,
}
#[derive(Serialize, Deserialize, Parser)]
pub struct Config {
#[command(flatten)]
pub info: InfoCliOptions,
}
src/cli.rs
Outdated
| pub package_managers: bool, | ||
| } | ||
|
|
||
| impl Default for CliOptions { | ||
| fn default() -> CliOptions { | ||
| CliOptions { | ||
| input: PathBuf::from("."), | ||
| config_path: Default::default(), |
There was a problem hiding this comment.
| config_path: Default::default(), | |
| config_path: None, |
Nitpick: I think an explicit None is a bit better, here.
src/cli.rs
Outdated
| type Value = MyRegex; | ||
|
|
||
| fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| formatter.write_str("regex") |
There was a problem hiding this comment.
Nitpick: typically, with formatters, I use the write! macro (or writeln!):
write!(formatter, "A string that is a valid regex")
src/configuration.rs
Outdated
| pub fn read_cfg_file<P: AsRef<Path>>(path: &P) -> Result<CliOptions> { | ||
| let file = File::open(&path)?; | ||
| let mut buf_reader = BufReader::new(file); | ||
| let mut contents = String::new(); | ||
| buf_reader.read_to_string(&mut contents)?; | ||
| Ok(toml::from_str(contents.as_str()).unwrap()) | ||
| } | ||
|
|
||
| pub fn load_cfg<P: AsRef<Path>>(path: Option<&P>) -> Result<CliOptions> { | ||
| match path { | ||
| Some(config_path) => read_cfg_file(config_path), | ||
| None => { | ||
| let mut default_path = config_dir().unwrap(); | ||
| default_path.push("/onefetch/config.toml"); | ||
| if default_path.exists() { | ||
| read_cfg_file(&default_path) | ||
| } else { | ||
| create_dir(&default_path)?; | ||
| write_default_cfg(&default_path); | ||
| Err(anyhow!("Configuration file at {:?} does not exist!", default_path)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn write_default_cfg<P: AsRef<Path>>(default_path: &P) { | ||
| let toml = toml::to_string(&CliOptions::default()).expect("Config should be serializable"); | ||
| fs::write(default_path, toml).expect("Unable to write file"); | ||
| } |
There was a problem hiding this comment.
Looks mostly good, but I would move these to be implemented on an appropriate struct.
impl Config {
fn load_from_file<P: AsRef<Path>>(path: P) -> Result<Self> {
// ...
}
}
src/configuration.rs
Outdated
| pub fn load_cfg<P: AsRef<Path>>(path: Option<&P>) -> Result<CliOptions> { | ||
| match path { | ||
| Some(config_path) => read_cfg_file(config_path), | ||
| None => { | ||
| let mut default_path = config_dir().unwrap(); | ||
| default_path.push("/onefetch/config.toml"); | ||
| if default_path.exists() { | ||
| read_cfg_file(&default_path) | ||
| } else { | ||
| create_dir(&default_path)?; | ||
| write_default_cfg(&default_path); | ||
| Err(anyhow!("Configuration file at {:?} does not exist!", default_path)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
IMO this can just take a P instead of an Option<P>. Any unwrapping can happen before this is called, which can be simplified with:
path.unwrap_or_else(|| default_config_dir())We may also want to replace some of these unwraps and expects with non-panicking Result::Err returns, instead.
|
Why would it close bruh |
Looks like you force-pushed all the way back to b37959c. The PR auto-closed because there were no changes to merge after that force-push. I think this PR can be re-opened once new commits are pushed to it. |
|
I pushed changes but it stacked... I'll just open another one |
Very WIP around #745
@spenserblack I wish you make some review on code. I'm kinda newbie with rust, this code may seems ugly asf so please help me write it cool.
Code is uncompilable for now (as i had no time to impl some stuff yet) and all changes are under one commit(ignore flake lol) , I will refactor commits later. Waiting for your advices