Brotli import error #4697 #5037
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5037 +/- ##
==========================================
- Coverage 88.01% 86.63% -1.38%
==========================================
Files 158 158
Lines 9736 9618 -118
Branches 1435 1436 +1
==========================================
- Hits 8569 8333 -236
- Misses 911 1027 +116
- Partials 256 258 +2
|
| body = brotli.decompress(body) | ||
| try: | ||
| body = brotli.decompress(body) | ||
| except ImportError: |
There was a problem hiding this comment.
This exception cannot happen here as far as I know, can it?
There was a problem hiding this comment.
I think here I have to make changes here and leave rest as it is, and if not installed than give a warning? Will it do the work?
There was a problem hiding this comment.
Again, please read the instructions in the issue:
If the user sets br manually on the Accept-Encoding header, I think we should:
- Log a warning if brotlipy is not installed.
- Have the middleware cause an error if the server sends the response br-encoded, and brotlipy is not installed.
So you need to raise an error here, it brotli is not installed, and in addition to this you need to log a warning if brotli is not installed but ACCEPTED_ENCODINGS contains 'br' (not sure which is the best place for this).
There was a problem hiding this comment.
I think this much change can handle the issue?
There was a problem hiding this comment.
As we are already catching ImportError in this file, you can just set a flag there and check it here.
Also, I'm not sure whether logging an error and returning the compressed body or raising an exception instead is a better way to handle this.
|
I am not able to push my change further, it is showing that Edit: I had made a new pr with my master branch but this branch's problem is still there. Should I close this PR now? |
|
No, you shouldn't need to make new PRs if you just created additional commits in the same branch. |
|
Actually I tried pushing with my master and it is recognized in #4697 conversation. In this branch I got the above error and not able to push. Edit: Now my master is 1 branch ahead of scrapy/master. What should I do now? I think the best solution is to create a new pr with that and closing this one? |
Merging my master and br handling branch
|
I am really sorry, this was my first one so I got really confused. Should I close this one and create a fresh? |
I tried to solve #4697 but if there is some problem please do let me know. Also this is my first pull request so if there is any formatting error, please correct me!