(#378) Use diff-quality rather than pyright, make pyright, ruff happy

.github/workflows/ci.yml

@@ -13,7 +13,7 @@ jobs:
     if: needs.check.outputs.branch-pr == ''
     uses: ./.github/workflows/_tox.yml
     with:
-      tox: pre-commit,type-checking
+      tox: pre-commit,diff-quality
 
   test:
     needs: check

pyproject.toml

@@ -130,7 +130,7 @@ legacy_tox_ini = """
 [tox]
 skipsdist=True
 
-[testenv:{pre-commit,type-checking,tests,docs}]
+[testenv:{pre-commit,type-checking,tests,docs,diff-quality}]
 # Don't create a virtualenv for the command, requires tox-direct plugin
 direct = True
 passenv = *
@@ -145,6 +145,7 @@ commands =
     type-checking: pyright src tests {posargs}
     pre-commit: pre-commit run --all-files --show-diff-on-failure {posargs}
     docs: sphinx-{posargs:build -E} -T docs build/html
+    diff-quality: diff-quality --violations=pyright --fail-under=100
 """
 
 [tool.ruff]

src/dodal/devices/eiger.py

@@ -53,17 +53,15 @@ def with_params(
         cls,
         params: DetectorParams,
         name: str = "EigerDetector",
-        *args,
-        **kwargs,
     ):
-        det = cls(name=name, *args, **kwargs)
+        det = cls(name=name)
         det.set_detector_parameters(params)
         return det
 
     def set_detector_parameters(self, detector_params: DetectorParams):
         self.detector_params = detector_params
         if self.detector_params is None:
-            raise Exception("Parameters for scan must be specified")
+            raise ValueError("Parameters for scan must be specified")
 
         to_check = [
             (

src/dodal/devices/eiger_odin.py

@@ -1,5 +1,3 @@
-from typing import List, Tuple
-
 from ophyd import Component, Device, EpicsSignal, EpicsSignalRO, EpicsSignalWithRBV
 from ophyd.areadetector.plugins import HDF5Plugin_V22
 from ophyd.sim import NullStatus
@@ -59,12 +57,12 @@ class OdinNodesStatus(Device):
     node_3 = Component(OdinNode, "OD4:")
 
     @property
-    def nodes(self) -> List[OdinNode]:
+    def nodes(self) -> list[OdinNode]:
         return [self.node_0, self.node_1, self.node_2, self.node_3]
 
     def check_node_frames_from_attr(
         self, node_get_func, error_message_verb: str
-    ) -> Tuple[bool, str]:
+    ) -> tuple[bool, str]:
         nodes_frames_values = [0] * len(self.nodes)
         frames_details = []
         for node_number, node_pv in enumerate(self.nodes):
@@ -75,17 +73,17 @@ def check_node_frames_from_attr(
         bad_frames = any(v != 0 for v in nodes_frames_values)
         return bad_frames, "\n".join(frames_details)
 
-    def check_frames_timed_out(self) -> Tuple[bool, str]:
+    def check_frames_timed_out(self) -> tuple[bool, str]:
         return self.check_node_frames_from_attr(
             lambda node: node.frames_timed_out.get(), "timed out"
         )
 
-    def check_frames_dropped(self) -> Tuple[bool, str]:
+    def check_frames_dropped(self) -> tuple[bool, str]:
         return self.check_node_frames_from_attr(
             lambda node: node.frames_dropped.get(), "dropped"
         )
 
-    def get_error_state(self) -> Tuple[bool, str]:
+    def get_error_state(self) -> tuple[bool, str]:
         is_error = []
         error_messages = []
         for node_number, node_pv in enumerate(self.nodes):
@@ -99,7 +97,7 @@ def get_error_state(self) -> Tuple[bool, str]:
 
     def get_init_state(self) -> bool:
         is_initialised = []
-        for node_number, node_pv in enumerate(self.nodes):
+        for node_pv in self.nodes:
             is_initialised.append(node_pv.fr_initialised.get())
             is_initialised.append(node_pv.fp_initialised.get())
         return all(is_initialised)
@@ -132,15 +130,15 @@ def check_odin_state(self) -> bool:
         frames_timed_out, frames_timed_out_details = self.nodes.check_frames_timed_out()
 
         if not is_initialised:
-            raise Exception(error_message)
+            raise RuntimeError(error_message)
         if frames_dropped:
             self.log.error(f"Frames dropped: {frames_dropped_details}")
         if frames_timed_out:
             self.log.error(f"Frames timed out: {frames_timed_out_details}")
 
         return is_initialised and not frames_dropped and not frames_timed_out
 
-    def check_odin_initialised(self) -> Tuple[bool, str]:
+    def check_odin_initialised(self) -> tuple[bool, str]:
         is_error_state, error_messages = self.nodes.get_error_state()
         to_check = [
             (not self.fan.consumers_connected.get(), "EigerFan is not connected"),

tests/devices/unit_tests/test_eiger.py

@@ -163,7 +163,7 @@ def test_check_detector_variables(
         try:
             fake_eiger.set_detector_parameters(detector_params)
         except Exception as e:
-            assert False, f"exception was raised {e}"
+            raise AssertionError(f"exception was raised {e}") from e
 
 
 # Tests transition from set_odin_pvs_after_file_writer_set to set_mx_settings_pvs
@@ -218,11 +218,12 @@ def test_stage_raises_exception_if_odin_initialisation_status_not_ok(fake_eiger)
 
 
 @pytest.mark.parametrize(
-    "roi_mode, expected_num_change_roi_calls", [(True, 1), (False, 0)]
+    "roi_mode, expected_num_change_roi_calls, expected_exception",
+    [(True, 1, "Test Exception 2"), (False, 0, "Test Exception 1")],
 )
 @patch("dodal.devices.eiger.await_value")
 def test_stage_enables_roi_mode_correctly(
-    mock_await, fake_eiger, roi_mode, expected_num_change_roi_calls
+    mock_await, fake_eiger, roi_mode, expected_num_change_roi_calls, expected_exception
 ):
     when(fake_eiger.odin.nodes).clear_odin_errors().thenReturn(None)
     when(fake_eiger.odin).check_odin_initialised().thenReturn((True, ""))
@@ -237,9 +238,10 @@ def test_stage_enables_roi_mode_correctly(
     assert fake_eiger.change_roi_mode.call_count == expected_num_change_roi_calls
 
     # Tidy up async staging
-    change_roi_mode_status.set_exception(Exception)
-    with pytest.raises(Exception):
+    change_roi_mode_status.set_exception(UnknownStatusFailure("Test Exception 2"))
+    with pytest.raises(UnknownStatusFailure) as e:
         returned_status.wait(0.1)
+        assert e.args[0] == expected_exception
 
 
 def test_disable_roi_mode_sets_correct_roi_mode(fake_eiger):
@@ -544,7 +546,7 @@ def test_given_in_free_run_mode_and_not_all_frames_collected_in_time_when_unstag
 
     fake_eiger.detector_params.trigger_mode = TriggerMode.FREE_RUN
     fake_eiger.ALL_FRAMES_TIMEOUT = 0.1
-    with pytest.raises(Exception):
+    with pytest.raises(TimeoutError):
         fake_eiger.unstage()
 
     assert fake_eiger.odin.meta.stop_writing.get() == 1
@@ -605,7 +607,7 @@ def test_given_detector_arming_status_failed_when_unstage_then_detector_still_di
     fake_eiger.cam.acquire.sim_put(1)  # type: ignore
 
     fake_eiger.arming_status = get_bad_status()
-    with pytest.raises(Exception):
+    with pytest.raises(RuntimeError):
         fake_eiger.unstage()
 
     assert fake_eiger.cam.acquire.get() == 0
@@ -639,7 +641,7 @@ def test_unwrapped_arm_chain_functions_are_not_called_outside_util(
     fake_eiger: EigerDetector,
 ):
     fake_eiger.odin.stop = MagicMock(return_value=NullStatus())
-    fake_eiger.detector_params.use_roi_mode = True
+    fake_eiger.detector_params.use_roi_mode = True  # type: ignore
     done_status = NullStatus()
 
     call_func.return_value = done_status

tests/devices/unit_tests/test_focusing_mirror.py

@@ -68,10 +68,8 @@ def not_ok(_):
 def test_mirror_set_voltage_sets_and_waits_happy_path(
     vfm_mirror_voltages_with_set: VFMMirrorVoltages,
 ):
-    vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.set.return_value = (
-        NullStatus()
-    )
-    vfm_mirror_voltages_with_set._channel14_voltage_device._demand_accepted.sim_put(
+    vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.set.return_value = NullStatus()
+    vfm_mirror_voltages_with_set._channel14_voltage_device._demand_accepted.sim_put(  # type: ignore
         MirrorVoltageDemand.OK
     )
 
@@ -98,7 +96,7 @@ def test_mirror_set_voltage_sets_and_waits_set_fail(
     )
 
     status: StatusBase = vfm_mirror_voltages_with_set.voltage_channels[0].set(100)
-    with pytest.raises(Exception):
+    with pytest.raises(UnknownStatusFailure):
         status.wait()
 
     assert not status.success
@@ -108,9 +106,7 @@ def test_mirror_set_voltage_sets_and_waits_set_fail(
 def test_mirror_set_voltage_sets_and_waits_settle_timeout_expires(
     vfm_mirror_voltages_with_set_timing_out: VFMMirrorVoltages,
 ):
-    vfm_mirror_voltages_with_set_timing_out._channel14_voltage_device._setpoint_v.set.return_value = (
-        NullStatus()
-    )
+    vfm_mirror_voltages_with_set_timing_out._channel14_voltage_device._setpoint_v.set.return_value = NullStatus()
 
     status: StatusBase = vfm_mirror_voltages_with_set_timing_out.voltage_channels[
         0

tests/unit_tests/test_log.py

@@ -1,5 +1,6 @@
 import logging
 from pathlib import Path, PosixPath
+from typing import cast
 from unittest.mock import MagicMock, call, patch
 
 import pytest
@@ -13,6 +14,7 @@
     LOGGER,
     BeamlineFilter,
     CircularMemoryHandler,
+    DodalLogHandlers,
     clear_all_loggers_and_handlers,
     do_default_logging_setup,
     get_logging_file_path,
@@ -74,8 +76,7 @@ def test_dev_mode_sets_correct_graypy_handler(
         mock_logger, Path("tmp/dev"), "dodal.log", True, 10000
     )
     mock_GELFTCPHandler.assert_called_once_with("localhost", 5555)
-    for handler in handler_config.values():
-        handler.close()
+    _close_all_handlers(handler_config)
 
 
 @patch("dodal.log.GELFTCPHandler", autospec=True)
@@ -90,8 +91,7 @@ def test_prod_mode_sets_correct_graypy_handler(
     mock_GELFTCPHandler.assert_called_once_with(
         "graylog-log-target.diamond.ac.uk", 12231
     )
-    for handler in handler_config.values():
-        handler.close()
+    _close_all_handlers(handler_config)
 
 
 @patch("dodal.log.GELFTCPHandler", autospec=True)
@@ -118,8 +118,7 @@ def test_no_env_variable_sets_correct_file_handler(
     ]
 
     mock_file_handler.assert_has_calls(expected_calls, any_order=True)
-    for handler in handler_config.values():
-        handler.close()
+    _close_all_handlers(handler_config)
 
 
 def test_beamline_filter_adds_dev_if_no_beamline():
@@ -260,3 +259,8 @@ def __init__(self, name: str = test_device_name) -> None:
     assert f"[{test_signal_name}]" in stream_handler.stream.write.call_args.args[0]
     device.log.debug("test message")
     assert f"[{test_device_name}]" in stream_handler.stream.write.call_args.args[0]
+
+
+def _close_all_handlers(handler_config: DodalLogHandlers):
+    for handler in handler_config.values():
+        cast(logging.Handler, handler).close()