eprecate source as positional arg

Author: jenshnielsen

src/qcodes/parameters/delegate_parameter.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 import warnings
-from typing import TYPE_CHECKING, Any, Generic
+from typing import TYPE_CHECKING, Any, ClassVar, Generic
 
 from typing_extensions import TypeVar
 
@@ -31,6 +31,7 @@
     default="InstrumentBase | None",
     covariant=True,
 )
+_SOURCE_UNSET: Any = object()
 
 
 class DelegateParameter(
@@ -74,6 +75,11 @@ class DelegateParameter(
 
     """
 
+    _DEPRECATED_POSITIONAL_ARGS: ClassVar[tuple[str, ...]] = (
+        "source",
+        *Parameter._DEPRECATED_POSITIONAL_ARGS,
+    )
+
     class _DelegateCache(
         Generic[_local_ParameterDataTypeVar, _local_InstrumentTypeVar_co]
     ):
@@ -178,20 +184,58 @@ def __call__(self) -> _local_ParameterDataTypeVar:
     def __init__(
         self,
         name: str,
-        source: Parameter | None,
         *args: Any,
+        source: Parameter | None = _SOURCE_UNSET,
         **kwargs: Any,
     ):
         if args:
-            # TODO: After QCoDeS 0.57 remove the args argument
-            # and delete this code block.
+            # TODO: After QCoDeS 0.57 remove the args argument and delete this code block.
+            positional_names = DelegateParameter._DEPRECATED_POSITIONAL_ARGS
+            if len(args) > len(positional_names):
+                raise TypeError(
+                    f"{type(self).__name__}.__init__() takes at most "
+                    f"{len(positional_names) + 2} positional arguments "
+                    f"({len(args) + 2} given)"
+                )
+
+            for i in range(len(args)):
+                arg_name = positional_names[i]
+                if arg_name == "source":
+                    if source is not _SOURCE_UNSET:
+                        raise TypeError(
+                            f"{type(self).__name__}.__init__() got multiple "
+                            f"values for argument '{arg_name}'"
+                        )
+                elif arg_name in kwargs:
+                    raise TypeError(
+                        f"{type(self).__name__}.__init__() got multiple "
+                        f"values for argument '{arg_name}'"
+                    )
+
+            positional_arg_names = positional_names[: len(args)]
+            names_str = ", ".join(f"'{n}'" for n in positional_arg_names)
             warnings.warn(
-                "Passing extra positional arguments to "
+                f"Passing {names_str} as positional argument(s) to "
                 f"{type(self).__name__} is deprecated. "
                 "Please pass them as keyword arguments.",
                 QCoDeSDeprecationWarning,
                 stacklevel=2,
             )
+
+            positional_values = dict(zip(positional_names, args))
+            if "source" in positional_values:
+                source = positional_values["source"]
+
+            for arg_name in positional_names[1:]:
+                if arg_name in positional_values:
+                    kwargs[arg_name] = positional_values[arg_name]
+
+        if source is _SOURCE_UNSET:
+            raise TypeError(
+                f"{type(self).__name__}.__init__() missing required keyword "
+                "argument: 'source'"
+            )
+
         if "bind_to_instrument" not in kwargs.keys():
             kwargs["bind_to_instrument"] = False
 
@@ -213,7 +257,7 @@ def __init__(
 
         initial_cache_value = kwargs.pop("initial_cache_value", None)
         self.source = source
-        super().__init__(name, *args, **kwargs)
+        super().__init__(name, **kwargs)
         self.label = kwargs.get("label", None)
         self.unit = kwargs.get("unit", None)
 

tests/parameter/test_args_deprecated.py

@@ -127,18 +127,49 @@ def test_too_many_positional_args_raises(self) -> None:
 
 
 class TestDelegateParameterPositionalArgs:
-    """DelegateParameter should warn when extra positional args are passed."""
+    """DelegateParameter should warn when arguments after ``name`` are positional."""
 
-    def test_positional_args_warn(self) -> None:
+    def test_single_positional_arg_warns(self) -> None:
+        source = Parameter("source", set_cmd=None)
+        with pytest.warns(
+            QCoDeSDeprecationWarning,
+            match="Passing 'source' as positional argument",
+        ):
+            DelegateParameter("test", source)
+
+    def test_multiple_positional_args_warn(self) -> None:
         source = Parameter("source", set_cmd=None)
-        with pytest.warns(QCoDeSDeprecationWarning):
+        with pytest.warns(
+            QCoDeSDeprecationWarning,
+            match="'source', 'instrument'",
+        ):
             DelegateParameter("test", source, None)
 
     def test_keyword_args_do_not_warn(self) -> None:
         source = Parameter("source", set_cmd=None)
-        p = DelegateParameter("test", source, instrument=None)
+        p = DelegateParameter("test", source=source, instrument=None)
         assert p.name == "test"
 
+    def test_duplicate_positional_and_keyword_raises(self) -> None:
+        source = Parameter("source", set_cmd=None)
+        with pytest.raises(
+            TypeError,
+            match="got multiple values for argument 'source'",
+        ):
+            DelegateParameter("test", source, source=source)
+
+    def test_too_many_positional_args_raises(self) -> None:
+        too_many = (None,) * 20
+        with pytest.raises(TypeError, match="takes at most"):
+            DelegateParameter("test", *too_many)
+
+    def test_missing_source_raises(self) -> None:
+        with pytest.raises(
+            TypeError,
+            match="missing required keyword argument: 'source'",
+        ):
+            DelegateParameter("test")
+
 
 # Minimal concrete subclass of ArrayParameter for testing
 class _ConcreteArrayParameter(ArrayParameter):

tests/parameter/test_delegate_parameter.py

@@ -104,7 +104,7 @@ def test_observable_parameter_initial_value(
 
 
 def test_same_value(simple_param: Parameter) -> None:
-    d = DelegateParameter("test_delegate_parameter", simple_param)
+    d = DelegateParameter("test_delegate_parameter", source=simple_param)
     assert d() == simple_param()
 
 
@@ -113,21 +113,21 @@ def test_same_label_and_unit_on_init(simple_param: Parameter) -> None:
     Test that the label and unit get used from source parameter if not
     specified otherwise.
     """
-    d = DelegateParameter("test_delegate_parameter", simple_param)
+    d = DelegateParameter("test_delegate_parameter", source=simple_param)
     assert d.label == simple_param.label
     assert d.unit == simple_param.unit
 
 
 def test_overwritten_unit_on_init(simple_param: Parameter) -> None:
-    d = DelegateParameter("test_delegate_parameter", simple_param, unit="Ohm")
+    d = DelegateParameter("test_delegate_parameter", source=simple_param, unit="Ohm")
     assert d.label == simple_param.label
     assert not d.unit == simple_param.unit
     assert d.unit == "Ohm"
 
 
 def test_overwritten_label_on_init(simple_param: Parameter) -> None:
     d = DelegateParameter(
-        "test_delegate_parameter", simple_param, label="Physical parameter"
+        "test_delegate_parameter", source=simple_param, label="Physical parameter"
     )
     assert d.unit == simple_param.unit
     assert not d.label == simple_param.label
@@ -140,15 +140,15 @@ def test_get_set_raises(simple_param: Parameter) -> None:
     """
     for kwargs in ({"set_cmd": None}, {"get_cmd": None}):
         with pytest.raises(KeyError) as e:
-            DelegateParameter("test_delegate_parameter", simple_param, **kwargs)
+            DelegateParameter("test_delegate_parameter", source=simple_param, **kwargs)
         assert str(e.value).startswith("'It is not allowed to set")
 
 
 def test_scaling(simple_param: Parameter, numeric_val: int) -> None:
     scale = 5
     offset = 3
     d = DelegateParameter(
-        "test_delegate_parameter", simple_param, offset=offset, scale=scale
+        "test_delegate_parameter", source=simple_param, offset=offset, scale=scale
     )
 
     simple_param(numeric_val)
@@ -165,7 +165,7 @@ def test_scaling_delegate_initial_value(
     offset = 3
     DelegateParameter(
         "test_delegate_parameter",
-        simple_param,
+        source=simple_param,
         offset=offset,
         scale=scale,
         initial_value=numeric_val,
@@ -178,7 +178,7 @@ def test_scaling_initial_value(simple_param: Parameter) -> None:
     scale = 5
     offset = 3
     d = DelegateParameter(
-        "test_delegate_parameter", simple_param, offset=offset, scale=scale
+        "test_delegate_parameter", source=simple_param, offset=offset, scale=scale
     )
     assert d() == (simple_param() - offset) / scale
 
@@ -188,7 +188,7 @@ def test_snapshot() -> None:
         "testparam", set_cmd=None, get_cmd=None, offset=1, scale=2, initial_value=1
     )
     d = DelegateParameter(
-        "test_delegate_parameter", p, offset=3, scale=5, initial_value=2
+        "test_delegate_parameter", source=p, offset=3, scale=5, initial_value=2
     )
 
     delegate_snapshot = d.snapshot()
@@ -205,7 +205,7 @@ def test_set_source_cache_changes_delegate_cache(simple_param: Parameter) -> Non
     """
     offset = 4
     scale = 5
-    d = DelegateParameter("d", simple_param, offset=offset, scale=scale)
+    d = DelegateParameter("d", source=simple_param, offset=offset, scale=scale)
     new_source_value = 3
     simple_param.cache.set(new_source_value)
 
@@ -219,7 +219,7 @@ def test_set_source_cache_changes_delegate_get(simple_param: Parameter) -> None:
     """
     offset = 4
     scale = 5
-    d = DelegateParameter("d", simple_param, offset=offset, scale=scale)
+    d = DelegateParameter("d", source=simple_param, offset=offset, scale=scale)
     new_source_value = 3
 
     simple_param.cache.set(new_source_value)
@@ -230,7 +230,7 @@ def test_set_source_cache_changes_delegate_get(simple_param: Parameter) -> None:
 def test_set_delegate_cache_changes_source_cache(simple_param: Parameter) -> None:
     offset = 4
     scale = 5
-    d = DelegateParameter("d", simple_param, offset=offset, scale=scale)
+    d = DelegateParameter("d", source=simple_param, offset=offset, scale=scale)
 
     new_delegate_value = 2
     d.cache.set(new_delegate_value)
@@ -241,7 +241,7 @@ def test_set_delegate_cache_changes_source_cache(simple_param: Parameter) -> Non
 def test_set_delegate_cache_with_raw_value(simple_param: Parameter) -> None:
     offset = 4
     scale = 5
-    d = DelegateParameter("d", simple_param, offset=offset, scale=scale)
+    d = DelegateParameter("d", source=simple_param, offset=offset, scale=scale)
 
     new_delegate_value = 2
     d.cache._set_from_raw_value(new_delegate_value * scale + offset)
@@ -266,7 +266,7 @@ def test_instrument_val_invariant_under_delegate_cache_set(
 
 def test_delegate_cache_pristine_if_not_set() -> None:
     p = Parameter("test")
-    d = DelegateParameter("delegate", p)
+    d = DelegateParameter("delegate", source=p)
     gotten_delegate_cache = d.cache.get(get_if_invalid=False)
     assert gotten_delegate_cache is None
 
@@ -293,7 +293,7 @@ def test_delegate_get_updates_cache(
 ) -> None:
     initial_value = numeric_val
     t = make_observable_parameter("observable_parameter", initial_value=initial_value)
-    d = DelegateParameter("delegate", t)
+    d = DelegateParameter("delegate", source=t)
 
     assert d() == initial_value
     assert d.cache.get() == initial_value
@@ -333,7 +333,7 @@ def test_raw_value_scaling() -> None:
     """
 
     p = Parameter("testparam", set_cmd=None, get_cmd=None, offset=1, scale=2)
-    d = DelegateParameter("test_delegate_parameter", p, offset=3, scale=5)
+    d = DelegateParameter("test_delegate_parameter", source=p, offset=3, scale=5)
 
     val = 1
     p(val)
@@ -347,15 +347,17 @@ def test_raw_value_scaling() -> None:
 def test_setting_initial_value_delegate_parameter() -> None:
     value = 10
     p = Parameter("testparam", set_cmd=None, get_cmd=None)
-    d = DelegateParameter("test_delegate_parameter", p, initial_value=value)
+    d = DelegateParameter("test_delegate_parameter", source=p, initial_value=value)
     assert p.cache.get(get_if_invalid=False) == value
     assert d.cache.get(get_if_invalid=False) == value
 
 
 def test_setting_initial_cache_delegate_parameter() -> None:
     value = 10
     p = Parameter("testparam", set_cmd=None, get_cmd=None)
-    d = DelegateParameter("test_delegate_parameter", p, initial_cache_value=value)
+    d = DelegateParameter(
+        "test_delegate_parameter", source=p, initial_cache_value=value
+    )
     assert p.cache.get(get_if_invalid=False) == value
     assert d.cache.get(get_if_invalid=False) == value
 
@@ -524,7 +526,9 @@ def test_delegate_parameter_fixed_label_unit_unchanged() -> None:
 def test_cache_invalidation() -> None:
     value = 10
     p = BetterGettableParam("testparam", set_cmd=None, get_cmd=None)
-    d = DelegateParameter("test_delegate_parameter", p, initial_cache_value=value)
+    d = DelegateParameter(
+        "test_delegate_parameter", source=p, initial_cache_value=value
+    )
     assert p._get_count == 0
     assert d.cache.get() == value
     assert p._get_count == 0
@@ -562,7 +566,7 @@ def test_cache_no_source() -> None:
 
 def test_underlying_instrument_property_for_delegate_parameter() -> None:
     p = BetterGettableParam("testparam", set_cmd=None, get_cmd=None)
-    d = DelegateParameter("delegate_parameter_with_source", p)
+    d = DelegateParameter("delegate_parameter_with_source", source=p)
 
     assert d.underlying_instrument is p.root_instrument