From ef89b536fcc0a6dd6d6f0f777ec4934937e08d10 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 11:39:02 +0100 Subject: [PATCH 01/10] Remove assert statements from production code --- check_version_sync.py | 3 ++- pyproject.toml | 9 +++------ .../things/camera/picamera_tuning_file_utils.py | 3 ++- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/check_version_sync.py b/check_version_sync.py index 23c13b1b..40a4628e 100755 --- a/check_version_sync.py +++ b/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" diff --git a/pyproject.toml b/pyproject.toml index 9957a3b6..3cef0b2d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -126,6 +126,7 @@ select = [ "N", # PEP8 naming "D", # Docstring checks "ERA001", # Commented out code! + "S101",# No asserts! "ANN", ] @@ -139,14 +140,10 @@ ignore = [ ] [tool.ruff.lint.per-file-ignores] -# Tests are currently not fully docstring-ed, we'll ignore this for now. -"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 -] -"hardware-specific-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 ] [tool.ruff.lint.pydocstyle] diff --git a/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py b/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py index 00e5a0d3..f86753c7 100644 --- a/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py +++ b/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 From 338d49f9e613dce0b796637e36a078c6e47c1e8b Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 14:09:20 +0100 Subject: [PATCH 02/10] Remove remaining TODOs in favour of issues --- pyproject.toml | 2 +- src/openflexure_microscope_server/server/legacy_api.py | 3 +-- src/openflexure_microscope_server/things/system.py | 1 - 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3cef0b2d..fb072b44 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -112,7 +112,7 @@ select = [ "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") # "T20", # Warns for print statements, production code should log diff --git a/src/openflexure_microscope_server/server/legacy_api.py b/src/openflexure_microscope_server/server/legacy_api.py index e877f939..5e939116 100644 --- a/src/openflexure_microscope_server/server/legacy_api.py +++ b/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. diff --git a/src/openflexure_microscope_server/things/system.py b/src/openflexure_microscope_server/things/system.py index d2b022bb..ba70f065 100644 --- a/src/openflexure_microscope_server/things/system.py +++ b/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 From 65f172d65014c3ee45906264ad972e50f4cbb3d8 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 14:30:44 +0100 Subject: [PATCH 03/10] Stop using print statments or root level loggers --- pyproject.toml | 11 ++++++-- src/openflexure_microscope_server/logging.py | 8 +++--- .../server/__init__.py | 10 ++++--- .../things/autofocus.py | 6 +++-- .../things/camera/opencv.py | 14 +++++----- .../things/camera/picamera.py | 26 ++++++++++--------- .../camera/picamera_recalibrate_utils.py | 26 +++++++++---------- .../things/camera/simulation.py | 16 +++++++----- .../things/stage/sangaboard.py | 4 ++- 9 files changed, 69 insertions(+), 52 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index fb072b44..ef0c8af2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -114,8 +114,8 @@ select = [ "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 + "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 @@ -140,10 +140,17 @@ ignore = [ ] [tool.ruff.lint.per-file-ignores] +# All testing dirs "{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. +] +# Developer CLI scripts +"{pull_webapp,picamera_tests,change_log_helper}.py" = [ + "T20", # Printing is normal in small CLI scripts ] [tool.ruff.lint.pydocstyle] diff --git a/src/openflexure_microscope_server/logging.py b/src/openflexure_microscope_server/logging.py index 0dd3ee12..5e09b054 100644 --- a/src/openflexure_microscope_server/logging.py +++ b/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 @@ -58,12 +59,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("") - logging.info("****************************************************") - logging.info("OFM server root logger has been set up at INFO level") - logging.info("****************************************************") + LOGGER.info("OFM server root logger has been set up at INFO level") def retrieve_log() -> PlainTextResponse: diff --git a/src/openflexure_microscope_server/server/__init__.py b/src/openflexure_microscope_server/server/__init__.py index 3dfbb721..1f14fbd3 100644 --- a/src/openflexure_microscope_server/server/__init__.py +++ b/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 diff --git a/src/openflexure_microscope_server/things/autofocus.py b/src/openflexure_microscope_server/things/autofocus.py index e0b956f1..5c5e01dc 100644 --- a/src/openflexure_microscope_server/things/autofocus.py +++ b/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] diff --git a/src/openflexure_microscope_server/things/camera/opencv.py b/src/openflexure_microscope_server/things/camera/opencv.py index c159ce41..a4e5f81a 100644 --- a/src/openflexure_microscope_server/things/camera/opencv.py +++ b/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( - f"Failed to capture frame from camera {self.camera_index}" - ) + LOGGER.error(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.") - logging.warning(f"OpenCV camera doesn't respect {stream_name=}") + LOGGER.warning("OpenCV camera has no wait option. Use None.") + 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.") - logging.warning(f"OpenCV camera doesn't respect {stream_name=}") + LOGGER.warning("OpenCV camera has no wait option. Use None.") + LOGGER.warning(f"OpenCV camera doesn't respect {stream_name=}") return Image.fromarray(self.capture_array()) diff --git a/src/openflexure_microscope_server/things/camera/picamera.py b/src/openflexure_microscope_server/things/camera/picamera.py index be01b337..0158c56e 100644 --- a/src/openflexure_microscope_server/things/camera/picamera.py +++ b/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") - logging.info( + LOGGER.info("Closing picamera object for reinitialisation") + 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}") - logging.exception(e) + LOGGER.exception("Error while starting preview: {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") - logging.exception(e) + LOGGER.info("Stopping recording failed") + 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) diff --git a/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py b/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py index 5f38ee6e..b88976b6 100644 --- a/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py +++ b/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." ) @@ -230,11 +232,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( - f"Before gains, channel maxima are {np.max(channels, axis=(1, 2))}" - ) + LOGGER.info(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 +261,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 +270,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 +334,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 +346,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 +490,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) diff --git a/src/openflexure_microscope_server/things/camera/simulation.py b/src/openflexure_microscope_server/things/camera/simulation.py index 80dadb55..37df5d1c 100644 --- a/src/openflexure_microscope_server/things/camera/simulation.py +++ b/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.") - logging.warning(f"Simulation camera camera doesn't respect {stream_name=}") + LOGGER.warning("Simulation camera has no wait option. Use None.") + 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.") - logging.warning(f"Simulation camera camera doesn't respect {stream_name=}") + LOGGER.warning("Simulation camera has no wait option. Use None.") + LOGGER.warning(f"Simulation camera camera doesn't respect {stream_name=}") return Image.fromarray(self.generate_frame()) @lt.thing_action diff --git a/src/openflexure_microscope_server/things/stage/sangaboard.py b/src/openflexure_microscope_server/things/stage/sangaboard.py index 932fa373..733d4099 100644 --- a/src/openflexure_microscope_server/things/stage/sangaboard.py +++ b/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}" ) From 528c6a2b63de15c97507fd0bf46e2538f7e91a8e Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 14:58:00 +0100 Subject: [PATCH 04/10] Add PyLint ruff checks --- pyproject.toml | 12 ++++++++++-- src/openflexure_microscope_server/logging.py | 2 +- .../things/autofocus.py | 5 ++++- .../things/camera/picamera_recalibrate_utils.py | 6 +++++- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index ef0c8af2..02e5e93e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,7 +108,7 @@ 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 @@ -137,6 +137,9 @@ 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) ] [tool.ruff.lint.per-file-ignores] @@ -146,7 +149,8 @@ ignore = [ "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. + "LOG015", # Allow tests to use the root logger. + "PLC0415", # Allow tests to import later as this is needed for mocking ] # Developer CLI scripts "{pull_webapp,picamera_tests,change_log_helper}.py" = [ @@ -157,3 +161,7 @@ ignore = [ # 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 \ No newline at end of file diff --git a/src/openflexure_microscope_server/logging.py b/src/openflexure_microscope_server/logging.py index 5e09b054..d50fb72f 100644 --- a/src/openflexure_microscope_server/logging.py +++ b/src/openflexure_microscope_server/logging.py @@ -34,7 +34,7 @@ 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 + 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 diff --git a/src/openflexure_microscope_server/things/autofocus.py b/src/openflexure_microscope_server/things/autofocus.py index 5c5e01dc..88ad91eb 100644 --- a/src/openflexure_microscope_server/things/autofocus.py +++ b/src/openflexure_microscope_server/things/autofocus.py @@ -706,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. diff --git a/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py b/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py index b88976b6..ff15e02d 100644 --- a/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py +++ b/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py @@ -201,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, From 4655300910e383e73dd4d0e07b173d8577f0f433 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 14:58:37 +0100 Subject: [PATCH 05/10] Pin ruff version, allowing only bugfix updates Closes #548 --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 02e5e93e..e2a439da 100644 --- a/pyproject.toml +++ b/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 From d7d1f42b2c05175ff96c6c880631374fc38b4806 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 15:43:22 +0100 Subject: [PATCH 06/10] Enable PyTest ruff checks --- .../picamera2/test_acquisition.py | 4 +-- .../picamera2/test_calibration.py | 4 +-- .../picamera2/test_tuning.py | 3 --- pyproject.toml | 4 ++- tests/test_camera.py | 6 ++++- tests/test_dummy_server.py | 2 +- tests/test_logging.py | 12 ++++----- tests/test_serve_static_files.py | 10 +++---- tests/test_server_config.py | 4 +-- tests/test_smart_scan.py | 26 +++++++++---------- tests/test_version_strings.py | 4 ++- 11 files changed, 42 insertions(+), 37 deletions(-) diff --git a/hardware-specific-tests/picamera2/test_acquisition.py b/hardware-specific-tests/picamera2/test_acquisition.py index 3ab404eb..caf8849a 100644 --- a/hardware-specific-tests/picamera2/test_acquisition.py +++ b/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. diff --git a/hardware-specific-tests/picamera2/test_calibration.py b/hardware-specific-tests/picamera2/test_calibration.py index 7c56a9c8..6c803aed 100644 --- a/hardware-specific-tests/picamera2/test_calibration.py +++ b/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. diff --git a/hardware-specific-tests/picamera2/test_tuning.py b/hardware-specific-tests/picamera2/test_tuning.py index b193131d..d2733b8f 100644 --- a/hardware-specific-tests/picamera2/test_tuning.py +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index e2a439da..7ee7f85c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -116,7 +116,7 @@ select = [ # 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 + "PT", # pytest linting "RET", # Consistent clear return statements "RSE", # Raise parentheses # "SIM", # Simplifications detected @@ -140,6 +140,8 @@ ignore = [ "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", # Ifnore pytest.raises being used on too gereneral expectations without a + # match. We may need to revisit this. ] [tool.ruff.lint.per-file-ignores] diff --git a/tests/test_camera.py b/tests/test_camera.py index f3cfd1c7..2965b4d2 100644 --- a/tests/test_camera.py +++ b/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 statements 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())) diff --git a/tests/test_dummy_server.py b/tests/test_dummy_server.py index 66230e1b..cf745764 100644 --- a/tests/test_dummy_server.py +++ b/tests/test_dummy_server.py @@ -43,7 +43,7 @@ def thing_server(): 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 - yield server + yield server # noqa: PT022 @pytest.fixture diff --git a/tests/test_logging.py b/tests/test_logging.py index bdeec6d2..271939b5 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -161,13 +161,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, [], ["", ""]], - [FAKE_UVICORN_LOGGER.info, [""], [""]], - [FAKE_UVICORN_LOGGER.warning, [""], [""]], - [FAKE_UVICORN_LOGGER.error, [""], [""]], - [FAKE_UVICORN_LOGGER.exception, [""], [""]], + (FAKE_UVICORN_LOGGER.debug, [], ["", ""]), + (FAKE_UVICORN_LOGGER.info, [""], [""]), + (FAKE_UVICORN_LOGGER.warning, [""], [""]), + (FAKE_UVICORN_LOGGER.error, [""], [""]), + (FAKE_UVICORN_LOGGER.exception, [""], [""]), ], ) def test_uvicorn_error_only_says_error_on_error( diff --git a/tests/test_serve_static_files.py b/tests/test_serve_static_files.py index 8ec1a82c..9755cc1c 100644 --- a/tests/test_serve_static_files.py +++ b/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], - ["woff2.foo", False], - ["strange_file_ending_in_woff2", False], - ["fontfile.woff2", True], + ("foo", False), + ("woff2.foo", False), + ("strange_file_ending_in_woff2", False), + ("fontfile.woff2", True), ], ) def test_add_static_file(filename, allow_cache, mocker): diff --git a/tests/test_server_config.py b/tests/test_server_config.py index 565f02c6..3074d707 100644 --- a/tests/test_server_config.py +++ b/tests/test_server_config.py @@ -86,8 +86,8 @@ def test_customise_server(mocker): @pytest.mark.parametrize( - "config_file, expected_scan_dir", - [[FULL_CONFIG, "/var/openflexure/scans/"], [SIM_CONFIG, "./openflexure/scans/"]], + ("config_file", "expected_scan_dir"), + [(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.""" diff --git a/tests/test_smart_scan.py b/tests/test_smart_scan.py index ab619e8a..394778eb 100644 --- a/tests/test_smart_scan.py +++ b/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 - # was not expecting to receive the scan files - assert exc_info.value.status_code == 400 - assert len(caplog.records) == 1 - assert caplog.records[0].levelname == "WARNING" - assert caplog.records[0].name == "mock-invocation_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 + assert caplog.records[0].levelname == "WARNING" + assert caplog.records[0].name == "mock-invocation_logger" - # Make a dir for the fake scan and delete it. - os.makedirs(fake_scan_path) - assert os.path.exists(fake_scan_path) - smart_scan_thing.delete_scan(fake_scan_name, LOGGER) - assert not os.path.exists(fake_scan_path) - # Check no extra logs generated - assert len(caplog.records) == 1 + # Make a dir for the fake scan and delete it. + os.makedirs(fake_scan_path) + assert os.path.exists(fake_scan_path) + smart_scan_thing.delete_scan(fake_scan_name, LOGGER) + assert not os.path.exists(fake_scan_path) + # Check no extra logs generated + assert len(caplog.records) == 1 def test_delete_all_scans(smart_scan_thing, caplog): diff --git a/tests/test_version_strings.py b/tests/test_version_strings.py index ea700618..e1719c33 100644 --- a/tests/test_version_strings.py +++ b/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 temdir 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): From 6b1e40f6893b71247be2fc9b9aee8d61b8fc6697 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 15:56:10 +0100 Subject: [PATCH 07/10] Add flake8-simplicity rules --- pyproject.toml | 4 +- .../scan_directories.py | 13 +++--- .../scan_planners.py | 10 +---- .../stitching.py | 4 +- .../things/smart_scan.py | 5 +-- tests/test_logging.py | 43 +++++++++---------- tests/test_scan_planners.py | 8 ++-- 7 files changed, 38 insertions(+), 49 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 7ee7f85c..0e63127d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -119,7 +119,7 @@ select = [ "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 @@ -142,6 +142,8 @@ ignore = [ # magic (such as 255 when doing uint8 maths) "PT011", # Ifnore pytest.raises being used on too gereneral 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] diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py index 8256f414..e3449588 100644 --- a/src/openflexure_microscope_server/scan_directories.py +++ b/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 not os.path.exists(file_path): - return None + if check_exists and not os.path.exists(file_path): + 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 not os.path.exists(file_path): - return None + if check_exists and not os.path.exists(file_path): + 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: diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py index 4ef066bf..7387e70d 100644 --- a/src/openflexure_microscope_server/scan_planners.py +++ b/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 - else: - z = closest_pos[2] + z = None if closest_pos is None else 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 - else: - z = closest_pos[2] + z = None if closest_pos is None else closest_pos[2] return next_location, z diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index c81bd34d..09d72753 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/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 True - return False + return self._popen_obj.poll() is None def wait(self, cancel: lt.deps.CancelHook) -> None: """Wait for this preview stitch to return. diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py index ae723880..1b760e13 100644 --- a/src/openflexure_microscope_server/things/smart_scan.py +++ b/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 not self._preview_stitcher.running: - self._preview_stitcher.start() + if self._scan_data.image_count > 3 and not self._preview_stitcher.running: + self._preview_stitcher.start() @_scan_running def _run_scan(self) -> None: diff --git a/tests/test_logging.py b/tests/test_logging.py index 271939b5..85d59888 100644 --- a/tests/test_logging.py +++ b/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 tempfile.TemporaryDirectory() as tmpdir: - ofm_logging.configure_logging(tmpdir) - assert len(caplog.records) == 0 - with open(ofm_logging.OFM_LOG_FILE, "r", encoding="utf-8") as log_file: - log_txt = log_file.read() - assert "OFM server root logger has been set up at INFO level" in log_txt - root_logger = logging.getLogger() - assert ofm_logging.OFM_HANDLER in root_logger.handlers + with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir: + ofm_logging.configure_logging(tmpdir) + assert len(caplog.records) == 0 + with open(ofm_logging.OFM_LOG_FILE, "r", encoding="utf-8") as log_file: + log_txt = log_file.read() + assert "OFM server root logger has been set up at INFO level" in log_txt + root_logger = logging.getLogger() + 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 tempfile.TemporaryDirectory() as tmpdir: - ofm_logging.configure_logging(tmpdir) - assert len(caplog.records) == 1 - # Check OFM logger is added even if the file logger couldn't be. - root_logger = logging.getLogger() - assert ofm_logging.OFM_HANDLER in root_logger.handlers + with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir: + ofm_logging.configure_logging(tmpdir) + assert len(caplog.records) == 1 + # Check OFM logger is added even if the file logger couldn't be. + root_logger = logging.getLogger() + 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 tempfile.TemporaryDirectory() as tmpdir: - log_dir = os.path.join(tmpdir, "new_dir") - assert not os.path.isdir(log_dir) - ofm_logging.configure_logging(log_dir) - assert len(caplog.records) == 0 - assert os.path.isdir(log_dir) + with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir: + log_dir = os.path.join(tmpdir, "new_dir") + assert not os.path.isdir(log_dir) + ofm_logging.configure_logging(log_dir) + assert len(caplog.records) == 0 + assert os.path.isdir(log_dir) def test_max_logs(): diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py index 0b1adca2..89996945 100644 --- a/tests/test_scan_planners.py +++ b/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 (1, 6) == scan_planners.enforce_xy_tuple([1, 6]) + assert scan_planners.enforce_xy_tuple((1, 6)) == (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 (1, 6, 6) == scan_planners.enforce_xyz_tuple([1, 6, 6]) + assert scan_planners.enforce_xyz_tuple((1, 6, 2)) == (1, 6, 2) + assert scan_planners.enforce_xyz_tuple([1, 6, 6]) == (1, 6, 6) def test_base_class_not_implemented(): From 3227c0cddf520737e3013794da306244ea70986a Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 15:59:14 +0100 Subject: [PATCH 08/10] Remove ruff-increased-checking.toml as all checker are now on! --- .gitlab-ci.yml | 12 ----------- CONTRIBUTING.md | 2 +- pyproject.toml | 4 ---- ruff-increased-checking.toml | 41 ------------------------------------ 4 files changed, 1 insertion(+), 58 deletions(-) delete mode 100644 ruff-increased-checking.toml diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e0680fe5..a476aa23 100644 --- a/.gitlab-ci.yml +++ b/.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: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b171d161..a108e8c9 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/pyproject.toml b/pyproject.toml index 0e63127d..aae45c65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -100,10 +100,6 @@ 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 diff --git a/ruff-increased-checking.toml b/ruff-increased-checking.toml deleted file mode 100644 index fd63f8fe..00000000 --- a/ruff-increased-checking.toml +++ /dev/null @@ -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 From b4e28e232e9e05cb0c7514127e74753792749d50 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 18 Sep 2025 16:31:39 +0100 Subject: [PATCH 09/10] Rerun picamera tests --- picamera_coverage.zip | Bin 54038 -> 54038 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/picamera_coverage.zip b/picamera_coverage.zip index d20a51c0def86625914bd1a2b3f2e3a8ee75de8e..95750fa82ad8244c6dc290c616aae4653d56c6ff 100644 GIT binary patch delta 546 zcmbQXjCtBJW}yIYW)=|!5ZKvl6rH&Ey7NXMl?I_a2L7M?ANi;8H}gmHo#kue%iHWI z5XLuoejiW$?`h&b7i<{RI2sz5T-jL}IUBjynLsA|*q(EH+t26noy-gjJa0Zc=HzE! zYx=>s`^Wpa1z+lB6_ha@VEG{MW7(Hz&Li=MF2@V1H8f9aP*37#Z{%iUW#nuUV`F0Y z_kMdq=^34d(~}>bKfqJK##m!<_~ovBOfg%QyZ^2>`SCmH%l^x8&88a?dkh=?i!=PX z_nMFOt`tKZ_a0`31r7iIUKgJuTX(B1o|%E+Pck#Zl?x0BHy94onKAsaH&D34aNq?S z(*=e+h9A-n<{(E)Fi0>k9AHpeWXmkTz`!8Ez?0yxxx0V80|zKT_9fQKPIkI*QQg4M z)FRo`EY-lmEY%<_HQC72DAm{`#mLai(%8h@Br(Or(jd{yZ1VXFwv)>*N@}E~8JeaV z7^PSwn-~}+8>J?uSejcVrz9Jjnk1(fTBcZ}rKXx0TNq8gaKUEsmy441X%=SYrpC$U zCdS5TW@(mYi7BQ=iD{`RX=bVB21$lSNrpxii3XOIN;(Qkddc}^sYQwDsd@#O$%(nC zMTtsQ3QDQTiN==6#)&4$smA7pX36G8rX~hy7RG4?$tETisb&_&1_p^qrpZdR0p5&E VBFre!&geY3_mTy|&u1@r0s!*)z9awu delta 546 zcmbQXjCtBJW}yIYW)=|!5ICD*6n!pSHFTqpN`p{71OHF{kNng4oB5;p&hoYMW@BaKY!YK*V)*xd zdxESn_k&L1_9EoI}2me_c_I=;w z?6{q+;Xc!Q1_q4>_5Z)Jrnk@E_V79b1H=Ct3=F0w3=(Dx8|L#e{O9N4FlR8>)yQDd zAkFZP?E){z&1?*83=9Shof+pDSQr=>*cg~41U7f~uXo@8g@@ep^p%sHE?m?|N;NP{ zGB!3hu(U8WOfohyOS3RGPD-*cv@kWdu&^*lOENStFikO?eBpx48m4K+7OAOb zsfo!J7DguKsY%I3#sDD+ z7?_$Fo0yoH85kQGn;9pYo24ZtB_<{tnI Date: Fri, 19 Sep 2025 10:16:47 +0000 Subject: [PATCH 10/10] Apply suggestions from code review of branch Increased-linting Co-authored-by: Beth Probert --- pyproject.toml | 2 +- src/openflexure_microscope_server/logging.py | 1 + tests/test_camera.py | 2 +- tests/test_dummy_server.py | 2 +- tests/test_version_strings.py | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index aae45c65..dffc3831 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -136,7 +136,7 @@ ignore = [ "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", # Ifnore pytest.raises being used on too gereneral expectations without a + "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` diff --git a/src/openflexure_microscope_server/logging.py b/src/openflexure_microscope_server/logging.py index d50fb72f..ff4f8de7 100644 --- a/src/openflexure_microscope_server/logging.py +++ b/src/openflexure_microscope_server/logging.py @@ -34,6 +34,7 @@ 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 + # This requires silencing PLW0603 which disallows globals. global OFM_LOG_FILE # noqa: PLW0603 OFM_LOG_FILE = os.path.join(log_folder, "openflexure_microscope.log") diff --git a/tests/test_camera.py b/tests/test_camera.py index 2965b4d2..39a35d89 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -55,7 +55,7 @@ def test_handle_broken_frame(): # Check that this does cause broken frames. # The noqa is because we don't know exactly when the error is thrown so we - # can't have a single simple statements in the pytest raises. + # 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" ): diff --git a/tests/test_dummy_server.py b/tests/test_dummy_server.py index cf745764..3f911992 100644 --- a/tests/test_dummy_server.py +++ b/tests/test_dummy_server.py @@ -42,7 +42,7 @@ 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 # noqa: PT022 diff --git a/tests/test_version_strings.py b/tests/test_version_strings.py index e1719c33..b1a9fa50 100644 --- a/tests/test_version_strings.py +++ b/tests/test_version_strings.py @@ -82,7 +82,7 @@ def git_repo(temp_dir): _git("add -A") _git("commit -m 'message2'") - # Yielding here as temdir is yielding and we want to stay in the tempdir + # 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