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

kontrol: change callback signature of WatchKites() #9

Closed
humanchimp opened this issue Mar 13, 2014 · 8 comments
Closed

kontrol: change callback signature of WatchKites() #9

humanchimp opened this issue Mar 13, 2014 · 8 comments

Comments

@humanchimp
Copy link
Contributor

It should pass a single JSON object with unordered result and error properties. See #8

@cenkalti
Copy link
Contributor

Another idea: What about leaving the error field to the user? In other words, I am talking about putting the error value into the result object (the only argument sent or received [which will always be a struct/object]).

Example:

{
  "field0": "foo",
  "field1": 1,
  "error": {...}
}

I think this makes more sense because not every function has to return an error. For example simple Add(int, int) function cannot return an error.

However, we can make this a convention an check the error field at the library level automatically. Does that make sense?

@humanchimp
Copy link
Contributor Author

yes, sure

@humanchimp
Copy link
Contributor Author

actually, I'd rather nest the data one level deep:

{
  "result": {
     "field0": "foo",
     "field1": 1
  },
  // optionally
  "error": {...}
}

it gives us the possibility of adding meta-data at a later point, for example

@cenkalti
Copy link
Contributor

@fatih What's your opinion on this?

@humanchimp
Copy link
Contributor Author

another point: the application itself may provide an error property. We could ban this at the library level, but we can't really prevent it at a protocol level. I mean, somebody passes an object with error as a property, so what do we do to resolve a namespace collision like that, especially considering that "error" is a common word. So I think that it's best to nest any user-supplied error value one level deep.

{
  "result": {
    "error": "some arbitrary property called 'error'"
    "prop0": 42
  }
  // mutually exclusive, but not ambiguous
  "error": {
    "name": "ErrInvalidSession"
    ...
  }
}

@cenkalti
Copy link
Contributor

I'm convinced. I will make the change and open a pull request.

@fatih
Copy link
Contributor

fatih commented Mar 13, 2014

I'm confused with Chris's last example. How is that supposed to be simple from our current approach? There are two error fields. Is that example showing a wrong situation or current?

Can you guys post two example JSON's, one which reflects the current on and one which is supposed to be. I don't want to introduce complex changes.

@humanchimp
Copy link
Contributor Author

The point with my last example is that we should have 1 top-level namespace with the result and error, rather than mixing the result into the same namespace as the error. The latter can cause an ambiguity if the result actually has a property called "error"

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

3 participants