numtide / treefmt

one CLI to format your repo [maintainers=@zimbatm,@brianmcgee]
https://treefmt.com
MIT License
575 stars 35 forks source link

Move post processing logic for cli into clap TypedValueParser's #195

Closed brianmcgee closed 1 year ago

brianmcgee commented 1 year ago

Currently there is a post-processing step which ensures relative paths are expanded into absolute paths and searches for the config file if required.

https://github.com/numtide/treefmt/blob/fc133187bd0de57192f089ee2bc6bb298fdd9913/src/command/mod.rs#L78-L111

A more natural fit for this functionality would be as custom value parsers https://docs.rs/clap/latest/clap/struct.Arg.html#method.value_parser, one for realising PathBuf as absolute paths and a second which optionally expands or searches for the config file.

This is predicated upon the implementation of #194

zimbatm commented 1 year ago

Sometimes I find that stuffing too much logic in the CLI parsing obscures that logic. I encountered thins in go's flag library as well. I'm not precious about it though. If it improves the code readability and usage, why not.

aldoborrero commented 1 year ago

@brianmcgee @zimbatm We can take advantage of try_map function present in TypeValueParser to modify the behaviour of an already present TypeValueParser:

/// Run as if treefmt was started in <work-dir> instead of the current working directory.
    #[arg(short, default_value = ".", value_parser = PathBufValueParser::new().try_map(ensure_abs_dir))]
    work_dir: PathBuf,

And for ensuring absolute path:

fn ensure_abs_dir(dir: PathBuf) -> Result<Option<PathBuf>, &'static str> {
    // Obtain current dir and ensure absolute
    let cwd = match env::current_dir() {
        Ok(dir) => dir,
        Err(err) => return Err("Current directory does not exist or insufficient permissions to access the current directory"),
    };
    assert!(cwd.is_absolute());

    // Make sure the work_dir is an absolute path. Don't use the stdlib canonicalize() function
    // because symlinks should not be resolved.
    Ok(Some(expand_path(&dir, &cwd)))
}

Bear in mind the signatures of the function are not finished (nor the error handling), as I haven't had time to properly compile the draft and see how it goes.

brianmcgee commented 1 year ago

Sometimes I find that stuffing too much logic in the CLI parsing obscures that logic. I encountered thins in go's flag library as well. I'm not precious about it though. If it improves the code readability and usage, why not.

I understand what you're saying, but at the same time, path expansion seems like a natural fit. Trying to find the config file, that could be argued as separate to value parsing. But at the same time, if we use the value parser it allows the Cli struct to remain immutable, rather than having this post processing mutation.

brianmcgee commented 1 year ago

Given the increasing number of parameters being passed to the format method, perhaps it's time to create an explicit Config struct which is populated with the Cli parameters or possibly just retains a reference to it, with post processing being done on construction.

aldoborrero commented 1 year ago

For now, I resorted to using TypeValueParser with a custom function called parse_path instead of resorting to a custom Config struct.

https://github.com/numtide/treefmt/blob/5deccb7897cc53defbc078d5626a52f4503acd8d/src/command/mod.rs#L79-L94

We can re-evaluate this at a later time if the complexity grows.