cli / go-gh

A Go module for interacting with gh and the GitHub API from the command line.
https://pkg.go.dev/github.com/cli/go-gh/v2
MIT License
322 stars 45 forks source link

feat: add gh.ExecInteractive() #115

Closed stemar94 closed 1 year ago

stemar94 commented 1 year ago

For #114

stemar94 commented 1 year ago

Could also be nice to not wrap the error, if it's a proper exec.ExitError.

samcoe commented 1 year ago

@stemar94 This is looking good so far. What would be the reason not to wrap an exec.ExitError? Go provides mechanisms to unwrap it if the user wants to.

stemar94 commented 1 year ago

@stemar94 This is looking good so far. What would be the reason not to wrap an exec.ExitError? Go provides mechanisms to unwrap it if the user wants to.

I like for the ExecInteractive to happen transparently, where I like to hand over responsibility to the actual gh. So it would be nice, if I could easily forward the exit code aswell.

Without unwrapping:

if execErr := gh.ExecInteractive(); execErr != nil {
  if exitErr, ok := execErr.(*exec.ExitError); ok {
    os.Exit(exitErr.ExitCode())
  }
}

With unwrapping:

if err := gh.ExecInteractive(); err != nil {
  if execErr := errors.Unwrap(err); execErr != nil {
    if exiterr, ok := execErr.(*exec.ExitError); ok {
      os.Exit(exiterr.ExitCode())
    }
  }
}

Also what does the one layer of wrapping give us in value?

mislav commented 1 year ago

With unwrapping:

if err := gh.ExecInteractive(); err != nil {
  if execErr := errors.Unwrap(err); execErr != nil {
    if exiterr, ok := execErr.(*exec.ExitError); ok {
      os.Exit(exiterr.ExitCode())
    }
  }
}

Go callers that handle specific error types should neither assume that the error value directly returned is a specific type nor manually call Unwrap to get to the desired type. Instead, they should use errors.As() to match a specific type:

if err := gh.ExecInteractive(); err != nil {
  var exitError *exec.ExitError
  if errors.As(err, &exitError) {
    os.Exit(exitError.ExitCode())
  }
}

This way, it doesn't matter whether err is literally a *exec.ExitError or a wrapped *exec.ExitError. It will just work.