DMGzipMiddleware: emit warning when middleware receives an unknown X-Compression-Safe value #538
Conversation
…Compression-Safe value this should catch programming errors where people try returning "true"/"y" etc.
| warnings.warn( | ||
| f"{self.__class__.__name__} received unknown X-Compression-Safe header value {x_compression_safe!r} - " | ||
| "falling back to default" | ||
| ) |
There was a problem hiding this comment.
Do we use the built in warnings anywhere else? How do these look when fed through an app logger?
There was a problem hiding this comment.
I don't think they come out at all in logs - I'm expecting people to notice/get annoyed by them being shown after a pytest run.
| known_values = {"0": False, "1": True} | ||
| compress = known_values.get(x_compression_safe, self.compress_by_default) | ||
|
|
||
| if x_compression_safe not in known_values and x_compression_safe != "" and x_compression_safe is not None: |
There was a problem hiding this comment.
This conditional could be simplified to
| if x_compression_safe not in known_values and x_compression_safe != "" and x_compression_safe is not None: | |
| if x_compression_safe and x_compression_safe not in known_values: |
There was a problem hiding this comment.
Oh, also, I didn't think no it does, I'm think of something elsenot in worked in Python? 🤔
There was a problem hiding this comment.
This conditional could be simplified to
Nearly. Your line would not warn on actual python False, and I didn't go and check whether flask always stringifies header values between view and middleware and don't really want to make the reader have to go & check up on that.
@lfdebrux had a point in #537 that the choice of
0,1asX-Compression-Safe's values may seem funny to some - I still think it's the best choice but in case anyone misses the plot let's at least have a warning so developers know when an attempted use isn't having the effect they expect it to.