launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/txpkgupload:py3-context-managers into txpkgupload:master.

Commit message:
Use context managers to squash ResourceWarnings

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/txpkgupload/+git/txpkgupload/+merge/393037
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/txpkgupload:py3-context-managers into txpkgupload:master.

diff --git a/setup.py b/setup.py
index b4f4bf7..c52a978 100755
--- a/setup.py
+++ b/setup.py
@@ -24,9 +24,8 @@ def generate(*docname_or_string):
     res = []
     for value in docname_or_string:
         if value.endswith('.txt'):
-            f = open(value)
-            value = f.read()
-            f.close()
+            with open(value) as f:
+                value = f.read()
         res.append(value)
         if not value.endswith('\n'):
             res.append('')
@@ -34,7 +33,10 @@ def generate(*docname_or_string):
 # end generic helpers
 
 
-__version__ = open("src/txpkgupload/version.txt").read().strip()
+with open("src/txpkgupload/version.txt") as f:
+    __version__ = f.read().strip()
+with open("README.txt") as f:
+    description = f.readline().strip()
 
 setup(
     name='txpkgupload',
@@ -45,7 +47,7 @@ setup(
     zip_safe=False,
     maintainer='LAZR Developers',
     maintainer_email='lazr-developers@xxxxxxxxxxxxxxxxxxx',
-    description=open('README.txt').readline().strip(),
+    description=description,
     long_description=generate('src/txpkgupload/NEWS.txt'),
     license='AGPL v3',
     install_requires=[
diff --git a/src/txpkgupload/filesystem.py b/src/txpkgupload/filesystem.py
index 37c8146..495bd7b 100644
--- a/src/txpkgupload/filesystem.py
+++ b/src/txpkgupload/filesystem.py
@@ -230,21 +230,21 @@ class UploadFileSystem:
         elif start or end:
             open_flag = "r+b"
             if not os.path.exists(full_path):
-                open(full_path, 'wb')
+                with open(full_path, 'wb'):
+                    pass
 
         else:
             open_flag = 'wb'
-        outstream = open(full_path, open_flag)
-        if start:
-            outstream.seek(start)
-        chunk = instream.read()
-        while chunk:
-            outstream.write(chunk)
+        with open(full_path, open_flag) as outstream:
+            if start:
+                outstream.seek(start)
             chunk = instream.read()
-        if not end:
-            outstream.truncate()
+            while chunk:
+                outstream.write(chunk)
+                chunk = instream.read()
+            if not end:
+                outstream.truncate()
         instream.close()
-        outstream.close()
 
     def writable(self, path):
         """Return boolean indicating whether a file at path is writable."""
diff --git a/src/txpkgupload/tests/filesystem.txt b/src/txpkgupload/tests/filesystem.txt
index f64d035..97b2286 100644
--- a/src/txpkgupload/tests/filesystem.txt
+++ b/src/txpkgupload/tests/filesystem.txt
@@ -20,7 +20,8 @@ First we need to setup our test environment.
     >>> testfile = "testfile"
     >>> full_testfile = os.path.join(rootpath, testfile)
     >>> testfile_contents = b"contents of the file"
-    >>> open(full_testfile, 'wb').write(testfile_contents)
+    >>> with open(full_testfile, 'wb') as f:
+    ...     f.write(testfile_contents)
 
     >>> testdir = "testdir"
     >>> full_testdir = os.path.join(rootpath, testdir)
@@ -144,7 +145,8 @@ directory.
     >>> expected = [exp]
     >>> for i in [ "foo", "bar" ]:
     ...     filename = os.path.join(rootpath, i)
-    ...     x = open(filename, 'w')
+    ...     with open(filename, 'w'):
+    ...         pass
     ...     os.chmod(filename, stat.S_IRUSR | stat.S_IWUSR | \
     ...                         stat.S_IRGRP | stat.S_IROTH)
     ...     exp = copy.copy(def_exp)
@@ -285,7 +287,8 @@ file does not exist or is a directory.
     >>> ufs.remove(testfile)
     >>> os.path.exists(full_testfile)
     False
-    >>> open(full_testfile, 'wb').write(b"contents of the file")
+    >>> with open(full_testfile, 'wb') as f:
+    ...     f.write(b"contents of the file")
     
     >>> ufs.remove(testdir)
     Traceback (most recent call last):
@@ -314,7 +317,8 @@ directory.
     False
     >>> os.path.exists(new_full_testfile)
     True
-    >>> open(new_full_testfile, "rb").read() == testfile_contents
+    >>> with open(new_full_testfile, "rb") as f:
+    ...     f.read() == testfile_contents
     True
     >>> ufs.rename(new_testfile, testfile)
     
@@ -365,7 +369,8 @@ the given `filter` function returns True for it.
     testfile
 
     >>> for i in [ "foo", "bar", "baz", "bat" ]:
-    ...     x = open(os.path.join(rootpath, i), 'w')
+    ...     with open(os.path.join(rootpath, i), 'w'):
+    ...         pass
     >>> names = ufs.names(".", arbitrary_filter)
     >>> names.sort()
     >>> names == ['baz', 'foo']
@@ -380,7 +385,8 @@ writefile
 
     >>> import io
     >>> ufs.writefile("upload", io.BytesIO(propaganda))
-    >>> open(os.path.join(rootpath, "upload"), "rb").read() == propaganda
+    >>> with open(os.path.join(rootpath, "upload"), "rb") as f:
+    ...     f.read() == propaganda
     True
     >>> ufs.remove("upload")
 
@@ -388,7 +394,8 @@ If neither `start` nor `end` are specified, then the file contents
 are overwritten.
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"))
-    >>> open(full_testfile, "rb").read() == b"MOO"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"MOO"
     True
     >>> ufs.writefile(testfile, io.BytesIO(testfile_contents))
 
@@ -408,7 +415,8 @@ If `start` or `end` is not None, then only part of the file is
 written. The remainder of the file is unchanged.
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), 9, 12)
-    >>> open(full_testfile, "rb").read() == b"contents MOOthe file"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents MOOthe file"
     True
     >>> ufs.writefile(testfile, io.BytesIO(testfile_contents))
 
@@ -416,7 +424,8 @@ If `end` is None, then the file is truncated after the data are
 written.  
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), 9)
-    >>> open(full_testfile, "rb").read() == b"contents MOO"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents MOO"
     True
     >>> ufs.writefile(testfile, io.BytesIO(testfile_contents))
 
@@ -424,8 +433,8 @@ If `start` is specified and the file doesn't exist or is shorter
 than start, the file will contain undefined data before start.
 
     >>> ufs.writefile("didnt-exist", io.BytesIO(b"MOO"), 9)
-    >>> open(os.path.join(rootpath, "didnt-exist"), "rb").read() == (
-    ...     b"\x00\x00\x00\x00\x00\x00\x00\x00\x00MOO")
+    >>> with open(os.path.join(rootpath, "didnt-exist"), "rb") as f:
+    ...     f.read() == b"\x00\x00\x00\x00\x00\x00\x00\x00\x00MOO"
     True
     >>> ufs.remove("didnt-exist")
 
@@ -433,32 +442,37 @@ If `end` is not None and there isn't enough data in `instream` to fill
 out the file, then the missing data is undefined.
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), 9, 15)
-    >>> open(full_testfile, "rb").read() == b"contents MOOthe file"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents MOOthe file"
     True
     >>> ufs.writefile(testfile, io.BytesIO(testfile_contents))
 
 If `end` is less than or the same as `start no data is writen to the file.
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), 9, 4)
-    >>> open(full_testfile, "rb").read() == b"contents of the file"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents of the file"
     True
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), 9, 9)
-    >>> open(full_testfile, "rb").read() == b"contents of the file"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents of the file"
     True
 
 If `append` is true the file is appended to rather than being
 overwritten.
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), append=True)
-    >>> open(full_testfile, "rb").read() == b"contents of the fileMOO"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents of the fileMOO"
     True
     >>> ufs.writefile(testfile, io.BytesIO(testfile_contents))
 
 Additionally, if `append` is true, `start` and `end` are ignored.
 
     >>> ufs.writefile(testfile, io.BytesIO(b"MOO"), 10, 13, append=True)
-    >>> open(full_testfile, "rb").read() == b"contents of the fileMOO"
+    >>> with open(full_testfile, "rb") as f:
+    ...     f.read() == b"contents of the fileMOO"
     True
     >>> ufs.writefile(testfile, io.BytesIO(testfile_contents))
 
@@ -470,7 +484,8 @@ path:
     >>> ufs.writefile("foo/bar", io.BytesIO(b"fake"))
     >>> os.path.exists(os.path.join(rootpath, "foo/bar"))
     True
-    >>> open(os.path.join(rootpath, "foo/bar"), "rb").read() == b"fake"
+    >>> with open(os.path.join(rootpath, "foo/bar"), "rb") as f:
+    ...     f.read() == b"fake"
     True
 
 
diff --git a/src/txpkgupload/tests/test_plugin.py b/src/txpkgupload/tests/test_plugin.py
index c3601c8..0f6968e 100644
--- a/src/txpkgupload/tests/test_plugin.py
+++ b/src/txpkgupload/tests/test_plugin.py
@@ -572,7 +572,8 @@ class TestPkgUploadServiceMakerMixin:
         yield self.server.waitForClose()
 
         wanted_path = self._uploadPath('foo/bar/baz')
-        fs_content = open(os.path.join(wanted_path)).read()
+        with open(os.path.join(wanted_path)) as f:
+            fs_content = f.read()
         self.assertEqual(fs_content, "fake contents")
         # Expected mode is -rw-rwSr--.
         self.assertEqual(
@@ -613,7 +614,8 @@ class TestPkgUploadServiceMakerMixin:
         for upload in files:
             wanted_path = self._uploadPath(
                 "~ppa-user/ppa/ubuntu/%s" % upload)
-            fs_content = open(os.path.join(wanted_path)).read()
+            with open(os.path.join(wanted_path)) as f:
+                fs_content = f.read()
             self.assertEqual(fs_content, upload)
             # Expected mode is -rw-rwSr--.
             self.assertEqual(
@@ -661,8 +663,9 @@ class TestPkgUploadServiceMakerMixin:
         # Check the contents of files on each session.
         expected_contents = ['ONE', 'TWO', 'THREE', 'FOUR']
         for index in range(4):
-            content = open(os.path.join(
-                self.server.fsroot, upload_dirs[index], "test")).read()
+            with open(os.path.join(
+                    self.server.fsroot, upload_dirs[index], "test")) as f:
+                content = f.read()
             self.assertEqual(content, expected_contents[index])
 
 
diff --git a/src/txpkgupload/tests/test_twistedsftp.py b/src/txpkgupload/tests/test_twistedsftp.py
index 01b5e20..a1b3093 100644
--- a/src/txpkgupload/tests/test_twistedsftp.py
+++ b/src/txpkgupload/tests/test_twistedsftp.py
@@ -65,9 +65,8 @@ class TestSFTPServer(testtools.TestCase):
         upload_file = self.sftp_server.openFile('foo/bar', None, None)
         upload_file.writeChunk(0, b"This is a test")
         file_name = os.path.join(self.sftp_server._current_upload, 'foo/bar')
-        test_file = open(file_name, 'r')
-        self.assertEqual(test_file.read(), "This is a test")
-        test_file.close()
+        with open(file_name, 'r') as test_file:
+            self.assertEqual(test_file.read(), "This is a test")
         self.assertPermissions(0o100644, file_name)
         dir_name = os.path.join(self.sftp_server._current_upload, 'bar/foo')
         os.makedirs(dir_name)