close calibration file before deleting it, to avoid windows error (#164)

* close calibration file before deleting it, to avoid windows error

* refactors reading and validating calibration data

Author: andySigler

opentrons/instruments/instrument.py

@@ -5,9 +5,12 @@
 
 from opentrons.containers.calibrator import Calibrator
 from opentrons.util.vector import (Vector, VectorEncoder)
+from opentrons.util import environment
 from opentrons.robot.command import Command
 from opentrons import Robot
 
+from opentrons.util.log import get_logger
+
 
 JSON_ERROR = None
 if sys.version_info > (3, 4):
@@ -16,8 +19,7 @@
     JSON_ERROR = json.decoder.JSONDecodeError
 
 
-CALIBRATIONS_FOLDER = 'calibrations'
-CALIBRATIONS_FILE = 'calibrations.json'
+log = get_logger(__name__)
 
 
 class Instrument(object):
@@ -121,13 +123,8 @@ def init_calibrations(self, key, attributes=None):
             for key in attributes:
                 self.persisted_defaults[key] = copy.copy(getattr(self, key))
 
-        if not os.path.isdir(self._get_calibration_dir()):
-            os.mkdir(self._get_calibration_dir())
-
         if not os.path.isfile(self._get_calibration_file_path()):
             self._write_blank_calibrations_file()
-        else:
-            self._check_calibrations_version()
 
     def update_calibrations(self):
         """
@@ -180,18 +177,11 @@ def _write_blank_calibrations_file(self):
                 'data': {}
             }))
 
-    def _get_calibration_dir(self):
-        """
-        :return: the directory to save calibration data
-        """
-        DATA_DIR = os.environ.get('APP_DATA_DIR') or os.getcwd()
-        return os.path.join(DATA_DIR, CALIBRATIONS_FOLDER)
-
     def _get_calibration_file_path(self):
         """
         :return: the absolute file path of the calibration file
         """
-        return os.path.join(self._get_calibration_dir(), CALIBRATIONS_FILE)
+        return environment.get_path('CALIBRATIONS_FILE')
 
     def _get_calibration(self):
         """
@@ -212,30 +202,37 @@ def _build_calibration_data(self):
     def _read_calibrations(self):
         """
         Reads calibration data from file system.
+        Expects a valid valibration format
         :return: json of calibration data
         """
-        with open(self._get_calibration_file_path()) as f:
-            try:
-                loaded_json = json.load(f)
-            except json.decoder.JSONDecodeError:
-                self._write_blank_calibrations_file()
-                return self._read_calibrations()
-            return self._restore_vector(loaded_json)
+        file_path = self._get_calibration_file_path()
+        self._validate_calibration_file(file_path)
+        loaded_json = ""
+        with open(file_path) as f:
+            loaded_json = json.load(f)
 
-    def _check_calibrations_version(self):
+        return self._restore_vector(loaded_json)
+
+    def _validate_calibration_file(self, file_path):
         """
         Read calibration file, and checks for version number
         If no version number, file is replaced with version number
         """
-        with open(self._get_calibration_file_path()) as f:
+        valid = False
+        with open(file_path) as f:
             try:
                 file = json.load(f)
                 version = file.get('version')
                 data = file.get('data')
-                if not version or not data or len(file.keys()) > 2:
-                    self._write_blank_calibrations_file()
-            except json.decoder.JSONDecodeError:
-                self._write_blank_calibrations_file()
+                if version and data and len(file.keys()) == 2:
+                    valid = True
+            except json.decoder.JSONDecodeError as e:
+                log.error(
+                    'Error parsing calibration data (file: {}): {}'.format(
+                        file_path, e))
+
+        if not valid:
+            self._write_blank_calibrations_file()
 
     def _strip_vector(self, obj, root=True):
         """

opentrons/util/environment.py

@@ -16,6 +16,8 @@ def refresh():
         'LOG_FILE': os.path.join(APP_DATA_DIR, 'logs', 'api.log'),
         'CONTAINERS_DIR': os.path.join(APP_DATA_DIR, 'containers'),
         'CALIBRATIONS_DIR': os.path.join(APP_DATA_DIR, 'calibrations'),
+        'CALIBRATIONS_FILE':
+            os.path.join(APP_DATA_DIR, 'calibrations', 'calibrations.json'),
         'APP_IS_ALIVE_URL': 'http://localhost:31950',
         'APP_JUPYTER_UPLOAD_URL': 'http://localhost:31950/upload-jupyter',
     })

tests/opentrons/labware/data/calibrations.json

@@ -0,0 +1 @@
+{}

tests/opentrons/labware/data/invalid_json.json

@@ -0,0 +1 @@
+{asdfsad}

tests/opentrons/labware/test_crud_calibrations.py

@@ -1,8 +1,12 @@
+import shutil
+import os
+import json
 import unittest
 
 from opentrons import Robot
 from opentrons import containers, instruments
 from opentrons.util.vector import Vector
+from opentrons.util import environment
 
 
 class CrudCalibrationsTestCase(unittest.TestCase):
@@ -71,3 +75,22 @@ def test_delete_calibrations_data(self):
         self.assertDictEqual(self.p200.positions, {
             'top': None, 'bottom': None, 'blow_out': None, 'drop_tip': None
         })
+
+    def test_delete_old_calibration_file(self):
+
+        def test_file(file_name):
+            calib_dir = environment.get_path('CALIBRATIONS_DIR')
+            shutil.copyfile(
+                os.path.join(os.path.dirname(__file__), file_name),
+                os.path.join(calib_dir, 'calibrations.json')
+            )
+
+            instruments.instrument.Instrument()._read_calibrations()
+
+            file = os.path.join(calib_dir, 'calibrations.json')
+            with open(file) as f:
+                calib_object = json.load(f)
+                self.assertEquals(calib_object['version'], 1)
+
+        test_file('data/calibrations.json')
+        test_file('data/invalid_json.json')