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

JSON Requester #24

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

JSON Requester #24

wants to merge 12 commits into from

Conversation

DarrenN
Copy link
Contributor

@DarrenN DarrenN commented Aug 28, 2016

Fixes #12

  • Provides json-requester
  • json-requester returns json-response struct
  • When making requests with a json-requester the following headers are automatically injected: Accept: application/json and Content-Type: application/json. These can be overridden as normal.
  • When making requests with a json-requester the body needs to be a valid jsexpr? and will be automatically converted to a bytes representation of a JSON string (ex: (hasheq 'grand "larceny") -> "{\"grand\": \"larceny\"}")
  • json-response expects the response body to be a valid jsexpr?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 83.222% when pulling ea65022 on DarrenN:json-requester into 49936b2 on jackfirth:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 83.763% when pulling fd4b7ef on DarrenN:json-requester into 49936b2 on jackfirth:master.

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 28, 2016

Looks like some deps didn't install correctly on older versions of Racket. Usually I would restart the travis build but that doesn't seem to be an option on the free version. Hoping it will shake out on future commits pushed up to this PR.

@jackfirth
Copy link
Owner

jackfirth commented Aug 28, 2016

I have some general comments before addressing specifics:

  • I don't think this belongs in base.rkt. That module is for only the minimal API of requesters, as little as possible should go in there. I recommend a private/json.rkt module that exclusively contains the json stuff.
  • Rather than changing any of the call/input-url functions or the http-requester, it would be cleaner to make a json requester a simple wrapper over any requester that works with http-response structs and byte requests.
  • When sending a request with Accept: application/json, there is no guarantee that the response body will actually be json. The server could send back a 406 Not Acceptable response, or it could ignore the request completely. If the response of a json-requester is going to contain the headers and code, even in the case of an error code like 406, then it can't contain the body parsed ahead of time because the body might not be valid json. Not sure how to resolve this, but it might require that json-requester be built on top of http-requester/exn rather than http-requester.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 85.392% when pulling 6114fbc on DarrenN:json-requester into 49936b2 on jackfirth:master.

@DarrenN
Copy link
Contributor Author

DarrenN commented Sep 4, 2016

I think I addressed most of your comments in the recent push:

  • Move code into json.rkt
  • json-requesternow does all its work with simple wrappers around http-requester
  • I just bit the bullet and dispensed with json-response as a concept. json-requester now uses http-requester/exn as its base and throws exceptions on error codes. This means that json-requester only returns the parsed body (jsexpr?) or an exn.

Ideally I would have liked to keep the json-response struct with headers and codes, but as you mentioned it proved difficult when dealing with non-JSON responses. I wonder if there is a middle ground, perhaps keeping json-response with a non-parsed body and creating json-requester/exn that does attempt to parse the body into a jsexpr?.

@DarrenN
Copy link
Contributor Author

DarrenN commented Sep 20, 2016

Went ahead an rolled a new exception (exn:fail:json) that is thrown when json-requester receives invalid JSON as a response. Added an integration test for it.

@coveralls
Copy link

Coverage Status

Coverage increased (+5.9%) to 90.596% when pulling 84f1237 on DarrenN:json-requester into 49936b2 on jackfirth:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 84.636% when pulling 84f1237 on DarrenN:json-requester into 49936b2 on jackfirth:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 84.615% when pulling 5572e69 on DarrenN:json-requester into 49936b2 on jackfirth:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 84.595% when pulling 189e962 on DarrenN:json-requester into 49936b2 on jackfirth:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 84.636% when pulling a351c07 on DarrenN:json-requester into 49936b2 on jackfirth:master.

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.

None yet

3 participants