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

Emit returning errors #62

Closed
whyrusleeping opened this issue Feb 9, 2018 · 3 comments
Closed

Emit returning errors #62

whyrusleeping opened this issue Feb 9, 2018 · 3 comments
Assignees

Comments

@whyrusleeping
Copy link
Member

I think it provides a pretty poor UX to have Emit return errors. It complicates the calling code a bit (what do we even do with the errors? we cant use SetError). Lets discuss how to address this.

@Stebalien
Copy link
Member

Proposal: Entirely separate errors and values.

  1. Make SetError set the final error (and close the request).
  2. Use Emit(err) to send "informational" errors to the user.

This would also solve #31 as, in the HTTP case, RCP-level errors would be sent via trailer headers only (never inline as content).

@dignifiedquire
Copy link
Member

On that note, it would be really nice if Run could return an error which gets just passed to SetError if it is set, for the simple error case, the code becomes much more aligned with the way regular error handling is written:

func(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment) {
  err := doSth()
  if err != nil {
    re.SetError(err, cmdkit.ErrNormal)
    return
  }
  re.Emit("cool")
}
// vs
func(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment) error {
  err := doSth()
  if err != nil {
    return err
  }
  re.Emit("cool")
  return nil
}

@keks
Copy link
Contributor

keks commented Apr 14, 2018

May I propose

func(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment) error {
  err := doSth()
  if err != nil {
    return err
  }
  return re.Emit("cool")
}

That way we don't lose the error and the code is still clean.

ping @whyrusleeping

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

6 participants