Issue 29372215: Noissue - use with and contextlib.closing for closing urlopen stream (Closed)
|
Created:
Jan. 17, 2017, 11:05 a.m. by Vasily Kuznetsov Modified:
Jan. 17, 2017, 1:21 p.m. Reviewers:
Sebastian Noack CC:
paco, f.nicolaisen Visibility:
Public. |
DescriptionNoissue - use with and contextlib.closing for closing urlopen stream
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move contextlib.closing into _urlopen #MessagesTotal messages: 6
Using contextlib and with to close the stream is better indeed to let's make this right.
https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... File sitescripts/extensions/utils.py (right): https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... sitescripts/extensions/utils.py:263: with contextlib.closing( _urlopen(url)) as page: Since we already have a wrapper for urlopen(), i.e. _urlopen(), perhaps we should call contextlib.closing() there. So that if it is used more than once we don't need to duplicate that call.
https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... File sitescripts/extensions/utils.py (right): https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... sitescripts/extensions/utils.py:263: with contextlib.closing( _urlopen(url)) as page: On 2017/01/17 11:16:43, Sebastian Noack wrote: > Since we already have a wrapper for urlopen(), i.e. _urlopen(), perhaps we > should call contextlib.closing() there. So that if it is used more than once we > don't need to duplicate that call. Yeah, that would be nice. However, the context manager returned by `contextlib.closing` can't be used for reading directly, so then the only way to use the result of `_urlopen()` will be with the `with` block and something like `dom.parse(_urlopen(url))` will not be possible any more. At the moment it doesn't matter since `_urlopen` is not used anywhere else but it seems we'd make reuse more complicated. So I'd say it's more flexible the way it is and if we end up using `_urlopen()` in other places, and they turn out to also be `with` blocks, we will move `contextlib.closing` into it.
https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... File sitescripts/extensions/utils.py (right): https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... sitescripts/extensions/utils.py:263: with contextlib.closing( _urlopen(url)) as page: On 2017/01/17 11:39:13, Vasily Kuznetsov wrote: > On 2017/01/17 11:16:43, Sebastian Noack wrote: > > Since we already have a wrapper for urlopen(), i.e. _urlopen(), perhaps we > > should call contextlib.closing() there. So that if it is used more than once > we > > don't need to duplicate that call. > > Yeah, that would be nice. However, the context manager returned by > `contextlib.closing` can't be used for reading directly, so then the only way to > use the result of `_urlopen()` will be with the `with` block and something like > `dom.parse(_urlopen(url))` will not be possible any more. At the moment it > doesn't matter since `_urlopen` is not used anywhere else but it seems we'd make > reuse more complicated. So I'd say it's more flexible the way it is and if we > end up using `_urlopen()` in other places, and they turn out to also be `with` > blocks, we will move `contextlib.closing` into it. I don't see any issues with that restriction. It should always be used with a context manager anyway. Something like dom.parse(_urlopen(...)) is bad practice. Instead, you should use a with-block and pass the response from there.
https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... File sitescripts/extensions/utils.py (right): https://codereview.adblockplus.org/29372215/diff/29372216/sitescripts/extensi... sitescripts/extensions/utils.py:263: with contextlib.closing( _urlopen(url)) as page: On 2017/01/17 11:56:43, Sebastian Noack wrote: > On 2017/01/17 11:39:13, Vasily Kuznetsov wrote: > > On 2017/01/17 11:16:43, Sebastian Noack wrote: > > > Since we already have a wrapper for urlopen(), i.e. _urlopen(), perhaps we > > > should call contextlib.closing() there. So that if it is used more than once > > we > > > don't need to duplicate that call. > > > > Yeah, that would be nice. However, the context manager returned by > > `contextlib.closing` can't be used for reading directly, so then the only way > to > > use the result of `_urlopen()` will be with the `with` block and something > like > > `dom.parse(_urlopen(url))` will not be possible any more. At the moment it > > doesn't matter since `_urlopen` is not used anywhere else but it seems we'd > make > > reuse more complicated. So I'd say it's more flexible the way it is and if we > > end up using `_urlopen()` in other places, and they turn out to also be `with` > > blocks, we will move `contextlib.closing` into it. > > I don't see any issues with that restriction. It should always be used with a > context manager anyway. Something like dom.parse(_urlopen(...)) is bad practice. > Instead, you should use a with-block and pass the response from there. Fair enough, it is bad practice and in general I guess there's no harm if people will be forced to use the result of `_urlopen` in a way that makes sure it's closed at the end. Done
LGTM 
|
