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

v2 feature: ExitCoder should implement Unwrap #1090

Closed
Nokel81 opened this issue Mar 24, 2020 · 6 comments
Closed

v2 feature: ExitCoder should implement Unwrap #1090

Nokel81 opened this issue Mar 24, 2020 · 6 comments

Comments

@Nokel81
Copy link

@Nokel81 Nokel81 commented Mar 24, 2020

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • Did you perform a search about this feature? Here's the Github guide about searching.

What problem does this solve?

In golang v1.13 the notion of error wrapping was introduced with the following optional method signature on error types: func (*T) Unwrap error. The ExitCoder type is very similar to a wrapped error with addition information (the exit code), so it seems like a good addition.

A long term consideration (for this package's v3) might be to change the implementation of ExitCoder to be:

type exitError struct {
	exitCode int
	err      error
}
@lynncyrin
Copy link
Member

@lynncyrin lynncyrin commented Mar 30, 2020

Makes sense 👍 I'm wondering if there's something we can do about this in v2? But you should definitely comment about this on the v3 thread! #833

@rliebz
Copy link
Member

@rliebz rliebz commented Mar 31, 2020

We can absolutely support this in v2 in a backward compatible way by adding a new function:

type exitError struct {
  err error
  exitCode int
}

func (e exitError) Error() string {
  return e.err.Error()
}

func (e exitError) ExitCode() int {
  return e.exitCode
}

func (e exitError) Unwrap() error {
  return err.err
}

// This could be a preferred alternative to Exit, or just an alternative.
func WrapErrorWithExitCodeButAlsoWithBetterFunctionName(err error, exitCode int) error {
  return exitError{
    err: err,
    exitCode: exitCode,
  }
}

// Exit can now return the new version of the unexported type without breaking the interface
func Exit(message interface{}, exitCode int) ExitCoder {
  // If we wanted to get fancy here, we could check if the "message"
  // was already an error, and if so, wrap it. I'm coding in GitHub so
  // I'm not going to write it out, but I recommend it

  return exitError{
    err:  fmt.Errorf("%v", message),
    exitCode: exitCode,
  }
}
@lynncyrin
Copy link
Member

@lynncyrin lynncyrin commented Mar 31, 2020

WrapErrorWithExitCodeButAlsoWithBetterFunctionName

😆

@Nokel81
Copy link
Author

@Nokel81 Nokel81 commented Mar 31, 2020

I think for v2 the simplest solution would be the following change to the Exit function.

// Exit returns an ExitCoder. If err is an error then it is directly wrapped, otherwise err is
// treated as an error message and a new error is created.
//
// This is done to be backwards comparable with previous v2 versions.
func Exit(err interface{}, exitCode int) ExitCoder {
    if e, ok := err.(error); ok {
        return exitError {
            err: e,
            exitCode: exitCode,
        }
    }

    return exitError {
        err: fmt.Errorf("%v", message),
        exitCode: exitCode,
    }
}
@stale
Copy link

@stale stale bot commented Jun 30, 2020

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

@stale stale bot added the status/stale label Jun 30, 2020
@stale
Copy link

@stale stale bot commented Jul 30, 2020

Closing this as it has become stale.

@stale stale bot closed this Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.