Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: sitescripts/extensions/utils.py

Issue 29372215: Noissue - use with and contextlib.closing for closing urlopen stream (Closed)
Patch Set: Created Jan. 17, 2017, 11:05 a.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: sitescripts/extensions/utils.py
--- a/sitescripts/extensions/utils.py
+++ b/sitescripts/extensions/utils.py
@@ -9,16 +9,17 @@
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# GNU General Public License for more details.
# You should have received a copy of the GNU General Public License
# along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
import codecs
+import contextlib
import os
import json
import re
import subprocess
import traceback
import time
import urlparse
import urllib
@@ -254,19 +255,18 @@
error = Exception('Error {0} while opening {1} url'
.format(e, url))
raise error
def _parseXMLDocument(url, attempts=2):
for i in range(attempts):
- page = _urlopen(url)
- content = page.read()
- page.close()
+ with contextlib.closing( _urlopen(url)) as page:
Sebastian Noack 2017/01/17 11:16:43 Since we already have a wrapper for urlopen(), i.e
Vasily Kuznetsov 2017/01/17 11:39:13 Yeah, that would be nice. However, the context man
Sebastian Noack 2017/01/17 11:56:43 I don't see any issues with that restriction. It s
Vasily Kuznetsov 2017/01/17 13:13:51 Fair enough, it is bad practice and in general I g
+ content = page.read()
return dom.parseString(content)
except ExpatError as err:
exception = Exception('Error {0} while parsing xml:\n{1}\nfrom {2}'
.format(err, content, url))
raise exception
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld