Repository: pymeasure/pymeasure

Issue:
Do not allow kwargs in property creators
We noticed, that removing kwargs from `setting` caught some errors. ( #843)

We could deprecated the usage of "kwargs" in control and measurement, adding a "v_kwargs" parameter, which is given to values as kwargs.

Additionally, all parameters of "values" have to be added to the signature as well. (spyder already composes the signature, including called methods, too) 

That way we keep flexibility (you can hand on arbitrary arguments to values and then to ask), but we catch many errors in normal cases.

What do you think? 

Discussion:

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 217e0c91..de9bfb0e 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -13,6 +13,7 @@ Deprecated features
 - Toptica IBeamSmart: :code:`channel1_enabled`, use :code:`ch_1.enabled` property instead (similar channel2). Also :code:`laser_enabled` is deprecated in favor of :code:`emission`.
 - TelnetAdapter: use :code:`VISAAdapter` instead. VISA supports TCPIP connections. Use the resource_name :code:`TCPIP[board]::<hostname>::<port>::SOCKET` to connect to a server.
 - Attocube ANC300: :code:`host` argument, pass a resource string or adapter as :code:`Adapter` passed to :code:`Instrument`. Now communicates through the :code:`VISAAdapter` rather than deprecated :code:`TelnetAdapter`. The initializer now accepts :code:`name` as its second keyword argument so all previous initialization positional arguments (`axisnames`, `passwd`, `query_delay`) should be switched to keyword arguments.
+- The property creators :code:`control`, :code:`measurement`, and :code:`setting` do not accept arbitrary keyword arguments anymore. Use the :code:`v_kwargs` parameter to give further arguments to :code:`values` method.
 
 Version 0.11.1 (2022-12-31)
 ===========================
diff --git a/pymeasure/instruments/agilent/agilent33500.py b/pymeasure/instruments/agilent/agilent33500.py
index cdc5e52a..8feac45b 100644
--- a/pymeasure/instruments/agilent/agilent33500.py
+++ b/pymeasure/instruments/agilent/agilent33500.py
@@ -690,10 +690,6 @@ class Agilent33500(Instrument):
         """
         self.write("DATA:VOL:CLE")
 
-    def phase_sync(self):
-        """ Synchronize the phase of all channels."""
-        self.write("PHAS:SYNC")
-
     def data_arb(self, arb_name, data_points, data_format="DAC"):
         """
         Uploads an arbitrary trace into the volatile memory of the device.
diff --git a/pymeasure/instruments/aja/dcxs.py b/pymeasure/instruments/aja/dcxs.py
index 1ba3bc09..5a3ea6c3 100644
--- a/pymeasure/instruments/aja/dcxs.py
+++ b/pymeasure/instruments/aja/dcxs.py
@@ -71,60 +71,60 @@ class DCXS(Instrument):
     id = Instrument.measurement(
         "?", """Get the power supply type identifier.""",
         cast=str,
-        reply_length=9,
+        values_kwargs={'reply_length': 9},
     )
 
     software_version = Instrument.measurement(
         "z", """Get the software revision of the power supply firmware.""",
         cast=str,
-        reply_length=5,
+        values_kwargs={'reply_length': 5},
     )
 
     power = Instrument.measurement(
         "d", """Measure the actual output power in W.""",
         cast=int,
-        reply_length=4,
+        values_kwargs={'reply_length': 4},
     )
 
     voltage = Instrument.measurement(
         "e", """Measure the output voltage in V.""",
         cast=int,
-        reply_length=4,
+        values_kwargs={'reply_length': 4},
     )
 
     current = Instrument.measurement(
         "f", """Measure the output current in mA.""",
         cast=int,
-        reply_length=4,
+        values_kwargs={'reply_length': 4},
     )
 
     remaining_deposition_time_min = Instrument.measurement(
         "k", """Get the minutes part of remaining deposition time.""",
         cast=int,
-        reply_length=3,
+        values_kwargs={'reply_length': 3},
     )
 
     remaining_deposition_time_sec = Instrument.measurement(
         "l", """Get the seconds part of remaining deposition time.""",
         cast=int,
-        reply_length=2,
+        values_kwargs={'reply_length': 2},
     )
 
     fault_code = Instrument.measurement(
         "o", """Get the error code from the power supply.""",
-        reply_length=1,
+        values_kwargs={'reply_length': 1},
     )
 
     shutter_state = Instrument.measurement(
         "p", """Get the status of the gun shutters. 0 for closed and 1 for open shutters.""",
-        reply_length=1,
+        values_kwargs={'reply_length': 1},
         cast=lambda x: int.from_bytes(x.encode(), "big"),
         get_process=lambda x: [x & 1, x & 2, x & 4, x & 8, x & 16],
     )
 
     enabled = Instrument.control(
         "a", "%s", """Control the on/off state of the power supply""",
-        reply_length=1,
+        values_kwargs={'reply_length': 1},
         validator=strict_discrete_set,
         map_values=True,
         cast=int,
@@ -136,7 +136,7 @@ class DCXS(Instrument):
         "b", "C%04d",
         """Control the setpoint value. Units are determined by regulation mode
            (power -> W, voltage -> V, current -> mA).""",
-        reply_length=4,
+        values_kwargs={'reply_length': 4},
         validator=strict_range,
         map_values=True,
         values=range(0, 1001),
@@ -145,7 +145,7 @@ class DCXS(Instrument):
     regulation_mode = Instrument.control(
         "c", "D%d",
         """Control the regulation mode of the power supply.""",
-        reply_length=1,
+        values_kwargs={'reply_length': 1},
         validator=strict_discrete_set,
         map_values=True,
         values={"power": 0,
@@ -157,7 +157,7 @@ class DCXS(Instrument):
     ramp_time = Instrument.control(
         "g", "E%02d",
         """Control the ramp time in seconds. Can be set only when 'enabled' is False.""",
-        reply_length=2,
+        values_kwargs={'reply_length': 2},
         cast=int,
         validator=strict_range,
         values=range(100),
@@ -166,7 +166,7 @@ class DCXS(Instrument):
     shutter_delay = Instrument.control(
         "h", "F%02d",
         """Control the shutter delay in seconds. Can be set only when 'enabled' is False.""",
-        reply_length=2,
+        values_kwargs={'reply_length': 2},
         cast=int,
         validator=strict_range,
         values=range(100),
@@ -175,7 +175,7 @@ class DCXS(Instrument):
     deposition_time_min = Instrument.control(
         "i", "G%03d",
         """Control the minutes part of deposition time. Can be set only when 'enabled' is False.""",
-        reply_length=3,
+        values_kwargs={'reply_length': 3},
         cast=int,
         validator=strict_range,
         values=range(1000),
@@ -184,7 +184,7 @@ class DCXS(Instrument):
     deposition_time_sec = Instrument.control(
         "j", "H%02d",
         """Control the seconds part of deposition time. Can be set only when 'enabled' is False.""",
-        reply_length=2,
+        values_kwargs={'reply_length': 2},
         cast=int,
         validator=strict_range,
         values=range(60),
@@ -193,7 +193,7 @@ class DCXS(Instrument):
     material = Instrument.control(
         "n", "I%08s", """Control the material name of the sputter target.""",
         cast=str,
-        reply_length=8,
+        values_kwargs={'reply_length': 8},
         validator=lambda value, maxlength: value[:maxlength],
         values=8,
     )
@@ -201,7 +201,7 @@ class DCXS(Instrument):
     active_gun = Instrument.control(
         "y", "Z%d", """Control the active gun number.""",
         cast=int,
-        reply_length=1,
+        values_kwargs={'reply_length': 1},
         validator=strict_range,
         values=range(1, 6),
     )
diff --git a/pymeasure/instruments/anritsu/anritsuMS2090A.py b/pymeasure/instruments/anritsu/anritsuMS2090A.py
index 38e559da..49ab1a89 100644
--- a/pymeasure/instruments/anritsu/anritsuMS2090A.py
+++ b/pymeasure/instruments/anritsu/anritsuMS2090A.py
@@ -219,7 +219,6 @@ class AnritsuMS2090A(Instrument):
         """
         Return the EMF measurement data for a specified sample number. JSON format.
         """,
-        validator=strict_range,
         values=[1, 16],
     )
 
@@ -242,7 +241,6 @@ class AnritsuMS2090A(Instrument):
         """
         Returns the different set of measurement information depending on the suffix.
         """,
-        validator=truncated_range,
         values=[1, 2]
     )
 
diff --git a/pymeasure/instruments/common_base.py b/pymeasure/instruments/common_base.py
index 005f318d..0e5591af 100644
--- a/pymeasure/instruments/common_base.py
+++ b/pymeasure/instruments/common_base.py
@@ -24,6 +24,7 @@
 
 from inspect import getmembers
 import logging
+from warnings import warn
 
 log = logging.getLogger(__name__)
 log.addHandler(logging.NullHandler())
@@ -295,15 +296,17 @@ class CommonBase:
         """Write a command to the instrument and return a list of formatted
         values from the result.
 
-        :param command: SCPI command to be sent to the instrument
-        :param separator: A separator character to split the string into a list
-        :param cast: A type to cast the result
-        :param preprocess_reply: optional callable used to preprocess values
-            received from the instrument. The callable returns the processed
-            string.
-        :param maxsplit: At most `maxsplit` splits are done. -1 (default) indicates no limit.
+        :param command: SCPI command to be sent to the instrument.
+        :param preprocess_reply: Optional callable used to preprocess the string
+            received from the instrument, before splitting it.
+            The callable returns the processed string.
+        :param separator: A separator character to split the string returned by
+            the device into a list.
+        :param maxsplit: The string returned by the device is splitted at most `maxsplit` times.
+            -1 (default) indicates no limit.
+        :param cast: A type to cast each element of the splitted string.
         :param \\**kwargs: Keyword arguments to be passed to the :meth:`ask` method.
-        :returns: A list of the desired type, or strings where the casting fails
+        :returns: A list of the desired type, or strings where the casting fails.
         """
         results = self.ask(command, **kwargs).strip()
         if callable(preprocess_reply):
@@ -348,6 +351,11 @@ class CommonBase:
         check_set_errors=False,
         check_get_errors=False,
         dynamic=False,
+        preprocess_reply=None,
+        separator=',',
+        maxsplit=-1,
+        cast=float,
+        values_kwargs=None,
         **kwargs
     ):
         """Return a property for the class based on the supplied
@@ -375,8 +383,20 @@ class CommonBase:
         :param check_get_errors: Toggles checking errors after getting
         :param dynamic: Specify whether the property parameters are meant to be changed in
             instances or subclasses.
+        :param preprocess_reply: Optional callable used to preprocess the string
+            received from the instrument, before splitting it.
+            The callable returns the processed string.
+        :param separator: A separator character to split the string returned by
+            the device into a list.
+        :param maxsplit: The string returned by the device is splitted at most `maxsplit` times.
+            -1 (default) indicates no limit.
+        :param cast: A type to cast each element of the splitted string.
+        :param dict values_kwargs: Further keyword arguments for :meth:`values`.
         :param \\**kwargs: Keyword arguments for :meth:`values`.
 
+            ..deprecated:: 0.12
+                Use `values_kwargs` dictionary parameter instead.
+
         Example of usage of dynamic parameter is as follows:
 
         .. code-block:: python
@@ -408,6 +428,13 @@ class CommonBase:
         parameters name except `dynamic` and `docs` (e.g. `values` in the example) has to be
         considered reserved for dynamic property control.
         """
+        if values_kwargs is None:
+            values_kwargs = {}
+        if kwargs:
+            warn(f"Do not use keyword arguments {kwargs} as `control` parameter "
+                 f"for the `values` method, use `values_kwargs` parameter instead. docs:\n{docs}",
+                 FutureWarning)
+            values_kwargs.update(kwargs)
 
         def fget(self,
                  get_command=get_command,
@@ -419,7 +446,12 @@ class CommonBase:
                  ):
             if get_command is None:
                 raise LookupError("Property can not be read.")
-            vals = self.values(command_process(get_command), **kwargs)
+            vals = self.values(command_process(get_command),
+                               separator=separator,
+                               cast=cast,
+                               preprocess_reply=preprocess_reply,
+                               maxsplit=maxsplit,
+                               **values_kwargs)
             if check_get_errors:
                 self.check_errors()
             if len(vals) == 1:
@@ -487,7 +519,13 @@ class CommonBase:
     @staticmethod
     def measurement(get_command, docs, values=(), map_values=None,
                     get_process=lambda v: v, command_process=lambda c: c,
-                    check_get_errors=False, dynamic=False, **kwargs):
+                    check_get_errors=False, dynamic=False,
+                    preprocess_reply=None,
+                    separator=',',
+                    maxsplit=-1,
+                    cast=float,
+                    values_kwargs=None,
+                    **kwargs):
         """ Return a property for the class based on the supplied
         commands. This is a measurement quantity that may only be
         read from the instrument, not set.
@@ -505,8 +543,27 @@ class CommonBase:
         :param check_get_errors: Toggles checking errors after getting
         :param dynamic: Specify whether the property parameters are meant to be changed in
             instances or subclasses. See :meth:`control` for an usage example.
+        :param preprocess_reply: Optional callable used to preprocess the string
+            received from the instrument, before splitting it.
+            The callable returns the processed string.
+        :param separator: A separator character to split the string returned by
+            the device into a list.
+        :param maxsplit: The string returned by the device is splitted at most `maxsplit` times.
+            -1 (default) indicates no limit.
+        :param cast: A type to cast each element of the splitted string.
+        :param dict values_kwargs: Further keyword arguments for :meth:`values`.
         :param \\**kwargs: Keyword arguments for :meth:`values`.
+
+            ..deprecated:: 0.12
+                Use `values_kwargs` dictionary parameter instead.
         """
+        if values_kwargs is None:
+            values_kwargs = {}
+        if kwargs:
+            warn(f"Do not use keyword arguments {kwargs} as `measurement` parameter "
+                 f"for the `values` method, use `values_kwargs` parameter instead. docs:\n{docs}",
+                 FutureWarning)
+            values_kwargs.update(kwargs)
 
         return CommonBase.control(get_command=get_command,
                                   set_command=None,
@@ -517,7 +574,12 @@ class CommonBase:
                                   command_process=command_process,
                                   check_get_errors=check_get_errors,
                                   dynamic=dynamic,
-                                  **kwargs)
+                                  preprocess_reply=preprocess_reply,
+                                  separator=separator,
+                                  maxsplit=maxsplit,
+                                  cast=cast,
+                                  values_kwargs=values_kwargs,
+                                  )
 
     @staticmethod
     def setting(set_command, docs,
diff --git a/pymeasure/instruments/lecroy/lecroyT3DSO1204.py b/pymeasure/instruments/lecroy/lecroyT3DSO1204.py
index 6a8987cd..4f2e8eb5 100644
--- a/pymeasure/instruments/lecroy/lecroyT3DSO1204.py
+++ b/pymeasure/instruments/lecroy/lecroyT3DSO1204.py
@@ -662,7 +662,6 @@ class LeCroyT3DSO1204(Instrument):
 
     acquisition_status = Instrument.measurement(
         "SAST?", """A string parameter that defines the acquisition status of the scope.""",
-        validator=strict_discrete_set,
         values={"stopped": "Stop", "triggered": "Trig'd", "ready": "Ready", "auto": "Auto",
                 "armed": "Arm"},
         map_values=True
diff --git a/pymeasure/instruments/temptronic/temptronic_base.py b/pymeasure/instruments/temptronic/temptronic_base.py
index c8342318..1649c0a5 100644
--- a/pymeasure/instruments/temptronic/temptronic_base.py
+++ b/pymeasure/instruments/temptronic/temptronic_base.py
@@ -325,7 +325,6 @@ class ATSBase(Instrument):
 
         :type: :class:`.TemperatureStatusCode`
         """,
-        validator=truncated_range,
         values=[0, 255],
         get_process=lambda v: TemperatureStatusCode(int(v)),
     )
@@ -379,8 +378,6 @@ class ATSBase(Instrument):
         Refere to chapter 4 in the manual
 
         """,
-        validator=truncated_range,
-        values=[0, 1023]
     )
 
     copy_active_setup_file = Instrument.setting(
@@ -447,8 +444,6 @@ class ATSBase(Instrument):
 
         :type: :class:`.ErrorCode`
         """,
-        validator=truncated_range,
-        values=[0, int(2**16-1)],
         get_process=lambda v: ErrorCode(int(v)),
     )
 
@@ -521,8 +516,6 @@ class ATSBase(Instrument):
         Hint: Reading will clear register content.
 
         """,
-        validator=strict_range,
-        values=[0, 255]
     )
 
     air_temperature = Instrument.measurement(


Test patch:
diff --git a/pymeasure/instruments/advantest/advantestR624X.py b/pymeasure/instruments/advantest/advantestR624X.py
index 5dedde43..cb9481ed 100644
--- a/pymeasure/instruments/advantest/advantestR624X.py
+++ b/pymeasure/instruments/advantest/advantestR624X.py
@@ -938,7 +938,6 @@ class AdvantestR624X(Instrument):
             SESR is cleared after being read.
 
         """,
-        validator=truncated_range,
         values=[0, 255],
         get_process=lambda v: SESR(int(v)),
     )
@@ -949,7 +948,6 @@ class AdvantestR624X(Instrument):
         in the form of a :class:`DOR` ``IntFlag`` (``DOC?``).
 
         """,
-        validator=strict_range,
         values=range(0, 65535),
         get_process=lambda v: DOR(int(v)),
     )
@@ -2011,7 +2009,6 @@ class SMUChannel(Channel):
         in the form of a :class:`COR` ``IntFlag`` (``COC?``).
 
         """,
-        validator=strict_range,
         values=range(0, 65535),
         get_process=lambda v: COR(int(v)),
     )
diff --git a/tests/instruments/agilent/test_agilent33500.py b/tests/instruments/agilent/test_agilent33500.py
index 102a4fd8..701a4223 100644
--- a/tests/instruments/agilent/test_agilent33500.py
+++ b/tests/instruments/agilent/test_agilent33500.py
@@ -291,16 +291,3 @@ def test_ramp_symmetry(ramp_symmetry):
         inst.ch[1].ramp_symmetry = ramp_symmetry
         inst.ch[2].ramp_symmetry = ramp_symmetry
         inst.ramp_symmetry = ramp_symmetry
-
-
-def test_channel_phase_synchronization():
-    """
-    Test Agilent 33500 channel phase synchronization
-    """
-    with expected_protocol(
-        Agilent33500,
-        [
-            ("PHAS:SYNC", None)
-        ]
-    ) as inst:
-        assert inst.phase_sync() is None
diff --git a/tests/instruments/agilent/test_agilent33500_with_device.py b/tests/instruments/agilent/test_agilent33500_with_device.py
index a41036ab..5ddd9a19 100644
--- a/tests/instruments/agilent/test_agilent33500_with_device.py
+++ b/tests/instruments/agilent/test_agilent33500_with_device.py
@@ -76,6 +76,7 @@ BURST_NCYCLES = [1, 99999]
 SRATE = [1e-6, 1.6e8]
 
 
+@pytest.skip('Only works with connected hardware', allow_module_level=True)
 def generate_simple_harmonic_waveform(harmonic, distortion):
     """
     Generates a waveform with onlhy one harmonic
@@ -450,7 +451,3 @@ def test_uploaded_arb_file_channel(generator, channel):
     generator.ch[channel].data_arb("test", waveform, data_format="float")
     generator.ch[channel].arb_file = "test"
     assert '"TEST"' == generator.ch[channel].arb_file
-
-
-def test_phase_sync(generator):
-    generator.phase_sync()
diff --git a/tests/instruments/test_common_base.py b/tests/instruments/test_common_base.py
index baa9d282..2ddb892b 100644
--- a/tests/instruments/test_common_base.py
+++ b/tests/instruments/test_common_base.py
@@ -539,6 +539,84 @@ def test_control_preprocess_reply_property(dynamic):
     assert type(fake.x) == int
 
 
+def test_control_kwargs_handed_to_values():
+    """Test that kwargs parameters are handed to `values` method."""
+    with pytest.warns(FutureWarning, match="Do not use keyword arguments"):
+        class Fake(FakeBase):
+            x = CommonBase.control(
+                "", "JUNK%d",
+                "",
+                preprocess_reply=lambda v: v.replace('JUNK', ''),
+                cast=int,
+                testing=True,
+            )
+
+            def values(self, cmd, testing=False, **kwargs):
+                self.testing = testing
+                return super().values(cmd, **kwargs)
+
+    fake = Fake()
+    fake.x = 5
+    fake.x
+    assert fake.testing is True
+
+
+def test_control_warning_at_kwargs():
+    """Test whether a control kwarg raises a warning."""
+    with pytest.warns(FutureWarning, match="Do not use keyword arguments"):
+        class Fake(CommonBase):
+            x = CommonBase.control("", "", "", testing=True)
+
+
+def test_measurement_warning_at_kwargs():
+    """Test whether a measurement kwarg raises a warning."""
+    with pytest.warns(FutureWarning, match="Do not use keyword arguments"):
+        class Fake2(CommonBase):
+            x2 = CommonBase.measurement("", "", testing=True)
+
+
+def test_control_parameters_for_values():
+    """Test how to hand a parameter to `values` method."""
+    class Fake(FakeBase):
+        x = CommonBase.control(
+            "", "JUNK%d",
+            "",
+            preprocess_reply=lambda v: v.replace('JUNK', ''),
+            cast=int,
+            values_kwargs={'testing': True},
+        )
+
+        def values(self, cmd, testing=False, **kwargs):
+            self.testing = testing
+            return super().values(cmd, **kwargs)
+
+    fake = Fake()
+    fake.x = 5
+    fake.x
+    assert fake.testing is True
+
+
+def test_measurement_parameters_for_values():
+    """Test how to hand a parameter to `values` method."""
+    class Fake(FakeBase):
+        x = CommonBase.measurement(
+            "JUNK%d",
+            "",
+            preprocess_reply=lambda v: v.replace('JUNK', ''),
+            cast=int,
+            values_kwargs={'testing': True},
+        )
+
+        def values(self, cmd, testing=False, **kwargs):
+            self.testing = testing
+            return super().values(cmd, **kwargs)
+
+    fake = Fake()
+    fake.write("5")
+    fake.x
+    assert fake.testing is True
+
+
 @pytest.mark.parametrize("cast, expected", ((float, 5.5),
                                             (ureg.Quantity, ureg.Quantity(5.5)),
                                             (str, "5.5"),
diff --git a/tests/test_expected_protocol.py b/tests/test_expected_protocol.py
index 5619c374..baeab742 100644
--- a/tests/test_expected_protocol.py
+++ b/tests/test_expected_protocol.py
@@ -124,14 +124,17 @@ def test_non_empty_read_buffer():
 def test_preprocess_reply_on_values():
     class InstrumentWithPreprocessValues(BasicTestInstrument):
         """Workaround to get preprocess_reply working with protocol tests."""
-        def values(self, command, **kwargs):
-            return super().values(command, preprocess_reply=lambda v: v + "2345", **kwargs)
+        simple2 = Instrument.control(
+            "VOLT?", "VOLT %s V",
+            """Simple property replying with plain floats""",
+            preprocess_reply=lambda v: v + "2345"
+        )
 
     with expected_protocol(
         InstrumentWithPreprocessValues,
         [("VOLT?", "3.1")]
     ) as instr:
-        assert instr.simple == 3.12345
+        assert instr.simple2 == 3.12345
 
 
 class TestConnectionCalls: