fastai / ghapi

A delightful and complete interface to GitHub's amazing API
https://ghapi.fast.ai/
Apache License 2.0
527 stars 57 forks source link

Add support for media types #102

Closed lfdebrux closed 2 years ago

lfdebrux commented 2 years ago

For some endpoints GitHub lets you request different response format using the Accept header [1]. However, by default if the response is not in JSON format GhApi.__call__ will raise an error.

This commit makes it possible by adding a bit of logic to look at the Accept header in the request and tell fastcore.core.urlsend not to return JSON if it doesn't look like the user is requesting a JSON media type.

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

rshk commented 2 years ago

I was looking into mimetype support as well.

I wonder if adding an extra argument to __call__() to control this behaviour would be better, as it makes more explicit what's the desired return type?

(Eg. there is a slight chance that a non-json mimetype contains the "json" string -- or that a json-based format doesn't).

So eg. change the signature to:

def __call__(
    self,
    path:str,
    verb:str=None,
    headers:dict=None,
    route:dict=None,
    query:dict=None,
    data=None,
    return_json:bool=True,
):
    # ...
lfdebrux commented 2 years ago

@rshk I considered this initially, but it seemed a bit unnecessary because a) I think the developer's intent is already made clear by changing the accept header b) with a return_json param, if you changed the accept header but forgot to change the param, you'd get an exception from the JSON decoder, so there's no value to having the two settings separated and c) the GitHub media types seem to be well thought out and logical, a scenario where the media type contains json but is not JSON (or vice versa) seems unlikely.

jph00 commented 2 years ago

Many thanks! FYI you forgot to run nbdev_build_lib - I'll do that and push.