fix has_manifest for 404 responses - do not evaluate_media_type() #176
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steveej
reviewed
Sep 11, 2020
Contributor
steveej
left a comment
steveej
left a comment
There was a problem hiding this comment.
Thank you for this contribution @edwardgeorge!
I took the liberty to push some opinionated style changes directly to your fork as it seems easier to read for me that way. Please squash the commit into yours if you like it, otherwise feel free to drop it. Either way I'll merge the PR once you tell me to 😉
If getting a 404 response when calling 'has_manifest' then if the content-type returned for the json error body is 'application/json; charset=utf-8' then the call to 'evaluate_media_type' will fail because it does not expect charset=utf-8 to be valid here. see discussion in: camallo#113 This change follows the approach taken in other methods above which is to match on 'status' before continuing.
edwardgeorge
force-pushed
the
fix-has_manifest-404
branch
from
3e47393 to
cf79772
Compare
Contributor
Author
|
@steveej yep, those changes make sense. I have gone ahead and squashed them, feel free to merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If getting a 404 response when calling
has_manifestthen if the content-type returned for the json error body isapplication/json; charset=utf-8then the call toevaluate_media_typewill fail with a strum error because it does not expect thecharset=utf-8param to be valid here. see discussion in: #113This change follows the approach taken in other methods above which is to match on
statusbefore continuing to inspect the media type.