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

Improve ergonomics of passing certain args conditionally #88

Open
Boscop opened this issue Oct 26, 2020 · 2 comments · May be fixed by #113
Open

Improve ergonomics of passing certain args conditionally #88

Boscop opened this issue Oct 26, 2020 · 2 comments · May be fixed by #113

Comments

@Boscop
Copy link

Boscop commented Oct 26, 2020

Currently the cmd!() macro only works for a fixed number of args.
In real-world scenarios, often some args have to be passed only conditionally, and then the cmd!() macro can't be used. Leading to hard to read code where a let mut args: Vec<String> is constructed and conditionally pushed to. And then it makes the pushing of static arg strs unnecessarily verbose: args.push("-foo".to_string());
This becomes even less ergonomic when dealing with paths such that the whole Vec has to now be OsString.

The ergonomics of passing some args conditionally to cmd!() could be greatly improved by allowing cmd!() to take Option<T> where T: Into<OsString>!
So that one can write: cmd!("prog", "-foo", cond.then_some("-bar"), "-baz")

After adding this, it also makes sense to add multiple args whose existence depends on the same condition. This requires allowing passing something like T: Iterator<Item = U> where U: Into<OsString> or simply Vec<U>/&[U].
So that one can write: cmd!("prog", "-foo", cond.then_some(&["-bar", "-baz"]))

With these small changes, the ergonomics of duct would be greatly improved :)


Notes on implementation:
The bound of the cmd function would be changed to U::Item: Into<Option<OsString>>, and in it's body, it would then be:

argv_vec.extend(args.into_iter().filter_map(|opt_arg| opt_arg).map(Into::<OsString>::into));

But: We don't want to require all unconditional args to the cmd!() to be wrapped in Some.
Although in the body of the macro, we can only do one .into() call, since we don't know whether the arg type is T: Option<OsString> or T: OsString.
So solve this problem, it could help to introduce a new trait CmdArg (which is essentially acting like Into<Option<OsString>> but allowing a conversion from T: Into<OsString> to Option<OsString> in one function call (as well as from Option<OsString>) and then changing the trait bound of the cmd function to take U::Item: CmdArg.

@oconnor663
Copy link
Owner

Interesting idea. It sounds like what we really want is one of two things:

  • either an Into<OsString>
  • or an IntoIterator<Item = impl Into<OsString>> (taking advantage of the fact that that Option is also an iterator)

Is it possible to implement something like CmdArg to abstract over this in stable Rust? Do we need to wait for specialization to land?

@Boscop
Copy link
Author

Boscop commented Oct 30, 2020

@oconnor663 It seems it's not possible to due orphan rules to impl a trait for both cases, like:

trait CmdArg {}
impl<T: Into<OsString>> CmdArg for T {}
impl<T: Into<OsString>, I: IntoIterator<Item = T>> CmdArg for I {}

error[E0119]: conflicting implementations of trait CmdArg

So I suggest requiring the user to specify for each arg which case it is, with a small syntax addition, e.g. args that are iterators are prepended with .., like:

cmd!("prog", "-foo", ..cond.then_some(&["-bar", "-baz"]))

swlynch99 added a commit to swlynch99/duct.rs that referenced this issue Sep 12, 2023
This commit allows the cmd! macro to also splice in any sequence which
implements IntoIterator<Item: Into<OsString>>. This requires a bit of
extra syntax in the cmd! macro since it would be possible for a type to
implement both Into<OsString> and IntoIterator.

So, in order to pass in a sequence to cmd! it needs to be prefixed with
... like this:

    cmd!("echo", "a", ...sequence, "b");

I have chosen ... here because ...<expr> is not a valid rust expression
and so it shouldn't conflict with anything else. Unfortunately, it's not
really possible to cleanly create a macro input which declaratively
parses either ...$arg or $arg so the cmd! macro just takes a bunch of
tokens and uses a second helper macro (cmd_expand_args!) in order to
parse that into something usable.

Since Option also implements IntoIterator this can also be rather
easily used for optional arguments since you can just do:

    cmd!("echo", "a", ...Some("b"));

I have tried to expand the docs with some examples to cover all of these
use cases which should cover for the actual macro arguments shown in
rustdoc being less readable now.

Fixes oconnor663#88
swlynch99 added a commit to swlynch99/duct.rs that referenced this issue Sep 12, 2023
This commit allows the cmd! macro to also splice in any sequence which
implements IntoIterator<Item: Into<OsString>>. This requires a bit of
extra syntax in the cmd! macro since it would be possible for a type to
implement both Into<OsString> and IntoIterator.

So, in order to pass in a sequence to cmd! it needs to be prefixed with
... like this:

    cmd!("echo", "a", ...sequence, "b");

I have chosen ... here because ...<expr> is not a valid rust expression
and so it shouldn't conflict with anything else. Unfortunately, it's not
really possible to cleanly create a macro input which declaratively
parses either ...$arg or $arg so the cmd! macro just takes a bunch of
tokens and uses a second helper macro (cmd_expand_args!) in order to
parse that into something usable.

Since Option also implements IntoIterator this can also be rather
easily used for optional arguments since you can just do:

    cmd!("echo", "a", ...Some("b"));

I have tried to expand the docs with some examples to cover all of these
use cases which should cover for the actual macro arguments shown in
rustdoc being less readable now.

Fixes oconnor663#88
@swlynch99 swlynch99 linked a pull request Sep 12, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants