clap icon indicating copy to clipboard operation
clap copied to clipboard

Implement convenient helper methods for `PathBufValueParser` to reduce boilerplate

Open SupImDos opened this issue 3 years ago • 2 comments

Please complete the following tasks

Clap Version

3.2.17

Describe your use case

Summary

Popular command-line argument parser libraries for other languages implement validation helpers for paths for more convenient and terse CLI construction.

Example

For example, Python's click provides a click.Path type for validation, with which you can do things like:

click.Path(exists=True, file_okay=True, dir_okay=False, writable=False, readable=True)

Why?

This would allow for a reduction of boilerplate for common use-cases (i.e., existing files, existing directories, etc), and ultimately provide a more friendly and convenient "batteries-included" developer experience.

TOCTOU?

The TOCTOU issue will still be present for the validated paths, but in my opinion this enhancement would still allow for a more convenient "fail-fast" approach for CLI applications.

Describe the solution you'd like

Existing Helpers

The existing integer value parsers appear to already provide a .range(x..y) helper method to simplify parsing of narrowed integer ranges.

This allows for usage such as (as per the tutorial):

#[derive(Parser)]
#[clap(author, version, about, long_about = None)]
struct Cli {
    /// Network port to use
    #[clap(value_parser = clap::value_parser!(u16).range(1..))]
    port: u16,
}

Possible Solution

I would like similar helper methods for other types, in this case specifically the PathBufValueParser.

For example, this could allow usage such as the follows (exact form up for discussion):

#[derive(Parser)]
#[clap(author, version, about, long_about = None)]
struct Cli {
    /// Example 1: existing file only
    #[clap(value_parser = clap::value_parser!(PathBuf).exists(PathType::File)]
    file: PathBuf,

    /// Example 2: existing directory only
    #[clap(value_parser = clap::value_parser!(PathBuf).exists(PathType::Dir)]
    dir: PathBuf,

    /// Example 3: existing file or directory
    #[clap(value_parser = clap::value_parser!(PathBuf).exists(PathType::FileOrDir)]
    file_or_dir: PathBuf,
}

Future Scope

There are other less common use cases - such as checking for readability and writability, or checking that a file/directory does not currently exist - that could also be implemented (possibly with more enums, or method chaining?).

For example:

/// Example: Possible method chaining approach
clap::value_parser!(PathBuf).exists(PathType::File).permissions(Permissions::Read)

Alternatives, if applicable

No response

Additional Context

No response

SupImDos avatar Aug 13 '22 15:08 SupImDos

Huh, I don't think I ever created an issue for this but I had designed this with click's features in mind. Specifically, we have ValueParserFactory which will let us define field types that you can just put in your struct and clap will then automatically do the right thing, including

  • FileReader
    • Eagerly opens the file
    • Supports -
  • FileWriter
    • Lazily opens the file
    • Supports -
    • Option to do atomic writes (default?)
    • Eagerly error if parent directory doesn't exist (maybe an option to create it instead?)

Also supporting customization on Path would be useful.

To allow extra dependencies to be pulled in and for faster iteration, we might start this off in a clap_fs crate

epage avatar Aug 14 '22 02:08 epage

Something I saw that is important to keep in mind for - support from clig.dev

If your command is expecting to have something piped to it and stdin is an interactive terminal, display help immediately and quit. This means it doesn’t just hang, like cat. Alternatively, you could print a log message to stderr.

epage avatar Sep 09 '22 19:09 epage

IIUC, it's worth checking out https://github.com/aj-bagwell/clio, which does some of these...

max-sixty avatar Jun 09 '23 03:06 max-sixty

Thanks for letting us know of that!

I've created

  • aj-bagwell/clio#12
  • aj-bagwell/clio#11

which I think would bring it to parity with clap

epage avatar Jun 09 '23 14:06 epage

Thanks for the great ideas and opening the issues. I have implemented most of them.

parser/validator

I have added a bunch of validation options to parser/validator

There is also InputPath which require the path to exist and be a readable file, and OutputPath which requres the parent directory to exist and the file to be writeable if it exists.

#[derive(Parser)]
#[clap(author, version, about, long_about = None)]
struct Cli {
    /// Example 1: existing file or pipe only, must not be interactive tty
    #[clap(value_parser = clap::value_parser!(PathBuf).exists().is_file().not_tty())]
    file: ClioPath,

    /// Example 2: existing directory only
    #[clap(value_parser = clap::value_parser!(PathBuf).exists().is_dir()]
    dir: PathBuf,

    /// Example 3: existing file or directory
    #[clap(value_parser = clap::value_parser!(PathBuf).exists()]
    file_or_dir: PathBuf,
}

Input

  • Eagerly opens the file
  • Supports -
  • has is_tty method to check for interactive terminals
#[derive(Parser)]
struct Cli {
    /// must be existing readable file, default to stdin
    #[clap(value_parser, default = "-")]
    input: Input,
}

OutputPath

  • Defers opening/creating the file
  • Supports -
  • Option to do atomic writes (not default)
  • Eagerly error if parent directory doesn't exist
  • has is_tty method to check for interactive terminals
#[derive(Parser)]
struct Cli {
    /// If path is a directory output will be file with default name in that directory, must not be interactive tty
    /// if the file exists it must be writeable
    #[clap(value_parser = clap::value_parser!(OutputPath).not_tty().default_name("out.log"), default = "-")]
    output: OutputPath,
}

fn main() -> Result<()> {
    let mut cli = Cli::parse();
    if cli.output.is_tty() {
        // enable colors, or show an error or whatever
    }
    cli.output.create()?
}

Output

  • Eagerly opens/creats the file (but using atomic will mean it will clean up any partialy written files on exit)
  • Supports -
  • Option to do atomic writes (not default)
  • has is_tty method to check for interactive terminals
#[derive(Parser)]
struct Cli {
    /// Atomic output using tempfile and atomic rename.
    #[clap(value_parser = clap::value_parser!(Output).atomic().default_name("out.log"), default = "-")]
    output: Output,
}

Pull requests to clio are always welcome, or if this looks useful enough to get pulled into clap proper that would be excelent too.

aj-bagwell avatar Jun 21 '23 09:06 aj-bagwell

clap v4.3.6 is released which sugests using clio.

I'd love to hear people's thoughts on it over time to decide where to go from here.

Personally, I would lean towards this being kept as a separate crate so we have more flexibility regarding the implementation. If anything (and with the author's go ahead), I could see it being renamed to clap_io / clap_fs and moved into the clap repo or at least clap-rs org once we feel comfortable with it.

Personally, I'd prefer not to have HTTP support. imo arguments that accept URLs should explicitly opt-in, at minimum.

epage avatar Jun 23 '23 17:06 epage

Thank you for the official recommendation @epage!

I agree that HTTP support should be opt in which is why it is behind a feature that is off by default. For most uses piping via curl is a better choice. I added it because I needed to stream to S3 which needs to be told the content size when starting the upload, also knowing the size of the input turned out to give nicer progress bars.

If enough people show interest and want to help work on it then we can look into moving the code into the clap-rs.

aj-bagwell avatar Jun 24 '23 11:06 aj-bagwell

I agree that HTTP support should be opt in which is why it is behind a feature that is off by default. For most uses piping via curl is a better choice. I added it because I needed to stream to S3 which needs to be told the content size when starting the upload, also knowing the size of the input turned out to give nicer progress bars.

What I mean is it should be opt-in on a per-argument basis rather than on an application level basis with feature flags

epage avatar Jun 26 '23 14:06 epage