diff --git a/src/qcodes/parameters/delegate_parameter.py b/src/qcodes/parameters/delegate_parameter.py index 285f84c5c79..f0a42ba8739 100644 --- a/src/qcodes/parameters/delegate_parameter.py +++ b/src/qcodes/parameters/delegate_parameter.py @@ -1,9 +1,12 @@ from __future__ import annotations +import warnings from typing import TYPE_CHECKING, Any, Generic from typing_extensions import TypeVar +from qcodes.utils import QCoDeSDeprecationWarning + from .parameter import Parameter from .parameter_base import InstrumentTypeVar_co, ParameterDataTypeVar @@ -179,6 +182,16 @@ def __init__( *args: Any, **kwargs: Any, ): + if args: + # TODO: After QCoDeS 0.57 remove the args argument + # and delete this code block. + warnings.warn( + "Passing extra positional arguments to " + f"{type(self).__name__} is deprecated. " + "Please pass them as keyword arguments.", + QCoDeSDeprecationWarning, + stacklevel=2, + ) if "bind_to_instrument" not in kwargs.keys(): kwargs["bind_to_instrument"] = False diff --git a/src/qcodes/parameters/parameter.py b/src/qcodes/parameters/parameter.py index ab3ba8c2a28..049597a25ab 100644 --- a/src/qcodes/parameters/parameter.py +++ b/src/qcodes/parameters/parameter.py @@ -5,8 +5,11 @@ import logging import os +import warnings from types import MethodType -from typing import TYPE_CHECKING, Any, Generic, Literal +from typing import TYPE_CHECKING, Any, ClassVar, Generic, Literal + +from qcodes.utils import QCoDeSDeprecationWarning from .command import Command from .parameter_base import ( @@ -177,9 +180,27 @@ class Parameter( """ + # Ordered list of keyword argument names (after 'name') for + # backwards-compatible positional argument handling. + # TODO: remove with handling of args below after QCoDeS 0.57 + _DEPRECATED_POSITIONAL_ARGS: ClassVar[tuple[str, ...]] = ( + "instrument", + "label", + "unit", + "get_cmd", + "set_cmd", + "initial_value", + "max_val_age", + "vals", + "docstring", + "initial_cache_value", + "bind_to_instrument", + ) + def __init__( self, name: str, + *args: Any, # mypy seems to be confused here. The bound and default for InstrumentTypeVar_co # contains None but mypy will not allow None as a default as of v 1.19.0 instrument: InstrumentTypeVar_co = None, # type: ignore[assignment] @@ -195,6 +216,82 @@ def __init__( bind_to_instrument: bool = True, **kwargs: Any, ) -> None: + if args: + # TODO: After QCoDeS 0.57 remove the args argument and delete this code block. + positional_names = self._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)" + ) + + _defaults: dict[str, Any] = { + "instrument": None, + "label": None, + "unit": None, + "get_cmd": None, + "set_cmd": False, + "initial_value": None, + "max_val_age": None, + "vals": None, + "docstring": None, + "initial_cache_value": None, + "bind_to_instrument": True, + } + + # Snapshot keyword values before any reassignment so we can + # detect duplicates (keyword value differs from its default). + _kwarg_vals: dict[str, Any] = { + "instrument": instrument, + "label": label, + "unit": unit, + "get_cmd": get_cmd, + "set_cmd": set_cmd, + "initial_value": initial_value, + "max_val_age": max_val_age, + "vals": vals, + "docstring": docstring, + "initial_cache_value": initial_cache_value, + "bind_to_instrument": bind_to_instrument, + } + + # Check for duplicate arguments (passed both positionally and + # as keyword). We detect this by checking whether the keyword + # value differs from its default for each positionally-supplied + # argument. + for i in range(len(args)): + arg_name = positional_names[i] + if _kwarg_vals[arg_name] is not _defaults[arg_name]: + 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( + f"Passing {names_str} as positional argument(s) to " + f"{type(self).__name__} is deprecated. " + f"Please pass them as keyword arguments.", + QCoDeSDeprecationWarning, + stacklevel=2, + ) + + # Apply positional values to the keyword parameter variables. + _pos = dict(zip(positional_names, args)) + instrument = _pos.get("instrument", instrument) + label = _pos.get("label", label) + unit = _pos.get("unit", unit) + get_cmd = _pos.get("get_cmd", get_cmd) + set_cmd = _pos.get("set_cmd", set_cmd) + initial_value = _pos.get("initial_value", initial_value) + max_val_age = _pos.get("max_val_age", max_val_age) + vals = _pos.get("vals", vals) + docstring = _pos.get("docstring", docstring) + initial_cache_value = _pos.get("initial_cache_value", initial_cache_value) + bind_to_instrument = _pos.get("bind_to_instrument", bind_to_instrument) + def _get_manual_parameter(self: Parameter) -> ParamRawDataType: if self.root_instrument is not None: mylogger: InstrumentLoggerAdapter | logging.Logger = ( diff --git a/src/qcodes/parameters/parameter_base.py b/src/qcodes/parameters/parameter_base.py index 9696cae375f..6ae75dcaf9c 100644 --- a/src/qcodes/parameters/parameter_base.py +++ b/src/qcodes/parameters/parameter_base.py @@ -15,7 +15,12 @@ from qcodes.metadatable import Metadatable, MetadatableWithName from qcodes.parameters import ParamSpecBase -from qcodes.utils import DelegateAttributes, full_class, qcodes_abstractmethod +from qcodes.utils import ( + DelegateAttributes, + QCoDeSDeprecationWarning, + full_class, + qcodes_abstractmethod, +) from qcodes.validators import ( Arrays, ComplexNumbers, @@ -226,10 +231,38 @@ class ParameterBase( Callable[[ParameterBase, ParamDataType], None] | None ] = None + # Ordered list of keyword argument names (after 'name') for + # backwards-compatible positional argument handling. + # TODO: remove with handling of args below after QCoDeS 0.57 + _DEPRECATED_POSITIONAL_ARGS: ClassVar[tuple[str, ...]] = ( + "instrument", + "snapshot_get", + "metadata", + "step", + "scale", + "offset", + "inter_delay", + "post_delay", + "val_mapping", + "get_parser", + "set_parser", + "snapshot_value", + "snapshot_exclude", + "max_val_age", + "vals", + "abstract", + "bind_to_instrument", + "register_name", + "on_set_callback", + ) + def __init__( self, name: str, - instrument: InstrumentTypeVar_co, + *args: Any, + # mypy seems to be confused here. The bound and default for InstrumentTypeVar_co + # contains None but mypy will not allow None as a default as of v 1.19.0 + instrument: InstrumentTypeVar_co = None, # type: ignore[assignment] snapshot_get: bool = True, metadata: Mapping[Any, Any] | None = None, step: float | None = None, @@ -250,6 +283,106 @@ def __init__( on_set_callback: Callable[[ParameterBase, ParameterDataTypeVar], None] | None = None, ) -> None: + if args: + # TODO: After QCoDeS 0.57 remove the args argument and delete this code block. + positional_names = self._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)" + ) + + _defaults: dict[str, Any] = { + "instrument": None, + "snapshot_get": True, + "metadata": None, + "step": None, + "scale": None, + "offset": None, + "inter_delay": 0, + "post_delay": 0, + "val_mapping": None, + "get_parser": None, + "set_parser": None, + "snapshot_value": True, + "snapshot_exclude": False, + "max_val_age": None, + "vals": None, + "abstract": False, + "bind_to_instrument": True, + "register_name": None, + "on_set_callback": None, + } + + # Snapshot keyword values before any reassignment so we can + # detect duplicates (keyword value differs from its default). + _kwarg_vals: dict[str, Any] = { + "instrument": instrument, + "snapshot_get": snapshot_get, + "metadata": metadata, + "step": step, + "scale": scale, + "offset": offset, + "inter_delay": inter_delay, + "post_delay": post_delay, + "val_mapping": val_mapping, + "get_parser": get_parser, + "set_parser": set_parser, + "snapshot_value": snapshot_value, + "snapshot_exclude": snapshot_exclude, + "max_val_age": max_val_age, + "vals": vals, + "abstract": abstract, + "bind_to_instrument": bind_to_instrument, + "register_name": register_name, + "on_set_callback": on_set_callback, + } + + # Check for duplicate arguments (passed both positionally and + # as keyword). We detect this by checking whether the keyword + # value differs from its default for each positionally-supplied + # argument. + for i in range(len(args)): + arg_name = positional_names[i] + if _kwarg_vals[arg_name] is not _defaults[arg_name]: + 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( + f"Passing {names_str} as positional argument(s) to " + f"{type(self).__name__} is deprecated. " + f"Please pass them as keyword arguments.", + QCoDeSDeprecationWarning, + stacklevel=2, + ) + + # Apply positional values to the keyword parameter variables. + _pos = dict(zip(positional_names, args)) + instrument = _pos.get("instrument", instrument) + snapshot_get = _pos.get("snapshot_get", snapshot_get) + metadata = _pos.get("metadata", metadata) + step = _pos.get("step", step) + scale = _pos.get("scale", scale) + offset = _pos.get("offset", offset) + inter_delay = _pos.get("inter_delay", inter_delay) + post_delay = _pos.get("post_delay", post_delay) + val_mapping = _pos.get("val_mapping", val_mapping) + get_parser = _pos.get("get_parser", get_parser) + set_parser = _pos.get("set_parser", set_parser) + snapshot_value = _pos.get("snapshot_value", snapshot_value) + snapshot_exclude = _pos.get("snapshot_exclude", snapshot_exclude) + max_val_age = _pos.get("max_val_age", max_val_age) + vals = _pos.get("vals", vals) + abstract = _pos.get("abstract", abstract) + bind_to_instrument = _pos.get("bind_to_instrument", bind_to_instrument) + register_name = _pos.get("register_name", register_name) + on_set_callback = _pos.get("on_set_callback", on_set_callback) + super().__init__(metadata) if not str(name).isidentifier(): raise ValueError( diff --git a/tests/parameter/test_args_deprecated.py b/tests/parameter/test_args_deprecated.py new file mode 100644 index 00000000000..a741909fefb --- /dev/null +++ b/tests/parameter/test_args_deprecated.py @@ -0,0 +1,123 @@ +"""Tests that passing positional arguments (beyond ``name``) to +ParameterBase, Parameter, and DelegateParameter triggers a +QCoDeSDeprecationWarning. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from qcodes.instrument import Instrument +from qcodes.parameters import DelegateParameter, Parameter, ParameterBase +from qcodes.utils import QCoDeSDeprecationWarning + + +class _MockInstrument(Instrument): + """A minimal instrument for testing.""" + + def __init__(self, name: str = "mock"): + super().__init__(name) + + +@pytest.fixture +def mock_instrument() -> Any: + inst = _MockInstrument("dup_test") + yield inst + inst.close() + + +# Minimal concrete subclass of ParameterBase for testing +class _ConcreteParameterBase(ParameterBase): + def get_raw(self) -> Any: + return 0 + + +class TestParameterBasePositionalArgs: + """ParameterBase should warn when arguments after ``name`` are positional.""" + + def test_single_positional_arg_warns(self) -> None: + with pytest.warns( + QCoDeSDeprecationWarning, + match="Passing 'instrument' as positional argument", + ): + _ConcreteParameterBase("test", None) + + def test_multiple_positional_args_warn(self) -> None: + with pytest.warns( + QCoDeSDeprecationWarning, + match="'instrument', 'snapshot_get'", + ): + _ConcreteParameterBase("test", None, True) + + def test_keyword_args_do_not_warn(self) -> None: + # No warning should be raised when all args are keyword-only + p = _ConcreteParameterBase("test", instrument=None, snapshot_get=True) + assert p.name == "test" + + def test_duplicate_positional_and_keyword_raises( + self, mock_instrument: _MockInstrument + ) -> None: + with pytest.raises( + TypeError, + match="got multiple values for argument 'instrument'", + ): + _ConcreteParameterBase("test", None, instrument=mock_instrument) + + def test_too_many_positional_args_raises(self) -> None: + # More positional args than defined parameter names + too_many = (None,) * 25 + with pytest.raises(TypeError, match="takes at most"): + _ConcreteParameterBase("test", *too_many) + + +class TestParameterPositionalArgs: + """Parameter should warn when arguments after ``name`` are positional.""" + + def test_single_positional_arg_warns(self) -> None: + with pytest.warns( + QCoDeSDeprecationWarning, + match="Passing 'instrument' as positional argument", + ): + Parameter("test", None, set_cmd=None) + + def test_multiple_positional_args_warn(self) -> None: + with pytest.warns( + QCoDeSDeprecationWarning, + match="'instrument', 'label'", + ): + Parameter("test", None, "my label", set_cmd=None) + + def test_keyword_args_do_not_warn(self) -> None: + p = Parameter("test", instrument=None, label="my label", set_cmd=None) + assert p.name == "test" + assert p.label == "my label" + + def test_duplicate_positional_and_keyword_raises( + self, mock_instrument: _MockInstrument + ) -> None: + with pytest.raises( + TypeError, + match="got multiple values for argument 'instrument'", + ): + Parameter("test", None, instrument=mock_instrument) + + def test_too_many_positional_args_raises(self) -> None: + too_many = (None,) * 15 + with pytest.raises(TypeError, match="takes at most"): + Parameter("test", *too_many) + + +class TestDelegateParameterPositionalArgs: + """DelegateParameter should warn when extra positional args are passed.""" + + def test_positional_args_warn(self) -> None: + source = Parameter("source", set_cmd=None) + with pytest.warns(QCoDeSDeprecationWarning): + 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) + assert p.name == "test" diff --git a/tests/parameter/test_get_latest.py b/tests/parameter/test_get_latest.py index c58ee1a2542..d2c6780afa5 100644 --- a/tests/parameter/test_get_latest.py +++ b/tests/parameter/test_get_latest.py @@ -172,6 +172,6 @@ def __init__(self, *args: Any, **kwargs: Any): self.set_raw = lambda x: x # type: ignore[method-assign] self.set = self._wrap_set(self.set_raw) - localparameter = LocalParameter("test_param", None, max_val_age=1) + localparameter = LocalParameter("test_param", instrument=None, max_val_age=1) with pytest.raises(RuntimeError): localparameter.get_latest() diff --git a/tests/parameter/test_parameter_cache.py b/tests/parameter/test_parameter_cache.py index 2979957f75a..1768b7de72f 100644 --- a/tests/parameter/test_parameter_cache.py +++ b/tests/parameter/test_parameter_cache.py @@ -231,7 +231,7 @@ def __init__(self, *args: Any, **kwargs: Any): self.set_raw = lambda x: x # type: ignore[method-assign] self.set = self._wrap_set(self.set_raw) - local_parameter = LocalParameter("test_param", None, max_val_age=1) + local_parameter = LocalParameter("test_param", instrument=None, max_val_age=1) start = datetime.now() set_time = start - timedelta(seconds=10) local_parameter.cache._update_with(value=value, raw_value=value, timestamp=set_time)