-
Notifications
You must be signed in to change notification settings - Fork 166
Add some AD suggested wording to content coding decoding discussion #3367
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
Conversation
mikewest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with your text, but I took a stab at changing some of the words. Hopefully it transforms your text in a way that improves scannability "without loss of information"? :)
If you prefer your original text, land it!
| information", that doesn't necessarily mean a byte-for-byte equivalence. Many | ||
| registered content codings do provide equivalence but there is no requirement | ||
| for it; it remains a possibility that decoding could produce a different byte | ||
| sequence. In order to avoid unintended validation failures, care is advised when | ||
| selecting content coding for use with `Unencoded-Digest`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about this suggestion?
| information", that doesn't necessarily mean a byte-for-byte equivalence. Many | |
| registered content codings do provide equivalence but there is no requirement | |
| for it; it remains a possibility that decoding could produce a different byte | |
| sequence. In order to avoid unintended validation failures, care is advised when | |
| selecting content coding for use with `Unencoded-Digest`. | |
| information", that doesn't necessarily mean a byte-for-byte equivalence. It's | |
| entirely possible for content codings to perform semantically-meaningless | |
| transformations that nevertheless result in a decoded byte sequence that does | |
| not exactly match the original unencoded representation.In order to avoid | |
| unintended validation failures, care is advised when selecting content coding | |
| for use with `Unencoded-Digest`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that this maybe makes the risk sound too large. I think Julian identified one content coding that behaves that way, and I don't think its widely used that. I hope that implementers of the spec can just assume things like gzip, brotli etc can be used without issues. I'll make a hybrid change that incorporates some of your proposed text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL a look at the latest version after I pushed a commit.
MikeBishop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either version works for me.
d0d1e99 to
3a839d4
Compare
MikeBishop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this help to further scope the size of the risk? It's worth calling out for completeness, but in practice most will be byte-for-byte unless they're media-type-specific.
Co-authored-by: Mike Bishop <[email protected]>
Co-authored-by: Mike Bishop <[email protected]>
|
With the two applied suggestions, I think that it strikes a decent balance on identifying the scope of risk, without being overlying rabbitholing |
|
In the interest of moving forward, I'll merge this and cut a new release. We can continue to wordsmith up until publication if its really necessary. Thanks for the input! |
I found this a little difficult to integrate in and hence its quite a mouthful of some sentences. However, I think it captures what was described in the issue.
Fixes #3356