Apply suggestions from code review of branch split-scan-dir-operations

Co-authored-by: Beth Probert <beth_probert@outlook.com>

src/openflexure_microscope_server/scan_directories.py

@@ -35,7 +35,7 @@ class ScanInfo(BaseModel):
 
 class ScanDirectoryManager:
     """
-    A class for managinging interactions with scan directories
+    A class for managing interactions with scan directories
     """
 
     _base_scan_dir: str
@@ -57,7 +57,7 @@ class ScanDirectoryManager:
     def path_for(self, scan_name: str) -> str:
         """Return the path for a given scan name
 
-        Returns the path even if it doesn't exist)
+        Returns the path even if it doesn't exist
         """
         return os.path.join(self._base_scan_dir, scan_name)
 
@@ -97,7 +97,7 @@ class ScanDirectoryManager:
         return file_path
 
     @property
-    def all_scans(self):
+    def all_scans(self) -> list[str]:
         """Return a list of the scan names in the base directory"""
         return [f.name for f in os.scandir(self._base_scan_dir) if f.is_dir()]
 
@@ -138,11 +138,11 @@ class ScanDirectoryManager:
 
         return f"{scan_name}_{scan_num:0{SCAN_ZERO_PAD_DIGITS}d}"
 
-    def new_scan_dir(self, scan_name: str) -> str:
+    def new_scan_dir(self, scan_name: str) -> "ScanDirectory":
         """Get a unique name for this scan and create a directory for it
 
         The scan will be named `{scan_name}_0001` where the number is
-        zero-padded to be 4 digits long (to allow correct sorting if the
+        zero-padded to be SCAN_ZERO_PAD_DIGITS digits long (to allow correct sorting if the
         scans are ordered alphanumerically).
 
         Creates a new empty folder, into which scans are saved
@@ -161,11 +161,11 @@ class ScanDirectoryManager:
         os.makedirs(self.img_dir_for(full_scan_name))
         return ScanDirectory(full_scan_name, self.base_dir)
 
-    def delete_scan(self, scan_name: str):
+    def delete_scan(self, scan_name: str) -> None:
         """Delete a scan"""
         shutil.rmtree(self.path_for(scan_name))
 
-    def zip_scan(self, scan_name: str, final_version: bool = False) -> str:
+    def zip_scan(self, scan_name: str, final_version: bool = False) -> "ScanDirectory":
         """Zips any images from the scan not yet zipped, return full path to zip
 
         `final_version` Set true to stitch all files not just the scan images
@@ -213,7 +213,7 @@ class ScanDirectory:
             )
 
     @property
-    def name(self):
+    def name(self) -> str:
         """The name of the scan"""
         return self._name
 
@@ -249,7 +249,7 @@ class ScanDirectory:
         return max(os.stat(root).st_mtime for root, _, _ in os.walk(self.dir_path))
 
     def scan_info(self):
-        """Return the inforomation for the scan directory as a ScanInfo object"""
+        """Return the information for the scan directory as a ScanInfo object"""
         folder_contents = self.get_scan_files()
         if folder_contents:
             scan_images = [i for i in folder_contents if IMAGE_REGEX.search(i)]

src/openflexure_microscope_server/things/smart_scan.py

@@ -155,7 +155,7 @@ class SmartScanThing(Thing):
             if self._scan_data is not None:
                 self._return_to_starting_position()
                 if not isinstance(e, scan_directories.NotEnoughFreeSpaceError):
-                    # Don't stich if drive is full (already logged)
+                    # Don't stitch if drive is full (already logged)
                     self._perform_final_stitch()
             # Error must be raised so UI gives correct output
             raise e
@@ -671,7 +671,7 @@ class SmartScanThing(Thing):
         "scans/{scan_name}",
         responses={
             200: {"description": "Successfully deleted scan"},
-            400: {"description": "Scan not deleted does"},
+            400: {"description": "An error occurred while trying to delete scan"},
         },
     )
     def delete_scan(self, scan_name: str, logger: InvocationLogger) -> None:
@@ -744,7 +744,7 @@ class SmartScanThing(Thing):
 
         This will return None (`null` to JS) if there is no preview image to return.
 
-        This is used for two things reasons:
+        This is used for two reasons:
         1. If all caching was turned off this stitch would be sent over the network
            repeatedly
         2. If caching was is on, then the stitch will not update when needed.

tests/mock_things/mock_autofocus.py

@@ -12,6 +12,6 @@ class MockAutoFocusThing:
     # Counter for checking functions were called
     mock_call_count = {"looping_autofocus": 0}
 
-    def looping_autofocus(self, dz=2000, start="centre"):  # noqa: ARG002
+    def looping_autofocus(self, dz: int = 2000, start: str = "centre") -> None:  # noqa: ARG002
         """This function mocks autofocus with no return"""
         self.mock_call_count["looping_autofocus"] += 1

tests/mock_things/mock_background_detect.py

@@ -1,4 +1,4 @@
-"""This testing submodule contains mock autofocus things.
+"""This testing submodule contains mock background detect things.
 
 These mocks are designed to be inserted as dependencies to give specific
 functionality and returns.

tests/test_scan_directories.py

@@ -24,13 +24,13 @@ LOGGER = logging.getLogger("mock-invocation_logger")
 BASE_SCAN_DIR = os.path.join(tempfile.gettempdir(), "scans")
 
 
-def _clear_scan_dir():
+def _clear_scan_dir() -> None:
     """Delete the scan dir"""
     if os.path.exists(BASE_SCAN_DIR):
         shutil.rmtree(BASE_SCAN_DIR)
 
 
-def _add_fake_image(scan_dir: ScanDirectory):
+def _add_fake_image(scan_dir: ScanDirectory) -> None:
     """Make a fake image on disk in the scan directory"""
     x_pos = random.randint(-100, 100)
     y_pos = random.randint(-100, 100)
@@ -40,8 +40,14 @@ def _add_fake_image(scan_dir: ScanDirectory):
         f_obj.write("fake")
 
 
-def _add_fake_file(scan_dir: ScanDirectory, filename: str, in_im_dir=False):
+def _add_fake_file(
-    """Make a fake file  on disk in the scan directory"""
+    scan_dir: ScanDirectory, filename: str, in_im_dir: bool = False
+) -> None:
+    """Make a fake file on disk in the scan directory.
+
+    :param in_im_dir: Boolean, if set True the fake file is created in the images
+    directory of the scan not the root directory.
+    """
     if in_im_dir:
         filepath = os.path.join(scan_dir.images_dir, filename)
     else:
@@ -53,7 +59,12 @@ def _add_fake_file(scan_dir: ScanDirectory, filename: str, in_im_dir=False):
 def test_basic_directory_operations():
     """Test some basic operations
 
-    Rather a long test but systematically iterates through some basics.
+    Test some basic operations, including:
+    - ScanDirectoryManager creates a scan directory
+    - For a fake (dummy) scan, a path can be created and retrieved
+    - For a fake (dummy) file, a path can be created and retrieved (for both a .zip and .img file)
+    - When a scan directory is created, the directory exists as does the image directory
+    - Scan directories can be deleted and scans can be cleared
     """
     _clear_scan_dir()
     assert not os.path.isdir(BASE_SCAN_DIR)
@@ -66,20 +77,20 @@ def test_basic_directory_operations():
     scan_path = os.path.join(BASE_SCAN_DIR, scan_name)
     scan_im_dir = os.path.join(BASE_SCAN_DIR, scan_name, "images")
 
-    # It doesn't exitist but we can check its paths
+    # It doesn't exist but we can check its paths
     assert not scan_dir_manager.exists(scan_name)
     assert not os.path.isdir(scan_path)
     assert scan_dir_manager.path_for(scan_name) == scan_path
     assert scan_dir_manager.img_dir_for(scan_name) == scan_im_dir
 
-    # or a get the path of a fake file
+    # Get the path of a fake file
     fake_file = scan_dir_manager.get_file_from(scan_name, "foo.zip")
     assert fake_file == os.path.join(scan_path, "foo.zip")
     # But this is none if we check it exists
     fake_file = scan_dir_manager.get_file_from(scan_name, "foo.zip", check_exists=True)
     assert fake_file is None
 
-    # or a get the path of a fake file
+    # Get the path of another fake file
     fake_file = scan_dir_manager.get_file_from_img_dir(scan_name, "bar.img")
     assert fake_file == os.path.join(scan_im_dir, "bar.img")
     # But this is none if we check it exists
@@ -97,6 +108,7 @@ def test_basic_directory_operations():
     assert os.path.isdir(scan_path)
     assert os.path.isdir(scan_im_dir)
 
+    # Test cleanup. Delete the created scan and scan directory
     scan_dir_manager.delete_scan(scan_name)
     assert not scan_dir_manager.exists(scan_name)
     assert not os.path.isdir(scan_path)
@@ -116,7 +128,7 @@ def test_scan_sequence_and_listing():
 
     # Check they exist and are numbered sequentially
     all_scans = scan_dir_manager.all_scans
-    assert len(all_scans) == 4
+    assert len(set(all_scans)) == 4
     assert "fake_scan_0001" in all_scans
     assert "fake_scan_0002" in all_scans
     assert "fake_scan_0003" in all_scans
@@ -149,7 +161,7 @@ def test_scan_name_non_sequential():
     assert scan_dir.name == "fake_scan_0012"
 
     all_scans = scan_dir_manager.all_scans
-    assert len(all_scans) == 7
+    assert len(set(all_scans)) == 7
     assert "fake_scan_0001" in all_scans
     assert "fake_scan_0002" in all_scans
     assert "fake_scan_0003" in all_scans
@@ -192,15 +204,14 @@ def test_scan_info():
     now = time.time()
 
     assert info.name == "fake_scan_0001"
-    # Created and modifies in the last 5 seconds
+    # Created and modified in the last 5 seconds
-    assert now - 5 < info.created < now
-    # Created and modifies in the last 5 seconds
     assert now - 5 < info.created < now
+    assert now - 5 < info.modified < now
     assert info.number_of_images == 17
     assert not info.stitch_available
     assert info.dzi is None
 
-    # Add a fake scan and check this recognised as a stitch not a scan image
+    # Add a fake scan and check this is recognised as a stitch not a scan image
     _add_fake_file(scan_dir, "fake_scan_0001_stitched.jpg", in_im_dir=True)
     info = scan_dir.scan_info()
     assert info.number_of_images == 17
@@ -217,10 +228,9 @@ def test_empty_scan_info():
     now = time.time()
 
     assert info.name == "fake_scan_0001"
-    # Created and modifies in the last 5 seconds
+    # Created and modified in the last 5 seconds
-    assert now - 5 < info.created < now
-    # Created and modifies in the last 5 seconds
     assert now - 5 < info.created < now
+    assert now - 5 < info.modified < now
     assert info.number_of_images == 0
     assert not info.stitch_available
     assert info.dzi is None
@@ -236,7 +246,7 @@ def test_zipping_scan_data():
         scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR)
         scan_dir = scan_dir_manager.new_scan_dir("fake_scan")
 
-        # Create 21 fake scan files, a fake stitch, and a fake zip
+        # Create 21 fake scan images, a fake stitch, and a fake zip
         for i in range(21):
             _add_fake_image(scan_dir)
         _add_fake_file(scan_dir, "fake_scan_0001_stitched.jpg", in_im_dir=True)
@@ -249,7 +259,7 @@ def test_zipping_scan_data():
         else:
             zip_fname = scan_dir_manager.zip_scan("fake_scan_0001")
         zip_files = get_files_in_zip(zip_fname)
-        assert len(zip_files) == 21
+        assert len(set(zip_files)) == 21
 
         # Zip again with final version on and there should be 22 images
         if caller == "scan_dir":
@@ -257,7 +267,7 @@ def test_zipping_scan_data():
         else:
             scan_dir_manager.zip_scan("fake_scan_0001", final_version=True)
         zip_files = get_files_in_zip(zip_fname)
-        assert len(get_files_in_zip(zip_fname)) == 22
+        assert len(set(get_files_in_zip(zip_fname))) == 22
         # Check the zips are not in the zip
         for file in zip_files:
             assert not file.endswith(".zip")

tests/test_smart_scan.py

@@ -7,7 +7,7 @@ directly poll any properties and to start any methods.
 Any methods with LabThings dependency injections will require dependencies
 to be passed in manually. Rather than passing in real LabThings clients
 it is possible to create test objects that mock these clients. Thus, entirely
-isolating one Thing for testing, at the expense of neededing to create detailed
+isolating one Thing for testing, at the expense of needing to create detailed
 mock objects.
 
 For these tests to reliably represent real behaviour the mock Things will need to
@@ -41,7 +41,7 @@ LOGGER = logging.getLogger("mock-invocation_logger")
 SCAN_DIR = os.path.join(tempfile.gettempdir(), "scans")
 
 
-def _clear_scan_dir():
+def _clear_scan_dir() -> None:
     """Delete the scan dir"""
     if os.path.exists(SCAN_DIR):
         shutil.rmtree(SCAN_DIR)
@@ -77,6 +77,7 @@ def test_private_delete_scan(smart_scan_thing, caplog):
 
         # Make the outer scan dir, but not the one to delete
         os.makedirs(SCAN_DIR)
+        # Attempt to delete the fake scan. Expect it to fail and provide a warning
         deleted = smart_scan_thing._delete_scan(fake_scan_name, LOGGER)
         assert not deleted
         assert len(caplog.records) == 1
@@ -104,9 +105,10 @@ def test_public_delete_scan(smart_scan_thing, caplog):
         # Make the outer scan dir, but not the one to delete
         os.makedirs(SCAN_DIR)
 
-        with pytest.raises(HTTPException) as exc_info:
+        # Attempt to delete the fake scan. Expect it to fail
-            smart_scan_thing.delete_scan(fake_scan_name, LOGGER)
+    with pytest.raises(HTTPException) as exc_info:
-        # Should raise a 400 error if the scan doesn't exists, not a 404 as the server
+        smart_scan_thing.delete_scan(fake_scan_name, LOGGER)
+        # Should raise a 400 error if the scan doesn't exist, not a 404 as the server
         # was not expecting to receive the scan files
         assert exc_info.value.status_code == 400
         assert len(caplog.records) == 1
@@ -151,7 +153,7 @@ def _run_only_outer_scan(adjust_inital_state: Optional[Callable] = None):
     _run_scan method where this can be tested. Once this is done
     the final scan behaviour can be tested too.
 
-    adjust_initial_scan is a callable which accepts the mocked smart scan thing
+    adjust_initial_state is a callable which accepts the mocked smart scan thing
     as the only variable. It can be used to adjust the initial state of the test
 
 
@@ -177,7 +179,7 @@ def _run_only_outer_scan(adjust_inital_state: Optional[Callable] = None):
         # Counter for checking functions were called
         mock_call_count = {"_run_scan": 0}
 
-        # Mock thing settngs as a dictionary
+        # Mock thing settings as a dictionary
         thing_settings = {"skip_background": True}
 
         def _run_scan(self):
@@ -244,7 +246,7 @@ def test_outer_scan():
     assert mock_ss_thing.mock_call_count["_run_scan"] == 1
 
 
-def test_outer_scan_wo_scample_skip():
+def test_outer_scan_wo_sample_skip():
     """Test setup and teardown of the scan."""
 
     def _set_skip_background(mock_ss_thing):

webapp/src/components/tabContentComponents/scanListContent.vue

@@ -255,7 +255,7 @@ export default {
     formatDate(timestamp) {
       // Multiply by 1000 as JS uses ms not s
       let d = new Date(timestamp*1000);
-      // Convert to a sting in a very javascript way!
+      // Convert to a string in a very javascript way!
       let yyyy = d.getFullYear();
       let mm = d.getMonth() + 1;
       let dd = d.getDate();