Why shouldn't one have `impl Into<Result> for CommandWrapper`?

help
1

Uh, we made a wrapper around Command so we could get better errors, and... we thought it would be neat to have an

        impl trait Into<Result<Output, GitError>> {
            fn into(self) -> Result<Output, GitError> {
                let Args { inner: mut cmd, args } = self;
                let mut args = Some(args);
                cmd.output().map_err(|e| {
                    GitError::new(e, args.take().unwrap())
                }).and_then(|output| {
                    if output.status.success() {
                        Ok(output)
                    } else {
                        Err(GitError::new(output, args.take().unwrap()))
                    }
                })
            }
        }

but some ppl tried to discourage us from doing so? What are some reasons against doing this?

2

I think the main reason is just that it's unusual. I would define a normal method instead.

3

I think the reason this feels weird is because Into/From are meant to convert from one type to another without any side-effects, yet your Into implementation calls cmd.output() which has quite a noticeable side-effect.

If I were doing this quite often and it got annoying I'd probably define an extension trait who's method doesn't have such connotations.

trait CommandExt {
    fn execute(&mut self) -> Result<Output, CommandError>;
}

impl CommandExt for Command {
    fn execute(&mut self) -> Result<Output, CommandError> {
        let output = self.output().map_err(|e| CommandError::DidntRun { command: format!("{:?}", self) })?;

        if output.status.success() {
            Ok(output)
        } else {
            Err(CommandError::CommandFailed { command: format!("{:?}", self), output })
        }
    }
}

#[derive(Debug)]
enum CommandError {
    DidntRun { command: String, error: std::io::Error },
    CommandFailed { command: String, output: Output },
}

(playground)

2 Likes
4

We guess having a method also works better with ?. Fair enough.

5

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.