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

fixed is_json_response bug (failed to account for charset in response) #23

Merged
merged 1 commit into from
Sep 10, 2015
Merged

fixed is_json_response bug (failed to account for charset in response) #23

merged 1 commit into from
Sep 10, 2015

Conversation

vassudanagunta
Copy link
Contributor

The response from the GitHub API for 'GET user' had the Content-Type of "application/json; charset=utf-8". The is_json_response method failed to account for the charset appendage, resulting in the json never being returned by the GitHub object.

@woodb
Copy link
Contributor

woodb commented Sep 9, 2015

👍 Looks good to me, we could even just do...

return response.headers.get('Content-Type', '').startswith('application/json')

@woodb
Copy link
Contributor

woodb commented Sep 9, 2015

This also brings up an interesting issue... Is there a sandbox or test environment available to run unit tests against w/ the GitHub API?

@vassudanagunta
Copy link
Contributor Author

I was being conservative, not wanting to match against some future mime type with the same prefix, e.g. 'application/jsonX'. I'm a python noob so perhaps a single regex would be better?

Apparently there is no GitHub API sandbox. The thing to do I guess is to create test accounts and test repos.

@cenkalti
Copy link
Owner

cenkalti commented Sep 9, 2015

@woodb
Copy link
Contributor

woodb commented Sep 10, 2015

That is already handled by Werkzeug

Could just do:

return 'application/json' in request.headers.get('Content-Type', '')

cenkalti added a commit that referenced this pull request Sep 10, 2015
fixed is_json_response bug (failed to account for charset in response)
@cenkalti cenkalti merged commit 378ec6a into cenkalti:master Sep 10, 2015
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