enable filterwarnings=['error'] and strict-makers, strict-config#341
enable filterwarnings=['error'] and strict-makers, strict-config#341graingert wants to merge 8 commits intopsf:masterfrom
Conversation
cachecontrol/filewrapper.py
Outdated
|
|
||
| return data | ||
|
|
||
| def close(self): |
There was a problem hiding this comment.
Not sure I follow this: there's a _close method too, and it seems to be disjoint in implementation from this one. Does this have an effect on the rest of the changes in this PR?
There was a problem hiding this comment.
FileWrapper is assigned as a HTTPResponse._fp which if it has a close method gets called by HTTPResponse.close which if the request is streaming needs to be called by using the response as a context manager
There was a problem hiding this comment.
The windows issues are new and exciting, I'll have to investigate them later
There was a problem hiding this comment.
The _close is disjoint because it's called when the fp is exhausted and the buf has finished being written to, CallbackFileWrapper.close is called when the consumer of the response failed reading the body in some way, if the response was streaming
There was a problem hiding this comment.
Does this have an effect on the rest of the changes in this PR?
It's needed otherwise you get a ResourceWarning that's propagated as an unraisable exception
There was a problem hiding this comment.
I resolved the windows issues - turns out there's a bug in cpython! I think I've found a cpython bug every time I've added filterwarnings=["error"] to a project!
| vars(s).clear() # gc does this. | ||
| with pytest.raises(AttributeError): | ||
| s.x | ||
| with s._CallbackFileWrapper__buf: |
There was a problem hiding this comment.
it seems NamedTemporaryFile only issues a ResourceWarning on windows python/cpython#126639
|
@woodruffw can you take a look at this please? |
|
Yep, I'll do another review pass tonight. |
2 participants
No description provided.