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

New exceptions #108

Merged
merged 3 commits into from
Aug 22, 2016
Merged

New exceptions #108

merged 3 commits into from
Aug 22, 2016

Conversation

organisciak
Copy link
Member

@organisciak organisciak commented Aug 22, 2016

There is now a BookwormException which you can explicitly raise with a dict in this style:

raise BookwormException({"message": "I'm a teapot", code: 418})

This is sent up the stack and caught when executing a query, which will return a JSEND-style response:

    {
    " status " : "error",
    "message" : "I'm a teapot",
    "code": 418 
    }

The codes, as mentioned in #95, are HTTP status codes. This makes it easy for the API to format its HTTP response to match what the backend provides. This is currently in dbbinding-flask.py , since that's what I test with. The reason you want both the JSend response and an HTTP code is because clients parse the
HTTP code to see if the response should be handled as an error. Eg. request.on("error", function(error) { callback(error); }) in D3.

@bmschmidt
Copy link
Member

Super. I've almost got travis working, so may wait a minute to merge.

Couple questions about syntax:

  1. The syntax on this example looks weird--two closing parentheses. Is it correct?
raise BookwormException("I'm a teapot" code: 418)) 

Is there/should there be spaces around the word "status", and not the other dict keys? If so, why?

    {
    " status " : "error",
    "message" : "I'm a teapot",
    "code": 418 
    }

@bmschmidt
Copy link
Member

Also, what do we need to do to merge dbbinding-flask.py and dbbindings.py back together? I don't want to start maintaining two codebases in parallel, and I can't remember what the flask one is doing now.

@organisciak
Copy link
Member Author

There's an error above (my tab froze before posting, so I copied the text from a screenshot rather than rewriting). I'll edit it to avoid future confusion, but should be:

raise BookwormException({"message": "I'm a teapot", code: 418})

or tidier:

err = dict(message="....", code=418)
raise BookwormException(err)

I'm using dbbindings-flask because it can be run as a self-standing app rather than existing in cgi-bin, is written at a higher level (e.g. a vs b), and has a great debugging mode.

Noting that you have bookworm serve, I don't really need it. The other two benefits are just gravy, though they suggest that maybe it's worth evaluating whether the flask-based version can supercede the main one, rather than being deprecated.

By the way, I misremembered in the original message, it looks like I did write the dbbindings.py support for the new responses back in April.

@bmschmidt bmschmidt merged commit b2dd695 into master Aug 22, 2016
@bmschmidt bmschmidt deleted the new-exceptions branch August 22, 2016 19:33
@bmschmidt
Copy link
Member

OK, a succesful Travis merge.

Which doesn't mean too much because I don't think we're testing the new bookworm error formats.

Let me know if you want some guidelines on unit tests. Here's one just for reference. https://github.com/Bookworm-project/BookwormDB/blob/master/tests/test_API.py#L126-L145

@organisciak
Copy link
Member Author

👍

On Mon, Aug 22, 2016, 1:34 PM Benjamin Schmidt [email protected]
wrote:

OK, a succesful Travis merge.

Which doesn't mean too much because I don't think we're testing the new
bookworm error formats.

Let me know if you want some guidelines on unit tests. Here's one just for
reference.
https://github.com/Bookworm-project/BookwormDB/blob/master/tests/test_API.py#L126-L145


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#108 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADj76tk2KE_AAwz81pQieBWeBvjAz3Tks5qifmugaJpZM4JqIJS
.

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

Successfully merging this pull request may close these issues.

2 participants