Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement convenient helper methods for PathBufValueParser to reduce boilerplate #4074

Open
2 tasks done
SupImDos opened this issue Aug 13, 2022 · 8 comments
Open
2 tasks done
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue. 💸 $5

Comments

@SupImDos
Copy link

SupImDos commented Aug 13, 2022

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 SupImDos added the C-enhancement Category: Raise on the bar on expectations label Aug 13, 2022
@epage epage added A-builder Area: Builder API S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Aug 13, 2022
@epage
Copy link
Member

epage commented Aug 14, 2022

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
Copy link
Member

epage commented Sep 9, 2022

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 epage added E-easy Call for participation: Experience needed to fix: Easy / not much 💸 $5 E-help-wanted Call for participation: Help is requested to fix this issue. and removed S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Sep 20, 2022
bim9262 added a commit to bim9262/clap that referenced this issue Jan 28, 2023
Resolves a few of the use cases outlined in clap-rs#4074.
bim9262 added a commit to bim9262/clap that referenced this issue Jan 28, 2023
Resolves a few of the use cases outlined in clap-rs#4074.
bim9262 added a commit to bim9262/clap that referenced this issue Jan 30, 2023
Resolves a few of the use cases outlined in clap-rs#4074.
bim9262 added a commit to bim9262/clap that referenced this issue Jan 30, 2023
Resolves a few of the use cases outlined in clap-rs#4074.
bim9262 added a commit to bim9262/clap that referenced this issue Jan 30, 2023
Resolves a few of the use cases outlined in clap-rs#4074.
@max-sixty
Copy link

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

@epage
Copy link
Member

epage commented Jun 9, 2023

Thanks for letting us know of that!

I've created

which I think would bring it to parity with clap

@aj-bagwell
Copy link

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.

epage added a commit that referenced this issue Jun 23, 2023
@epage
Copy link
Member

epage commented Jun 23, 2023

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.

@aj-bagwell
Copy link

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.

@epage
Copy link
Member

epage commented Jun 26, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue. 💸 $5
Projects
None yet
Development

No branches or pull requests

4 participants