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

Update README.md #159

Closed
wants to merge 1 commit into from
Closed

Update README.md #159

wants to merge 1 commit into from

Conversation

kehao-chen
Copy link

@kehao-chen kehao-chen commented Oct 18, 2016

FIXME: The content type string could contain additional values like the charset.

// old
if (response.headers['content-type'] === 'application/json')
// new
if (response.headers['content-type'].includes('application/json')) 

For "FIXME: The content type string could contain additional values like the charset."
@coveralls
Copy link

coveralls commented Oct 18, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 2d0cb8a on HappyHackingNinja:master into a804c7f on request:master.

@analog-nico
Copy link
Member

Thank you very much. However, I put the FIXME there intentionally to make the reader aware that the code is not safe. The way you fixed it is probably not safe either.

In actual production code a more robust solution like using the content-type library would be necessary. Should we put that into the README? What do you think?

@kehao-chen
Copy link
Author

kehao-chen commented Oct 18, 2016

You right. I think it should not put unnecessary dependencies into the example code snippet. It's fine to keep what it is.

analog-nico added a commit that referenced this pull request Oct 18, 2016
discussed in pull request #159
@analog-nico
Copy link
Member

Anyways, you are pointing out something important. It is not helpful to mention a shortcoming and then not offering a solution. I agree, the example should remain simple. So I added a little note as you see in the commit linked above.

Thanks for bringing this up!

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.

3 participants