matklad / xshell

Apache License 2.0
695 stars 28 forks source link

Inconsistencies between run()/output()/read()/read_err() #57

Open rickvanprim opened 2 years ago

rickvanprim commented 2 years ago

Currently run() ignores stdin() and output()/read()/read_err() ignore quiet() (as it's always implied). It seems like either the documentation should get updated to reflect this, or the functionality made consistent.

Proposing:

run() method

    pub fn run(&self) -> Result<()> {
        if !self.data.quiet {
            eprintln!("$ {}", self);
        }
        let mut command = self.to_command();
        let status = command.status().map_err(|err| Error::new_cmd_io(self, err))?;
        self.check_status(status)?;
        Ok(())
    }

becomes

    pub fn run(&self) -> Result<()> {
        self.output_impl(false, false).map(|_| ())
    }

output_impl() method adds

        if !self.data.quiet {
            eprintln!("$ {}", self);
        }
domenicquirl commented 2 years ago

Your suggestion seems reasonable to me. I'd like to add that in some cases things that currently are the same across these methods can be surprising as well because the methods have different use cases.

As an example, I originally did not expect output() to fail (return Err) on non-zero exit codes. After all, it is the only option available if I want to look at precisely that exit code through the returned Output. However, without .ignore_status(true) it is currently only ever possible to see status code 0 in Ok results - which feels unhelpful, even if it matches the other methods on Command.

matklad commented 1 year ago

Oh, dear, the first on is the buggiest bug if I ever seen one. Terribly sorry about that, fixed in #61.

The second one is semi-intentional. The reason why .output family of functions implies .quiet is that they capture stout/stderr, so the inherited output you'll see would be confusing (you'll see the command, but not it's output, at which point its unclear whether the command failed or not).

I think to properly fix the second one we need to do the following:

matklad commented 1 year ago

See also https://github.com/matklad/xshell/issues/63#issuecomment-1630669289

blinsay commented 1 week ago

Just lost some time to read not echoing a command the way run does. I can work around it, but +1 for the option to have xshell echo the command.

(ps - this crate is fantastic, thanks for writing it!)