aj-bagwell / clio Goto Github PK

If a file is piped in to the program stdin will be a normal file and therefore seeking should work.

This may need to get seek implemented in core rust or some nefarious hacks to steal the file handle.

Features of this crate that are only present when a certain feature is enabled (e.g., the Http variant of Input) are currently not displayed in the crate's docs on docs.rs. This can be fixed by following these instructions.

Hi there! It's me again, from #4

We're now attempting to upgrade to clap 4 (PRQL/prql#999, though dependabot will probably open a new one for a new patch release).

I'm running cargo check --features clap/deprecated -p prql-compiler, and seeing messages like:

warning: use of deprecated function `<cli::CommandIO as clap::Args>::augment_args::parse_try_from_os_str`: Replaced with `#[clap(value_parser = ...)]` with a custom `TypedValuePa
rser`
  --> prql-compiler/src/cli.rs:42:55
   |
42 |     #[clap(default_value="-", parse(try_from_os_str = Input::try_from))]
   |                                                       ^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

Does this mean it requires a TypedValueParser? Would this be something we should write or would it come from clio?

Thank you as ever, clio has served us well (and deserves more installs!)

I can look more at this from my end, but wanted to flag this is case you recognized immediately what was happening. We get a clio error on upgrading clap=4.0.30 to clap=4.1.0.

Possibly this was a breaking change from the clap side? We can raise there if so.

ref PRQL/prql#1494

For for formats that are use seek slot e.g. zip or sqlite it would be good to be able to avoid downloading whole file by using range requests to only get bits needed.

Block devices require a special ioctl to get the length, the size returned via the metadata is zero.

Following on from #1, we're getting some warnings since upgrading to Clap 3.2 in PRQL/prql#597

warning: use of deprecated variant `clap::ArgAction::StoreValue`: Replaced with `ArgAction::Set` or `ArgAction::Append`
  --> prql-compiler/src/cli.rs:48:37
   |
48 |     #[clap(default_value="-", parse(try_from_os_str = TryFrom::try_from))]
   |                                     ^^^^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: use of deprecated variant `clap::ArgAction::StoreValue`: Replaced with `ArgAction::Set` or `ArgAction::Append`
  --> prql-compiler/src/cli.rs:51:52
   |
51 |     #[clap(short, long, default_value = "-", parse(try_from_os_str = Output::try_from))]
   |                                                    ^^^^^^^^^^^^^^^

Possibly we're doing something wrong (I actually don't know this well, the esteemed @kwigley generously implemented this).

Thank you!

I tried this our with a newer version of clap (3.1), but was unable to compile with with errors like the following

error[E0277]: `OsString` doesn't implement `std::fmt::Display`
   --> examples/cat.rs:8:36
    |
8   |     #[clap(parse(try_from_os_str = Input::try_from_os_str), default_value="-")]
    |                                    ^^^^^ `OsString` cannot be formatted with the default formatter
    |
   ::: /Users/kwigley/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/macros.rs:111:39
    |
111 |         let res = $crate::fmt::format($crate::__export::format_args!($($arg)*));
    |                                       ---------------------------------------- in this macro invocation
    |
    = help: the trait `std::fmt::Display` is not implemented for `OsString`
    = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
    = note: this error originates in the macro `$crate::__export::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)

This might be an issue with clap, sorry about that if that is the case! Here is an example where clap tests the same functionality: https://github.com/clap-rs/clap/blob/976f3d500ab890216e5d9929fcfc31bc7b4d16b4/tests/derive/custom_string_parsers.rs#L121

Just an FYI that over at PRQL we wanted our CLI to also be able take a directory, and so added some logic to clio in our crate.

We're very open to feedback on whether we're doing reasonable things!

Here's the code: https://github.com/prql/prql/blob/c1150818f4497c89164ba9ec9468459acb8a191c/prql-compiler/prqlc/src/cli.rs#L409

Feel free to close as soon as you've seen this. Thanks.

Hi, could you please create GitHub tags for each release. That makes it easier to investigate issues with each release.

The current impl will sometimes return a path of "-" for stdin/stdout which will give wrong answers for any of the methods on path that may be used.

All the interesting methods of Path need to be implemented on ClioPath with sensible values for stdin/stdout.

When using this crate with clap 4.0.18 and clap's default features, if a clio type returns an error (e.g., if a path supplied to an Input argument does not exist), then the resulting error message from clap will just be, e.g., "error: No such file or directory (os error 2)" instead of the typical "error: Invalid value 'bad-path.txt' for '--infile <INFILE>': No such file or directory (os error 2)".

The only way I am aware of to fix this would be, in your TypedValueParser implementation, instead of converting to a clap::Error using Into/?, follow the error construction steps seen here. Unfortunately, this method does not seem to let you set a custom message or source error to display in the final error message, and I'm not even sure if it would work under clap 3.

In mapping click concepts to this library,

• CachedInput, Input and Output line up with File
• InputPath and OutputPath sort of line up with File with lazy = true
• InputPath and OutputPath lines up with Path but none of Paths config options.

It seems like Path config options would belong as settings on OsStrParser so a user can pass in a custom value parser when the defaults expressed through the types don't quite work

This looks very close to what I was hoping for for clap-rs/clap#4074.

One thing I thing click.File has that this doesn't is the ability to configure writes to be atomic.

Because it relies on PathBuf::as_mut_osstring() which was stabilized in rust 1.70.0 it does not work on older version such as the one used by Debian. This makes packaging things that depend on Clio impossible.

Would it make sense to expand this crate to include directory args?

I created a simple --path arg at Electron100/butane#130 , and it was not simple to do with builder pattern, and needs to be more complicated to be correct. It should be easier, and a clio helper could provide it.

Then there are variations for whether the directory should exist already, or the user has sufficient permissions to create it.