Apply suggestions from code review of branch yet-another-smart-scan-refactor

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

src/openflexure_microscope_server/scan_directories.py

@@ -43,7 +43,7 @@ class ScanData(BaseModel):
 
     This serialises into a human readable format where possible with
 
-    timestamps in %Y-%m-%s_%H:%M:%S format
+    timestamps in %Y-%m-%d_%H:%M:%S format
     timedeltas in %H:%M:%S format
 
     Properties that are not known until the end have ``None`` serialised as "Unknown"
@@ -154,9 +154,7 @@ class ScanData(BaseModel):
     @classmethod
     def parse_unknown_as_none(cls, value: Optional[str | int]) -> Optional[str | int]:
         """Validate the string "Unknown" as None."""
-        if value == "Unknown":
+        return None if value == "Unknown" else value
-            return None
-        return value
 
     @field_serializer("scan_result")
     def serialize_none_as_unknown(self, value: Optional[str | int]) -> str | int:
@@ -258,7 +256,7 @@ class ScanDirectoryManager:
     def get_scan_data_dict(self, scan_name: str) -> Optional[dict[str, Any]]:
         """Return the scan data read from a JSON file as a dict.
 
-        This is a dictionary not a base models as the data format has changed
+        This is a dictionary not a base model as the data format has changed
         somewhat over time.
         """
         json_fpath = self.get_scan_data_path(scan_name)

src/openflexure_microscope_server/stitching.py

@@ -3,7 +3,7 @@
 This includes both live stitching and final stitching. This is done via subprocess
 to call openflexure-stitching over CLI. This cannot be done via Threading due to the
 CPU intensity of stitching causing scanning problems due to the Python Global
-Interpreter Lock (GIL). May be possible to shift to multiprocessing int the future.
+Interpreter Lock (GIL). May be possible to shift to multiprocessing in the future.
 """
 
 from typing import Optional, Any
@@ -99,7 +99,7 @@ class BaseStitcher:
         validate_command(full_cmd)
         return full_cmd
 
-    def validate_path(self):
+    def validate_path(self) -> None:
         """Check path is safe before making a command to run with subprocess.
 
         This is essential for stopping arbitrary code execution.
@@ -159,7 +159,7 @@ class PreviewStitcher(BaseStitcher):
                 return True
             return False
 
-    def wait(self):
+    def wait(self) -> None:
         """Wait for this preview stitch to return."""
         if self.running:
             with self._popen_lock:
@@ -167,7 +167,7 @@ class PreviewStitcher(BaseStitcher):
 
 
 class FinalStitcher(BaseStitcher):
-    """A class to handle the final stich for a scan."""
+    """A class to handle the final stitch for a scan."""
 
     def __init__(
         self,

tests/test_scan_data.py

@@ -63,7 +63,7 @@ def test_set_final_data():
 
     # Quickly take 123 images!
     scan_data.image_count += 123
-    # Should set duration based of finishing at datetime.now()
+    # Should set duration based on finishing at datetime.now()
     scan_data.set_final_data(result="Success")
     expected_duration = datetime.now() - MOCK_START_TIME
     expected_duration_s = expected_duration.total_seconds()
@@ -86,7 +86,7 @@ def test_custom_serialisation():
 
     scan_data.image_count += 123
     scan_data.set_final_data(result="Success")
-    # Can't mock datetime.now as datetime is immutable. So just replace duraction
+    # Can't mock datetime.now as datetime is immutable. So just replace duration
     scan_data.duration = MOCK_END_TIME - scan_data.start_time
     scan_data_dict = json.loads(scan_data.model_dump_json())
     assert scan_data_dict["image_count"] == 123

tests/test_scan_directories.py

@@ -309,6 +309,9 @@ def test_get_scan_data_path():
 
     # Remove the images directory.
     shutil.rmtree(scan_dir.images_dir)
+    # When the images directory is deleted, internally `scan_dir.images_dir`
+    # will return `None`. Check that `get_scan_data_path` copes with the `None`
+    # return from `scan_dir.images_dir`.
     assert scan_dir_manager.get_scan_data_path(scan_name) is None
 
 

tests/test_stitching.py

@@ -169,7 +169,7 @@ def _validation_error_tester(scan_path, **kwargs):
 
 
 def test_validation_error():
-    """Test a number of way to try to inject malicious arguments into the stitcher.
+    """Test a number of ways to try to inject malicious arguments into the stitcher.
 
     The stitcher should throw a validation error each attempt.
     """