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

Automatically split the fist argument to cmd! macro? #58

Closed
matklad opened this issue Apr 5, 2018 · 6 comments
Closed

Automatically split the fist argument to cmd! macro? #58

matklad opened this issue Apr 5, 2018 · 6 comments

Comments

@matklad
Copy link
Contributor

matklad commented Apr 5, 2018

Hi!

A common usability issue with constructing a command line is that each argument must be a separate string literal:

cmd!("git", "symbolic-ref", "--short", "HEAD")

I think a nice qol improvement would be to automatically split the first argument of cmd! on whitespace, so the same could be written more compactly as

cmd!("git symbolic-ref --short HEAD")

Note that this allows one to supply additional arguments just fine:

cmd!("git symbolic-ref --short", commit_hash)

Does this look like a useful feature to have?

@oconnor663
Copy link
Owner

oconnor663 commented Apr 6, 2018

Agreed that it's convenient in the common case, but I think the downsides are a little too big. The first one is that I'd be worried about confusing people with a command like this:

cmd!("git commit --author", my_author, "--message=foo --no-verify")

In that case, it appears that --no-verify is passed as a flag, but it's actually part of the message. The writer and the reader have to remember that whitespace in the first argument is different from whitespace in the third.

The second downside is that I'd worry about encouraging people to build commands with string concatenation. It's not as bad as passing commands to a shell, but it's still potentially problematic if you think you're letting the caller set the value of some flag, but in fact they can supply arbitrary extra flags.

That said, if you want to write shell commands without separately quoting things, there is an add-on crate that exposes a couple functions to make that easy. Is there any chance that this helps with some of your use cases? https://github.com/oconnor663/duct.rs/tree/master/duct_sh That one used to be part of duct itself, but I got too freaked out by it :)

@matklad
Copy link
Contributor Author

matklad commented Apr 6, 2018

Yeah, agree that the exact proposed API has pretty dangerous foot-guns. However, at the same time I do feel that "construct a complex command line invocation from a static string, and then append custom arguments at the end" is an overwhelmingly common case... I wonder if we can invent some safe API with a nice syntax? The syntax of duct_sh is sweet, but the semantics are outright horrifying :-)

A strawman proposal:

/// A convenience function to parse a string with command name and arguments
/// into an `Expression`
///
/// Example:
/// ```rust
/// let e = parse_cmd("git symbolic-ref --short").arg(commit_hash)
/// ```
fn parse_cmd(cmd: &'static str) -> Expression {
    let mut tokens = cmd.split_whitespace(); // TODO: error if quotes are present, or implement proper tokenization
    let prog_name = tokens.next().expect("Bad cmd: {:?}", cmd);
    cmd(prog_name, tokens)
}
  • there's only one argument, so there shouldn't be too much confusion about the difference in splitting behavior. It is pretty clear that in
    parse_cmd("git commit --author").arg(my_author).arg("--message=foo --no-verify")
    
    the last one is a single argument, and that it probably should be rewritten as
    parse_cmd("git commit --message=foo --no-verify --author").arg(my_author)
    
  • requiring &static str is a hack, which should prevent string concatenation

@oconnor663
Copy link
Owner

The "TODO: error if quotes are present" is another one of the details that makes me shy away from this. I totally agree with the TODO. It would be pretty confusing if people thought quotes were getting interpreted, but actually they were passing through as literals. Failing on that case is probably better. But that opens up a can of worms:

  • Should we also fail on backslashes? Those can be used for quoting whitespace too, and my shell tends to autogenerate them for me when I hit Tab.
  • But then again, we can't prohibit backslashes on Windows, since they're in every filepath.
  • What about other shell special characters? Should we fail if someone tries to include a > or a |? Would we want to respect platform specific differences here?

If there's a spectrum from "zero magic" to "100% magic", I kind of lean towards sitting at one of the ends. Being 80% magic tends to make things more complicated over time.

@matklad
Copy link
Contributor Author

matklad commented Apr 19, 2018

Seems reasonable! I definitely don't want to argue either way here, because I don't have strong feelings, but, if the aim is to be as easy as bash, some amount of magic wouldn't hurt. I've also seen https://github.com/ipetkov/conch-parser, which sounds like it could help with accurately dealing with parsing issues?

@oconnor663
Copy link
Owner

if the aim is to be as easy as bash

I've kind of imagined duct being as capable as bash, but with fewer footguns. I think shell programming suffers a lot from not handling corner cases correctly, and the correctness aspect matters a lot to me. That said, I'm actually thinking about removing some of the capabilities, the then operator in particular. It's nice to be able to express weirdo pipelines, but I don't think anyone actually uses it, and it significantly complicates the implementation. Anyway, there's an abstract thought for today :)

@matklad
Copy link
Contributor Author

matklad commented Apr 24, 2018

I guess it is fine to close the issue then! Thanks for an interesting discussion!

@matklad matklad closed this as completed Apr 24, 2018
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

No branches or pull requests

2 participants