Merge branch 'Increased-linting' into 'v3'

Enable all lint checkers, remove ruff-increased-checking job!

Closes #548

See merge request openflexure/openflexure-microscope-server!395

.gitlab-ci.yml

@@ -88,18 +88,6 @@ ruff-checks:
     - ruff format --check
   # Only run on changed python files
 
-# Increased checking of the code with ruff this is allowed to fail
-# while we improve the code quality. Rules from this are
-# slowly moved into the main check
-# Only runs when a .py file is edited
-ruff-lint-increased:
-  stage: analysis
-  extends: .python
-  allow_failure: true
-  script:
-    - ruff --config ruff-increased-checking.toml check
-
-
 # Python type checking with Mypy
 # Only runs when a .py file is edited
 mypy:

CONTRIBUTING.md

@@ -41,7 +41,7 @@ Ideally, python code should be:
 
 The server code is going through significant flux during the transition from v2 to v3, so currently our test coverage is being rebuilt and type checking does not yet pass.
 
-[Ruff](https://docs.astral.sh/ruff/) is used both for linting and formatting the codebase. Our custom linter rules are in our `pyproject.toml`. As we improve the codebase, more rules are being added to the linter; see `ruff-increased-checking.toml` for checks that we plan to get passing.
+[Ruff](https://docs.astral.sh/ruff/) is used both for linting and formatting the codebase. Our custom linter rules are in our `pyproject.toml`.
 
 Run `ruff format` to format the code in a way that will pass on our CI.
 

check_version_sync.py

@@ -14,7 +14,8 @@ with open(os.path.join("webapp", "package.json"), "r", encoding="utf-8") as json
 py_ver = pyproject_data["project"]["version"]
 js_ver = webapp_data["version"]
 
-assert py_ver == js_ver, (
+# Allow an assert here as this is a CI script, not production code.
+assert py_ver == js_ver, (  # noqa S101
     "The python and javascript version numbers should match.\n"
     f"Python version: {py_ver}\n"
     f"Javascript version: {js_ver}\n"

hardware-specific-tests/picamera2/test_acquisition.py

@@ -3,14 +3,14 @@
 from fastapi.testclient import TestClient
 from PIL import Image
 import numpy as np
-from pytest import fixture
+import pytest
 
 import labthings_fastapi as lt
 
 from openflexure_microscope_server.things.camera.picamera import StreamingPiCamera2
 
 
-@fixture(scope="module")
+@pytest.fixture(scope="module")
 def client() -> lt.ThingClient:
     """Initialise a test client for the StreamingPiCamera2 Thing.
 

hardware-specific-tests/picamera2/test_calibration.py

@@ -14,7 +14,7 @@ from openflexure_microscope_server.things.camera.picamera import (
 )
 
 
-@pytest.fixture()
+@pytest.fixture
 def picamera_thing() -> StreamingPiCamera2:
     """Return a StreamingPiCamera2 Thing.
 
@@ -24,7 +24,7 @@ def picamera_thing() -> StreamingPiCamera2:
     return StreamingPiCamera2()
 
 
-@pytest.fixture()
+@pytest.fixture
 def client(picamera_thing) -> lt.ThingClient:
     """Initialise a test client for the StreamingPiCamera2 Thing.
 

hardware-specific-tests/picamera2/test_tuning.py

@@ -68,9 +68,6 @@ def _test_bad_tuning_after_good_tuning(configure: bool = False):
     with pytest.raises(IndexError):
         # The bad version should cause a problem
         cam = Picamera2(tuning=bad_tuning)
-        print_tuning()
-        print("Success (not expected)!")
-        del cam
     recalibrate_utils.recreate_camera_manager()
     with Picamera2(tuning=default_tuning) as cam:
         # Reload the camera with working tuning, or it will stop responding

pyproject.toml

@@ -34,7 +34,7 @@ dependencies = [
 
 [project.optional-dependencies]
 dev = [
-  "ruff",
+  "ruff~=0.13.0",
   "mypy[reports]",
   "mypy-gitlab-code-quality",
 #  "pytest-gitlab-code-quality", # pytest version constraint clashes with labthings
@@ -100,32 +100,29 @@ testpaths = ["tests"]
 line-ending = "native"
 
 [tool.ruff.lint]
-
-# Slowly add linting rules from ruff-increased-checking.toml commented
-# out rules can be checked by running
-# ruff --config ruff-increased-checking.toml check
 select = [
     "E", # pycodestyle errors
     "W", #pycodestyle warnings
     "F", # PyFlakes
-#    "PL", # Pylint (a subset of, catches far less than pylint does)
+    "PL", # Pylint (a subset of, catches far less than pylint does)
     "B", # Flake8 bugbear
     "A", # Flake8 builtins checker
     "C", # Flake8 comprehensions
-#    "FIX", #TODO/FIXME's in code. These should be added during development for ongoing tasks.
+    "FIX", #TODO/FIXME's in code. These should be added during development for ongoing tasks.
            # If they are not fixed before merge they should be converted to GitLab issues
-#    "LOG", # Flake8 logging issues (pedantic logger formatting issues can be added with "G")
+    "LOG", # Flake8 logging issues (pedantic logger formatting issues can be added with "G")
-#    "T20", # Warns for print statements, production code should log
+    "T20", # Warns for print statements, production code should log
-#    "PT", # pytest linting
+    "PT", # pytest linting
     "RET", # Consistent clear return statements
     "RSE", # Raise parentheses
-#    "SIM", # Simplifications detected
+    "SIM", # Simplifications detected
     "ARG", # unused arguments
     "C90", # McCabe complexity!
     "NPY", # Numpy linting
     "N", # PEP8 naming
     "D", # Docstring checks
     "ERA001", # Commented out code!
+    "S101",# No asserts!
     "ANN",
 ]
 
@@ -136,20 +133,35 @@ ignore = [
     "D400",  # A stricter version of #415 that doesn't allow !
     "ANN401", # Disalows Any, Any is needed at times, Once MyPy is running Any will be
               # handled appropriately
+    "PLR2004", # Ignore magic values in comparison for now. It doesn't seem we can set
+               # allowed magic values and a number of our magic values are not really
+               # magic (such as 255 when doing uint8 maths)
+    "PT011", # Ignore pytest.raises being used on too general expectations without a
+             # match. We may need to revisit this.
+    "SIM105", # Not enforcing use of `contextlib.suppress(NotConnectedToServerError)`
+              # instead of `try`-`except`-`pass`
 ]
 
 [tool.ruff.lint.per-file-ignores]
-# Tests are currently not fully docstring-ed, we'll ignore this for now.
+# All testing dirs
-"tests/*" = [
+"{tests,integration-tests,hardware-specific-tests}/**" = [
     "B018", # Complaining about useless attribute access in tests, but we need them to check errors are raised
     "ANN", # Tests are not typehinted for fixtures etc
+    "S101", # Allow asserts in tests
+    "T20", # Allow prints in tests
+    "LOG015", # Allow tests to use the root logger.
+    "PLC0415", # Allow tests to import later as this is needed for mocking
 ]
-"hardware-specific-tests/*" = [
+# Developer CLI scripts
-    "B018", # Complaining about useless attribute access in tests, but we need them to check errors are raised
+"{pull_webapp,picamera_tests,change_log_helper}.py" = [
-    "ANN", # Tests are not typehinted for fixtures etc
+    "T20", # Printing is normal in small CLI scripts
 ]
 
 [tool.ruff.lint.pydocstyle]
 # This lets the D401 checker understand that decorated thing properties and thing
 # settings act like properties so should be documented as such.
 property-decorators = ["labthings_fastapi.thing_property", "labthings_fastapi.thing_setting"]
+
+[tool.ruff.lint.pylint]
+max-args = 7
+max-positional-args = 5

ruff-increased-checking.toml

@@ -1,41 +0,0 @@
-# This can be passed into ruff with nano ruff-increased-checking.toml
-#
-# Once the codebase is cleaner this can be added to pyproject.toml
-# under the [tool.ruff.lint] heading 
-#
-# Select more than the bare minimum checking in ruff
-# This is only a subset of possible rules
-# See: https://docs.astral.sh/ruff/rules
-# 
-# It will take a long time to get this all passing. We should
-# slowly move rules into the pyproject.toml as we improve the
-# codebase
-
-select = [
-    "E", # pycodestyle errors
-    "W", #pycodestyle warnings
-    "F", # PyFlakes
-    "PL", # Pylint (a subset of, catches far less than pylint does)
-    "B", # Flake8 bugbear
-    "A", # Flake8 builtins checker
-    "C", # Flake8 comprehensions
-    "FIX", #TODO/FIXME's in code. These should be added during development for ongoing tasks.
-           # If they are not fixed before merge they should be converted to GitLab issues
-    "LOG", # Flake8 logging issues (pedantic logger formatting issues can be added with "G")
-    "T20", # Warns for print statements, production code should log
-    "PT", # pytest linting
-    "RET", # Consistent clear return statements
-    "RSE", # Raise parentheses
-    "SIM", # Simplifications detected
-    "ARG", # unused arguments
-    "C90", # McCabe complexity!
-    "NPY", # Numpy linting
-    "N", # PEP8 naming
-    "D", # Docstring checks these may need to be added gradually
-    "ERA001", # Commented out code!
-]
-
-# Line length is set to 88 for ruff. This is what the formatter aims for
-# some lines are not formatted to 88 if the formatter can't easily break
-# them. Allow up to 99 before throwing a long line error
-line-length = 99

src/openflexure_microscope_server/logging.py

@@ -18,6 +18,7 @@ import os
 from fastapi.responses import PlainTextResponse
 from fastapi import HTTPException
 
+LOGGER = logging.getLogger(__name__)
 OFM_LOG_FILE = None
 
 
@@ -33,7 +34,8 @@ def configure_logging(log_folder: str) -> None:
     root_logger = logging.getLogger()
     root_logger.setLevel(logging.INFO)
     # Explicitly make OFM_LOG_FILE a global so it can be updated based on log settings
-    global OFM_LOG_FILE
+    # This requires silencing PLW0603 which disallows globals.
+    global OFM_LOG_FILE  # noqa: PLW0603
     OFM_LOG_FILE = os.path.join(log_folder, "openflexure_microscope.log")
 
     # Add OFM_HANDLER first so it can capture the error log if the
@@ -58,12 +60,9 @@ def configure_logging(log_folder: str) -> None:
         root_logger.addHandler(handler)
 
     except PermissionError as e:
-        logging.error(f"Cannot create log file at {OFM_LOG_FILE}: {e}")
+        LOGGER.error(f"Cannot create log file at {OFM_LOG_FILE}: {e}")
 
-    logging.info("")
+    LOGGER.info("OFM server root logger has been set up at INFO level")
-    logging.info("****************************************************")
-    logging.info("OFM server root logger has been set up at INFO level")
-    logging.info("****************************************************")
 
 
 def retrieve_log() -> PlainTextResponse:

src/openflexure_microscope_server/scan_directories.py

@@ -210,9 +210,8 @@ class ScanDirectoryManager:
         not exist.
         """
         file_path = os.path.join(self.path_for(scan_name), filename)
-        if check_exists:
+        if check_exists and not os.path.exists(file_path):
-            if not os.path.exists(file_path):
+            return None
-                return None
         return file_path
 
     @requires_lock
@@ -225,9 +224,8 @@ class ScanDirectoryManager:
         then the path is returned anyway
         """
         file_path = os.path.join(self.img_dir_for(scan_name), filename)
-        if check_exists:
+        if check_exists and not os.path.exists(file_path):
-            if not os.path.exists(file_path):
+            return None
-                return None
         return file_path
 
     def get_final_stitch_path(self, scan_name: str) -> Optional[str]:
@@ -528,7 +526,8 @@ class ScanDirectory:
         """
         zip_fname = os.path.join(self.dir_path, "images.zip")
 
-        if os.path.isfile(zip_fname):
+        # Use noqa as converting this into a 1 liner is not more readable.
+        if os.path.isfile(zip_fname):  # noqa: SIM108
             # get a list of files in the existing zip
             zip_files = get_files_in_zip(zip_fname)
         else:

src/openflexure_microscope_server/scan_planners.py

@@ -163,10 +163,7 @@ class ScanPlanner:
 
         # If focussed locations exist return closest location, favouring most recent
         closest_pos = self.closest_focus_site(next_location)
-        if closest_pos is None:
+        z = None if closest_pos is None else closest_pos[2]
-            z = None
-        else:
-            z = closest_pos[2]
 
         return next_location, z
 
@@ -363,10 +360,7 @@ class SmartSpiral(ScanPlanner):
 
         # If focused locations exist, return the neighbour with the lowest z position
         closest_pos = self.select_nearby_focus_site(next_location)
-        if closest_pos is None:
+        z = None if closest_pos is None else closest_pos[2]
-            z = None
-        else:
-            z = closest_pos[2]
 
         return next_location, z
 

src/openflexure_microscope_server/server/__init__.py

@@ -14,6 +14,8 @@ from .serve_static_files import add_static_files
 from .legacy_api import add_v2_endpoints
 from ..logging import configure_logging, retrieve_log, retrieve_log_from_file
 
+LOGGER = logging.getLogger(__name__)
+
 
 def set_shutdown_function(shutdown_function: Callable[[], None]) -> None:
     """Ensure a function is called before the shutdown.
@@ -93,7 +95,7 @@ def serve_from_cli(argv: Optional[list[str]] = None) -> None:
                 # Catch anything and log as it is essential that this
                 # function cannot raise an unhandled exception or Uvicorn
                 # will never get a shutdown signal.
-                logging.error(e, exc_info=True)
+                LOGGER.error(e, exc_info=True)
 
         # Monkey patch uvicorn's exit handling to stop the MJPEG Streams
         # before waiting for background tasks to complete.
@@ -109,9 +111,11 @@ def serve_from_cli(argv: Optional[list[str]] = None) -> None:
 
     except BaseException as e:
         if args.fallback:
-            print(f"Error: {e}")
+            # Allow printing to the terminal for fallback errors so they are not
+            # presented in the fallback logs.
+            print(f"Error: {e}")  # noqa: T201
             fallback_server = "labthings_fastapi.server.fallback:app"
-            print(f"Starting fallback server {fallback_server}.")
+            print(f"Starting fallback server {fallback_server}.")  # noqa: T201
             app = lt.cli.object_reference_to_object(fallback_server)
             app.labthings_config = config
             app.labthings_server = server

src/openflexure_microscope_server/server/legacy_api.py

@@ -21,10 +21,9 @@ def add_v2_endpoints(thing_server: lt.ThingServer) -> None:
     """Add the v2 API endpoints for OpenFlexure Connect discoverability."""
     app = thing_server.app
 
-    # TODO: update openflexure connect to make this unnecessary!!
     # The endpoints below fool OpenFlexure Connect into thinking we are a
     # v2 microscope, so we show up correctly.
-    # This is necessary until Connect is rebuilt.
+    # This is necessary until Connect is rebuilt. See #557.
     @app.get("/routes")
     def routes_stub() -> dict[str, dict]:
         """Return a stub list of routes.

src/openflexure_microscope_server/stitching.py

@@ -165,9 +165,7 @@ class PreviewStitcher(BaseStitcher):
         with self._popen_lock:
             if self._popen_obj is None:
                 return False
-            if self._popen_obj.poll() is None:
+            return self._popen_obj.poll() is None
-                return True
-            return False
 
     def wait(self, cancel: lt.deps.CancelHook) -> None:
         """Wait for this preview stitch to return.

src/openflexure_microscope_server/things/autofocus.py

@@ -25,6 +25,8 @@ from .camera import RawCameraDependency as RawCamera
 from .camera import CameraDependency as CameraClient
 from .stage import StageDependency as Stage
 
+LOGGER = logging.getLogger(__name__)
+
 
 class NotStreamingError(RuntimeError):
     """No images captured from stream. The camera is almost certainly not streaming."""
@@ -228,7 +230,7 @@ class JPEGSharpnessMonitor:
         self.camera = camera
         self.stage = stage
         self.portal = portal
-        print(f"Created sharpness monitor with {stage}, {camera}, {portal}")
+        LOGGER.debug(f"Created sharpness monitor with {stage}, {camera}, {portal}")
         self.stage_positions: list[Mapping[str, int]] = []
         self.stage_times: list[float] = []
         self.jpeg_times: list[float] = []
@@ -307,7 +309,7 @@ class JPEGSharpnessMonitor:
             raise e
         if stop < 1:
             stop = len(jpeg_times)
-            logging.debug("changing stop to %s", (stop))
+            LOGGER.debug("changing stop to %s", (stop))
         jpeg_times = jpeg_times[start:stop]
         jpeg_heights: np.ndarray = np.interp(jpeg_times, stage_times, stage_heights)
         return jpeg_times, jpeg_heights, jpeg_sizes[start:stop]
@@ -704,7 +706,10 @@ class AutofocusThing(lt.Thing):
             sharpness=cam.grab_jpeg_size(stream_name="lores"),
         )
 
-    def check_stack_result(
+    # Silence too many returns in this situation as refactoring to reduce returns is
+    # unlikely to improve readability. This function is basically a complex switch
+    # statement, having an explicit return after each option is clear.
+    def check_stack_result(  # noqa: PLR0911
         self, captures: list[CaptureInfo]
     ) -> tuple[Literal["success", "continue", "restart"], int]:
         """Check if the sharpest image in a list of captures is central enough.

src/openflexure_microscope_server/things/camera/opencv.py

@@ -21,6 +21,8 @@ from labthings_fastapi.types.numpy import NDArray
 
 from . import BaseCamera
 
+LOGGER = logging.getLogger(__name__)
+
 
 class OpenCVCamera(BaseCamera):
     """A Thing that provides and interface to an OpenCV Camera."""
@@ -70,9 +72,7 @@ class OpenCVCamera(BaseCamera):
         while self._capture_enabled:
             ret, frame = self.cap.read()
             if not ret:
-                logging.error(
+                LOGGER.error(f"Failed to capture frame from camera {self.camera_index}")
-                    f"Failed to capture frame from camera {self.camera_index}"
-                )
                 break
             jpeg = cv2.imencode(".jpg", frame)[1].tobytes()
             self.mjpeg_stream.add_frame(jpeg, portal)
@@ -99,8 +99,8 @@ class OpenCVCamera(BaseCamera):
         binary image formats will be added in due course.
         """
         if wait is not None:
-            logging.warning("OpenCV camera has no wait option. Use None.")
+            LOGGER.warning("OpenCV camera has no wait option. Use None.")
-        logging.warning(f"OpenCV camera doesn't respect {stream_name=}")
+        LOGGER.warning(f"OpenCV camera doesn't respect {stream_name=}")
         ret, frame = self.cap.read()
         if not ret:
             raise RuntimeError(
@@ -118,6 +118,6 @@ class OpenCVCamera(BaseCamera):
         This function will produce a JPEG image.
         """
         if wait is not None:
-            logging.warning("OpenCV camera has no wait option. Use None.")
+            LOGGER.warning("OpenCV camera has no wait option. Use None.")
-        logging.warning(f"OpenCV camera doesn't respect {stream_name=}")
+        LOGGER.warning(f"OpenCV camera doesn't respect {stream_name=}")
         return Image.fromarray(self.capture_array())

src/openflexure_microscope_server/things/camera/picamera.py

@@ -47,6 +47,8 @@ from . import picamera_tuning_file_utils as tf_utils
 
 from . import BaseCamera, ArrayModel
 
+LOGGER = logging.getLogger(__name__)
+
 SUPPORTED_CAMS_SENSOR_INFO = {
     "picamera_v2": recalibrate_utils.IMX219_SENSOR_INFO,
     "picamera_hq": recalibrate_utils.IMX477_SENSOR_INFO,
@@ -397,16 +399,16 @@ class StreamingPiCamera2(BaseCamera):
             os.environ["LIBCAMERA_RPI_TUNING_FILE"] = tuning_file.name
 
             if self._picamera is not None:
-                logging.info("Closing picamera object for reinitialisation")
+                LOGGER.info("Closing picamera object for reinitialisation")
-                logging.info(
+                LOGGER.info(
                     "Camera object already exists, closing for reinitialisation"
                 )
                 self._picamera.close()
-                logging.info("Picamera closed, deleting picamera")
+                LOGGER.info("Picamera closed, deleting picamera")
                 del self._picamera
                 recalibrate_utils.recreate_camera_manager()
 
-            logging.info("Creating new Picamera2 object")
+            LOGGER.info("Creating new Picamera2 object")
             # Specify tuning file otherwise it will be overwritten with None.
             self._picamera = Picamera2(
                 camera_num=self._camera_num,
@@ -519,7 +521,7 @@ class StreamingPiCamera2(BaseCamera):
                 )
                 stream_config["buffer_count"] = buffer_count
                 picam.configure(stream_config)
-                logging.info("Starting picamera MJPEG stream...")
+                LOGGER.info("Starting picamera MJPEG stream...")
                 stream_name = "lores" if main_resolution[0] > 1280 else "main"
                 picam.start_recording(
                     MJPEGEncoder(self.mjpeg_bitrate),
@@ -538,11 +540,11 @@ class StreamingPiCamera2(BaseCamera):
                     name="lores",
                 )
             except Exception as e:
-                logging.exception("Error while starting preview: {e}")
+                LOGGER.exception("Error while starting preview: {e}")
-                logging.exception(e)
+                LOGGER.exception(e)
             else:
                 self.stream_active = True
-                logging.debug(
+                LOGGER.debug(
                     "Started MJPEG stream at %s on port %s", self.stream_resolution, 1
                 )
 
@@ -553,15 +555,15 @@ class StreamingPiCamera2(BaseCamera):
             try:
                 picam.stop_recording()  # This should also stop the extra lores encoder
             except Exception as e:
-                logging.info("Stopping recording failed")
+                LOGGER.info("Stopping recording failed")
-                logging.exception(e)
+                LOGGER.exception(e)
             else:
                 self.stream_active = False
                 if stop_web_stream:
                     portal = lt.get_blocking_portal(self)
                     self.mjpeg_stream.stop(portal)
                     self.lores_mjpeg_stream.stop(portal)
-                logging.info("Stopped MJPEG stream.")
+                LOGGER.info("Stopped MJPEG stream.")
 
             # Adding a sleep to prevent camera getting confused by rapid commands
             time.sleep(self._sensor_info.short_pause)
@@ -579,7 +581,7 @@ class StreamingPiCamera2(BaseCamera):
         Restarts stream when complete.
         """
         with self._streaming_picamera(pause_stream=True) as cam:
-            logging.debug("Reconfiguring camera for full resolution capture")
+            LOGGER.debug("Reconfiguring camera for full resolution capture")
             cam.configure(cam.create_still_configuration(sensor=self._sensor_mode))
             cam.start()
             time.sleep(self._sensor_info.short_pause)

src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py

@@ -52,6 +52,8 @@ from scipy.ndimage import zoom
 from picamera2 import Picamera2
 import picamera2
 
+LOGGER = logging.getLogger(__name__)
+
 
 class SensorInfo(BaseModel):
     """Information about the sensor used for calibration and property setting."""
@@ -167,7 +169,7 @@ def adjust_shutter_and_gain_from_raw(
 
         # Check whether the shutter speed is still going up - if not, we've hit a maximum
         if camera.capture_metadata()["ExposureTime"] == test.exposure_time:
-            logging.info(f"Shutter speed has maxed out at {test.exposure_time}")
+            LOGGER.info(f"Shutter speed has maxed out at {test.exposure_time}")
             break
 
     # Now, if we've not converged, increase gain until we converge or run out of options.
@@ -185,13 +187,13 @@ def adjust_shutter_and_gain_from_raw(
 
         # Check the gain is still changing - if not, we have probably hit the maximum
         if camera.capture_metadata()["AnalogueGain"] == test.analog_gain:
-            logging.info(f"Gain has maxed out at {test.analog_gain}")
+            LOGGER.info(f"Gain has maxed out at {test.analog_gain}")
             break
 
     if _check_convergence(test, target_white_level, tolerance):
-        logging.info(f"Brightness has converged to within {tolerance * 100:.0f}%.")
+        LOGGER.info(f"Brightness has converged to within {tolerance * 100:.0f}%.")
     else:
-        logging.warning(
+        LOGGER.warning(
             f"Failed to reach target brightness of {target_white_level}."
             f"Brightness reached {test.level} after {iterations} iterations."
         )
@@ -199,9 +201,13 @@ def adjust_shutter_and_gain_from_raw(
     return test.level
 
 
-def adjust_white_balance_from_raw(
+# Explicitly allow this to have 8 arguments as the later arguments are keyword only
+# We should be able to enforce this without noqa once PyLint moves PLR0917 out of
+# preview
+def adjust_white_balance_from_raw(  # noqa: PLR0913
     camera: Picamera2,
     sensor_info: SensorInfo,
+    *,
     percentile: float = 99,
     luminance: Optional[np.ndarray] = None,
     Cr: Optional[np.ndarray] = None,
@@ -230,11 +236,9 @@ def adjust_white_balance_from_raw(
         channel_gains = 1 / grids
         if channel_gains.shape[1:] != channels.shape[1:]:
             channel_gains = _upsample_channels(channel_gains, channels.shape[1:])
-        logging.info(
+        LOGGER.info(f"Before gains, channel maxima are {np.max(channels, axis=(1, 2))}")
-            f"Before gains, channel maxima are {np.max(channels, axis=(1, 2))}"
-        )
         channels = channels * channel_gains
-        logging.info(f"After gains, channel maxima are {np.max(channels, axis=(1, 2))}")
+        LOGGER.info(f"After gains, channel maxima are {np.max(channels, axis=(1, 2))}")
     if method == "centre":
         _, height, width = channels.shape
         # Cut out the central 10% from 9/20 to 11/20...
@@ -261,7 +265,7 @@ def adjust_white_balance_from_raw(
         # Here, we decrease the gains by the minimum value of Cr and Cb.
         new_awb_gains = (green / red * np.min(Cr), green / blue * np.min(Cb))
 
-    logging.info(
+    LOGGER.info(
         f"Raw white point is R: {red} G: {green} B: {blue}, "
         f"setting AWB gains to ({new_awb_gains[0]:.2f}, "
         f"{new_awb_gains[1]:.2f})."
@@ -270,7 +274,7 @@ def adjust_white_balance_from_raw(
     camera.controls.ColourGains = new_awb_gains
     time.sleep(sensor_info.long_pause)
     m = camera.capture_metadata()
-    print(f"Camera confirms gains are now {m['ColourGains']}")
+    LOGGER.debug(f"Camera confirms gains are now {m['ColourGains']}")
     return new_awb_gains
 
 
@@ -334,7 +338,7 @@ def _test_exposure_settings(camera: Picamera2, percentile: float) -> _ExposureTe
     # because of black level compensation.  The line below forces a
     # minimum value of 1 which will keep things well-behaved!
     if max_brightness < 1:
-        logging.warning(
+        LOGGER.warning(
             f"Measured brightness of {max_brightness}. "
             "This should normally be >= 1, and may indicate the "
             "camera's black level compensation has gone wrong."
@@ -346,7 +350,7 @@ def _test_exposure_settings(camera: Picamera2, percentile: float) -> _ExposureTe
         exposure_time=int(metadata["ExposureTime"]),
         analog_gain=float(metadata["AnalogueGain"]),
     )
-    logging.info(f"{result.model_dump()}")
+    LOGGER.info(f"{result.model_dump()}")
     return result
 
 
@@ -490,5 +494,5 @@ def _raw_channels_from_camera(
     # de-mosaicing has been done, so 2/3 of the values are zero (3/4 for R and B
     # channels, 1/2 for green because there's twice as many green pixels).
     raw_format = camera.camera_configuration()["raw"]["format"]
-    print(f"Acquired a raw image in format {raw_format}")
+    LOGGER.debug(f"Acquired a raw image in format {raw_format}")
     return _channels_from_bayer_array(raw_image)

src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py

@@ -38,7 +38,8 @@ def set_static_lst(
     adaptive tweaking by the algorithm.
     """
     for table in luminance, cr, cb:
-        assert np.array(table).shape == (12, 16), "Lens shading tables must be 12x16!"
+        if np.array(table).shape != (12, 16):
+            raise ValueError("Lens shading tables must be 12x16!")
     alsc = Picamera2.find_tuning_algo(tuning, "rpi.alsc")
     alsc["n_iter"] = 0  # disable the adaptive part
     alsc["luminance_strength"] = 1.0

src/openflexure_microscope_server/things/camera/simulation.py

@@ -30,6 +30,8 @@ from openflexure_microscope_server.ui import (
 from . import BaseCamera, ArrayModel
 from ..stage import BaseStage
 
+LOGGER = logging.getLogger(__name__)
+
 # The ratio between "motor" steps and pixels
 # higher related to a faster movement
 RATIO = 0.2
@@ -204,7 +206,7 @@ class SimulatedCamera(BaseCamera):
         try:
             pos = self.get_stage_position()
         except Exception as e:
-            print(f"Failed to get stage position: {e}")
+            LOGGER.debug(f"Failed to get stage position: {e}")
             pos = {"x": 0, "y": 0, "z": 0}
         return self.generate_image((pos["y"], pos["x"], pos["z"]))
 
@@ -238,7 +240,7 @@ class SimulatedCamera(BaseCamera):
         If called while already streaming, the warning will be emitted and no other
         action will be taken.
         """
-        logging.warning(
+        LOGGER.warning(
             f"Simulation camera doesn't respect {main_resolution=} or {buffer_count=} "
             "arguments."
         )
@@ -271,7 +273,7 @@ class SimulatedCamera(BaseCamera):
                 jpeg_lores = cv2.imencode(".jpg", ds_frame)[1].tobytes()
                 self.lores_mjpeg_stream.add_frame(jpeg_lores, portal)
             except Exception as e:
-                logging.exception(f"Failed to capture frame: {e}, retrying...")
+                LOGGER.exception(f"Failed to capture frame: {e}, retrying...")
 
     @lt.thing_action
     def discard_frames(self) -> None:
@@ -293,8 +295,8 @@ class SimulatedCamera(BaseCamera):
         binary image formats will be added in due course.
         """
         if wait is not None:
-            logging.warning("Simulation camera has no wait option. Use None.")
+            LOGGER.warning("Simulation camera has no wait option. Use None.")
-        logging.warning(f"Simulation camera camera doesn't respect {stream_name=}")
+        LOGGER.warning(f"Simulation camera camera doesn't respect {stream_name=}")
         return self.generate_frame()
 
     def capture_image(
@@ -307,8 +309,8 @@ class SimulatedCamera(BaseCamera):
         It is used for capture to memory.
         """
         if wait is not None:
-            logging.warning("Simulation camera has no wait option. Use None.")
+            LOGGER.warning("Simulation camera has no wait option. Use None.")
-        logging.warning(f"Simulation camera camera doesn't respect {stream_name=}")
+        LOGGER.warning(f"Simulation camera camera doesn't respect {stream_name=}")
         return Image.fromarray(self.generate_frame())
 
     @lt.thing_action

src/openflexure_microscope_server/things/smart_scan.py

@@ -334,9 +334,8 @@ class SmartScanThing(lt.Thing):
         """Manage the stitching threads, starting them if needed and not already running."""
         # Assume 4 images means at least one offset in x and y, making the stitching
         # well constrained.
-        if self._scan_data.image_count > 3:
+        if self._scan_data.image_count > 3 and not self._preview_stitcher.running:
-            if not self._preview_stitcher.running:
+            self._preview_stitcher.start()
-                self._preview_stitcher.start()
 
     @_scan_running
     def _run_scan(self) -> None:

src/openflexure_microscope_server/things/stage/sangaboard.py

@@ -15,6 +15,8 @@ import labthings_fastapi as lt
 
 from . import BaseStage
 
+LOGGER = logging.getLogger(__name__)
+
 
 class SangaboardThing(BaseStage):
     """A Thing to manage a Sangaboard motor controller.
@@ -168,7 +170,7 @@ class SangaboardThing(BaseStage):
             # cannot be used.
             intended_brightness = float(return_value[7:])
             on_brightness = 0.32
-            logging.warning(
+            LOGGER.warning(
                 "Brightness control is not yet implemented. Desired brightness: "
                 f"{intended_brightness}. Set brightness: {on_brightness}"
             )

src/openflexure_microscope_server/things/system.py

@@ -52,7 +52,6 @@ class OpenFlexureSystem(lt.Thing):
 
     @microscope_id.setter
     def microscope_id(self, uuid: UUID) -> None:
-        # TODO make read only but still settable from disk
         self._microscope_id = uuid
 
     @lt.thing_property

tests/test_camera.py

@@ -54,7 +54,11 @@ def test_handle_broken_frame():
         portal = lt.get_blocking_portal(camera)
 
         # Check that this does cause broken frames.
-        with pytest.raises(OSError, match="broken data stream when reading image file"):
+        # The noqa is because we don't know exactly when the error is thrown so we
+        # can't have a single simple statement in the pytest raises.
+        with pytest.raises(  # noqa PT012
+            OSError, match="broken data stream when reading image file"
+        ):
             for _i in range(15):
                 jpeg = camera.grab_jpeg(portal)
                 np.asarray(Image.open(jpeg.open()))

tests/test_dummy_server.py

@@ -42,8 +42,8 @@ def thing_server():
     server.add_thing(CameraStageMapper(), "/camera_stage_mapping/")
     assert os.path.exists(os.path.join(temp_folder.name, "camera/"))
     # Note: yield is important. If return is used the temp folder gets deleted
-    # before the test runs
+    # before the test runs. Silence PT022 as ruff doesn't think yield is needed.
-    yield server
+    yield server  # noqa: PT022
 
 
 @pytest.fixture

tests/test_logging.py

@@ -22,15 +22,14 @@ def test_no_warnings_if_correct_permissions(caplog):
     """
     # Reset handler at start of test
     ofm_logging.OFM_HANDLER = ofm_logging.OFMHandler()
-    with caplog.at_level(logging.WARNING):
+    with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir:
-        with tempfile.TemporaryDirectory() as tmpdir:
+        ofm_logging.configure_logging(tmpdir)
-            ofm_logging.configure_logging(tmpdir)
+        assert len(caplog.records) == 0
-            assert len(caplog.records) == 0
+        with open(ofm_logging.OFM_LOG_FILE, "r", encoding="utf-8") as log_file:
-            with open(ofm_logging.OFM_LOG_FILE, "r", encoding="utf-8") as log_file:
+            log_txt = log_file.read()
-                log_txt = log_file.read()
+        assert "OFM server root logger has been set up at INFO level" in log_txt
-            assert "OFM server root logger has been set up at INFO level" in log_txt
+        root_logger = logging.getLogger()
-            root_logger = logging.getLogger()
+        assert ofm_logging.OFM_HANDLER in root_logger.handlers
-            assert ofm_logging.OFM_HANDLER in root_logger.handlers
 
 
 def test_permission_error_raises_warning(mocker, caplog):
@@ -41,26 +40,24 @@ def test_permission_error_raises_warning(mocker, caplog):
     )
     # Reset handler at start of test
     ofm_logging.OFM_HANDLER = ofm_logging.OFMHandler()
-    with caplog.at_level(logging.WARNING):
+    with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir:
-        with tempfile.TemporaryDirectory() as tmpdir:
+        ofm_logging.configure_logging(tmpdir)
-            ofm_logging.configure_logging(tmpdir)
+        assert len(caplog.records) == 1
-            assert len(caplog.records) == 1
+        # Check OFM logger is added even if the file logger couldn't be.
-            # Check OFM logger is added even if the file logger couldn't be.
+        root_logger = logging.getLogger()
-            root_logger = logging.getLogger()
+        assert ofm_logging.OFM_HANDLER in root_logger.handlers
-            assert ofm_logging.OFM_HANDLER in root_logger.handlers
 
 
 def test_making_log_dir(caplog):
     """Check that configure_logging will make a dir if needed."""
     # Reset handler at start of test
     ofm_logging.OFM_HANDLER = ofm_logging.OFMHandler()
-    with caplog.at_level(logging.WARNING):
+    with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir:
-        with tempfile.TemporaryDirectory() as tmpdir:
+        log_dir = os.path.join(tmpdir, "new_dir")
-            log_dir = os.path.join(tmpdir, "new_dir")
+        assert not os.path.isdir(log_dir)
-            assert not os.path.isdir(log_dir)
+        ofm_logging.configure_logging(log_dir)
-            ofm_logging.configure_logging(log_dir)
+        assert len(caplog.records) == 0
-            assert len(caplog.records) == 0
+        assert os.path.isdir(log_dir)
-            assert os.path.isdir(log_dir)
 
 
 def test_max_logs():
@@ -161,13 +158,13 @@ FAKE_UVICORN_LOGGER = logging.getLogger("uvicorn.error")
 
 
 @pytest.mark.parametrize(
-    "log_command, names_in_log, names_not_in_log",
+    ("log_command", "names_in_log", "names_not_in_log"),
     [
-        [FAKE_UVICORN_LOGGER.debug, [], ["<uvicorn>", "<uvicorn.error>"]],
+        (FAKE_UVICORN_LOGGER.debug, [], ["<uvicorn>", "<uvicorn.error>"]),
-        [FAKE_UVICORN_LOGGER.info, ["<uvicorn>"], ["<uvicorn.error>"]],
+        (FAKE_UVICORN_LOGGER.info, ["<uvicorn>"], ["<uvicorn.error>"]),
-        [FAKE_UVICORN_LOGGER.warning, ["<uvicorn>"], ["<uvicorn.error>"]],
+        (FAKE_UVICORN_LOGGER.warning, ["<uvicorn>"], ["<uvicorn.error>"]),
-        [FAKE_UVICORN_LOGGER.error, ["<uvicorn.error>"], ["<uvicorn>"]],
+        (FAKE_UVICORN_LOGGER.error, ["<uvicorn.error>"], ["<uvicorn>"]),
-        [FAKE_UVICORN_LOGGER.exception, ["<uvicorn.error>"], ["<uvicorn>"]],
+        (FAKE_UVICORN_LOGGER.exception, ["<uvicorn.error>"], ["<uvicorn>"]),
     ],
 )
 def test_uvicorn_error_only_says_error_on_error(

tests/test_scan_planners.py

@@ -23,8 +23,8 @@ def test_enforce_xy_tuple():
         with pytest.raises(TypeError):
             scan_planners.enforce_xy_tuple(value)
 
-    assert (1, 6) == scan_planners.enforce_xy_tuple((1, 6))
+    assert scan_planners.enforce_xy_tuple((1, 6)) == (1, 6)
-    assert (1, 6) == scan_planners.enforce_xy_tuple([1, 6])
+    assert scan_planners.enforce_xy_tuple([1, 6]) == (1, 6)
 
 
 def test_enforce_xyz_tuple():
@@ -39,8 +39,8 @@ def test_enforce_xyz_tuple():
         with pytest.raises(TypeError):
             scan_planners.enforce_xyz_tuple(value)
 
-    assert (1, 6, 2) == scan_planners.enforce_xyz_tuple((1, 6, 2))
+    assert scan_planners.enforce_xyz_tuple((1, 6, 2)) == (1, 6, 2)
-    assert (1, 6, 6) == scan_planners.enforce_xyz_tuple([1, 6, 6])
+    assert scan_planners.enforce_xyz_tuple([1, 6, 6]) == (1, 6, 6)
 
 
 def test_base_class_not_implemented():

tests/test_serve_static_files.py

@@ -26,12 +26,12 @@ def mock_static_dir():
 
 
 @pytest.mark.parametrize(
-    "filename, allow_cache",
+    ("filename", "allow_cache"),
     [
-        ["foo", False],
+        ("foo", False),
-        ["woff2.foo", False],
+        ("woff2.foo", False),
-        ["strange_file_ending_in_woff2", False],
+        ("strange_file_ending_in_woff2", False),
-        ["fontfile.woff2", True],
+        ("fontfile.woff2", True),
     ],
 )
 def test_add_static_file(filename, allow_cache, mocker):

tests/test_server_config.py

@@ -86,8 +86,8 @@ def test_customise_server(mocker):
 
 
 @pytest.mark.parametrize(
-    "config_file, expected_scan_dir",
+    ("config_file", "expected_scan_dir"),
-    [[FULL_CONFIG, "/var/openflexure/scans/"], [SIM_CONFIG, "./openflexure/scans/"]],
+    [(FULL_CONFIG, "/var/openflexure/scans/"), (SIM_CONFIG, "./openflexure/scans/")],
 )
 def test_get_scans_dir_ok(config_file, expected_scan_dir):
     """Test the _get_scans_dir function and also check the standard config files."""

tests/test_smart_scan.py

@@ -120,20 +120,20 @@ def test_public_delete_scan(smart_scan_thing, caplog):
         # Attempt to delete the fake scan. Expect it to fail
     with pytest.raises(HTTPException) as exc_info:
         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
+    # 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
+    # was not expecting to receive the scan files
-        assert exc_info.value.status_code == 400
+    assert exc_info.value.status_code == 400
-        assert len(caplog.records) == 1
+    assert len(caplog.records) == 1
-        assert caplog.records[0].levelname == "WARNING"
+    assert caplog.records[0].levelname == "WARNING"
-        assert caplog.records[0].name == "mock-invocation_logger"
+    assert caplog.records[0].name == "mock-invocation_logger"
 
-        # Make a dir for the fake scan and delete it.
+    # Make a dir for the fake scan and delete it.
-        os.makedirs(fake_scan_path)
+    os.makedirs(fake_scan_path)
-        assert os.path.exists(fake_scan_path)
+    assert os.path.exists(fake_scan_path)
-        smart_scan_thing.delete_scan(fake_scan_name, LOGGER)
+    smart_scan_thing.delete_scan(fake_scan_name, LOGGER)
-        assert not os.path.exists(fake_scan_path)
+    assert not os.path.exists(fake_scan_path)
-        # Check no extra logs generated
+    # Check no extra logs generated
-        assert len(caplog.records) == 1
+    assert len(caplog.records) == 1
 
 
 def test_delete_all_scans(smart_scan_thing, caplog):

tests/test_version_strings.py

@@ -82,7 +82,9 @@ def git_repo(temp_dir):
     _git("add -A")
     _git("commit -m 'message2'")
 
-    yield temp_dir
+    # Yielding here as tempdir is yielding and we want to stay in the tempdir
+    # context manager. Silencing ruff saying it should be return.
+    yield temp_dir  # noqa: PT022
 
 
 def test_version_data_git_and_toml(mocker, git_repo):