Use with contexts for opens (#356)

Author: sveinse

canopen/objectdictionary/__init__.py

@@ -48,6 +48,10 @@ def export_od(od, dest:Union[str,TextIO,None]=None, doc_type:Optional[str]=None)
         from . import eds
         return eds.export_dcf(od, dest)
 
+    # If dest is opened in this fn, it should be closed
+    if type(dest) is str:
+        dest.close()
+
 
 def import_od(
     source: Union[str, TextIO, None],

canopen/objectdictionary/eds.py

@@ -33,7 +33,11 @@ def import_eds(source, node_id):
     except AttributeError:
         # Python 2
         eds.readfp(fp)
-    fp.close()
+    finally:
+        # Only close object if opened in this fn
+        if not hasattr(source, "read"):
+            fp.close()
+
     od = objectdictionary.ObjectDictionary()
 
     if eds.has_section("FileInfo"):
@@ -181,8 +185,8 @@ def import_from_node(node_id, network):
     network.subscribe(0x580 + node_id, sdo_client.on_response)
     # Create file like object for Store EDS variable
     try:
-        eds_fp = sdo_client.open(0x1021, 0, "rt")
-        od = import_eds(eds_fp, node_id)
+        with sdo_client.open(0x1021, 0, "rt") as eds_fp:
+            od = import_eds(eds_fp, node_id)
     except Exception as e:
         logger.error("No object dictionary could be loaded for node %d: %s",
                      node_id, e)

canopen/sdo/client.py

@@ -114,9 +114,9 @@ def upload(self, index: int, subindex: int) -> bytes:
         :raises canopen.SdoAbortedError:
             When node responds with an error.
         """
-        fp = self.open(index, subindex, buffering=0)
-        size = fp.size
-        data = fp.read()
+        with self.open(index, subindex, buffering=0) as fp:
+            size = fp.size
+            data = fp.read()
         if size is None:
             # Node did not specify how many bytes to use
             # Try to find out using Object Dictionary
@@ -155,10 +155,9 @@ def download(
         :raises canopen.SdoAbortedError:
             When node responds with an error.
         """
-        fp = self.open(index, subindex, "wb", buffering=7, size=len(data),
-                       force_segment=force_segment)
-        fp.write(data)
-        fp.close()
+        with self.open(index, subindex, "wb", buffering=7, size=len(data),
+                       force_segment=force_segment) as fp:
+            fp.write(data)
 
     def open(self, index, subindex=0, mode="rb", encoding="ascii",
              buffering=1024, size=None, block_transfer=False, force_segment=False, request_crc_support=True):

doc/sdo.rst

@@ -72,14 +72,11 @@ Variables can be opened as readable or writable file objects which can be useful
 when dealing with large amounts of data::
 
     # Open the Store EDS variable as a file like object
-    infile = node.sdo[0x1021].open('r', encoding='ascii')
-    # Open a file for writing to
-    outfile = open('out.eds', 'w', encoding='ascii')
-    # Iteratively read lines from node and write to file
-    outfile.writelines(infile)
-    # Clean-up
-    infile.close()
-    outfile.close()
+    with node.sdo[0x1021].open('r', encoding='ascii') as infile,
+            open('out.eds', 'w', encoding='ascii') as outfile:
+
+       # Iteratively read lines from node and write to file
+       outfile.writelines(infile)
 
 Most APIs accepting file objects should also be able to accept this.
 
@@ -88,17 +85,16 @@ server supports it. This is done through the file object interface::
 
     FIRMWARE_PATH = '/path/to/firmware.bin'
     FILESIZE = os.path.getsize(FIRMWARE_PATH)
-    infile = open(FIRMWARE_PATH, 'rb')
-    outfile = node.sdo['Firmware'].open('wb', size=FILESIZE, block_transfer=True)
-
-    # Iteratively transfer data without having to read all into memory
-    while True:
-        data = infile.read(1024)
-        if not data:
-            break
-        outfile.write(data)
-    infile.close()
-    outfile.close()
+
+    with open(FIRMWARE_PATH, 'rb') as infile,
+            node.sdo['Firmware'].open('wb', size=FILESIZE, block_transfer=True) as outfile:
+
+        # Iteratively transfer data without having to read all into memory
+        while True:
+            data = infile.read(1024)
+            if not data:
+                break
+            outfile.write(data)
 
 .. warning::
    Block transfer is still in experimental stage!

test/test_eds.py

@@ -15,7 +15,8 @@ def test_load_nonexisting_file(self):
             canopen.import_od('/path/to/wrong_file.eds')
 
     def test_load_file_object(self):
-        od = canopen.import_od(open(EDS_PATH))
+        with open(EDS_PATH) as fp:
+            od = canopen.import_od(fp)
         self.assertTrue(len(od) > 0)
 
     def test_variable(self):

test/test_local.py

@@ -40,17 +40,19 @@ def test_expedited_upload(self):
 
     def test_block_upload_switch_to_expedite_upload(self):
         with self.assertRaises(canopen.SdoCommunicationError) as context:
-            self.remote_node.sdo[0x1008].open('r', block_transfer=True)
+            with self.remote_node.sdo[0x1008].open('r', block_transfer=True) as fp:
+                pass
         # We get this since the sdo client don't support the switch
         # from block upload to expedite upload
         self.assertEqual("Unexpected response 0x41", str(context.exception))
 
     def test_block_download_not_supported(self):
         data = b"TEST DEVICE"
         with self.assertRaises(canopen.SdoAbortedError) as context:
-            self.remote_node.sdo[0x1008].open('wb',
-                                              size=len(data),
-                                              block_transfer=True)
+            with self.remote_node.sdo[0x1008].open('wb',
+                                                   size=len(data),
+                                                   block_transfer=True) as fp:
+                pass
         self.assertEqual(context.exception.code, 0x05040001)
 
     def test_expedited_upload_default_value_visible_string(self):

test/test_sdo.py

@@ -110,10 +110,9 @@ def test_block_download(self):
             (RX, b'\xa1\x00\x00\x00\x00\x00\x00\x00')
         ]
         data = b'A really really long string...'
-        fp = self.network[2].sdo['Writable string'].open(
-            'wb', size=len(data), block_transfer=True)
-        fp.write(data)
-        fp.close()
+        with self.network[2].sdo['Writable string'].open(
+            'wb', size=len(data), block_transfer=True) as fp:
+            fp.write(data)
 
     def test_block_upload(self):
         self.data = [
@@ -128,9 +127,8 @@ def test_block_upload(self):
             (RX, b'\xc9\x40\xe1\x00\x00\x00\x00\x00'),
             (TX, b'\xa1\x00\x00\x00\x00\x00\x00\x00')
         ]
-        fp = self.network[2].sdo[0x1008].open('r', block_transfer=True)
-        data = fp.read()
-        fp.close()
+        with self.network[2].sdo[0x1008].open('r', block_transfer=True) as fp:
+            data = fp.read()
         self.assertEqual(data, 'Tiny Node - Mega Domains !')
 
     def test_writable_file(self):
@@ -144,10 +142,9 @@ def test_writable_file(self):
             (TX, b'\x0f\x00\x00\x00\x00\x00\x00\x00'),
             (RX, b'\x20\x00\x20\x00\x00\x00\x00\x00')
         ]
-        fp = self.network[2].sdo['Writable string'].open('wb')
-        fp.write(b'1234')
-        fp.write(b'56789')
-        fp.close()
+        with self.network[2].sdo['Writable string'].open('wb') as fp:
+            fp.write(b'1234')
+            fp.write(b'56789')
         self.assertTrue(fp.closed)
         # Write on closed file
         with self.assertRaises(ValueError):