fix command doc string handling and change default stop doc string

- fix inheritance of command description
- when no stop method is given, then the description should indicate
  that stop is a no-op -> add missing doc strings to stop methods
- add test to make sure stop command doc strings are given
  when implemented

Change-Id: If891359350e8dcdec39a706841d61d4f8ec8926f
Reviewed-on: https://forge.frm2.tum.de/review/c/secop/frappy/+/33266
Tested-by: Jenkins Automated Tests <pedersen+jenkins@frm2.tum.de>
Reviewed-by: Enrico Faulhaber <enrico.faulhaber@frm2.tum.de>
Reviewed-by: Alexander Zaft <a.zaft@fz-juelich.de>
Reviewed-by: Markus Zolliker <markus.zolliker@psi.ch>

frappy/extparams.py

@@ -79,7 +79,9 @@ class StructParam(Parameter):
             datatype = StructOf(**{m: p.datatype for m, p in paramdict.items()})
             kwds['influences'] = [p.name for p in paramdict.values()]
         self.updateEnable = {}
-        super().__init__(description, datatype, paramdict=paramdict, readonly=readonly, **kwds)
+        if paramdict:
+            kwds['paramdict'] = paramdict
+        super().__init__(description, datatype, readonly=readonly, **kwds)
 
     def __set_name__(self, owner, name):
         # names of access methods of structed param (e.g. ctrlpars)

frappy/lib/sequence.py

@@ -141,6 +141,7 @@ class SequencerMixin:
         return self.Status.IDLE, ''
 
     def stop(self):
+        """stop sequence"""
         if self.seq_is_alive():
             self._seq_stopflag = True
 

frappy/modulebase.py

@@ -83,15 +83,14 @@ class HasAccessibles(HasProperties):
                     override_values.pop(key, None)
                 elif key in accessibles:
                     override_values[key] = value
+        # remark: merged_properties contain already the properties of accessibles of cls
         for aname, aobj in list(accessibles.items()):
             if aname in override_values:
-                aobj = aobj.copy()
                 value = override_values[aname]
                 if value is None:
                     accessibles.pop(aname)
                     continue
-                aobj.merge(merged_properties[aname])
+                aobj = aobj.create_from_value(merged_properties[aname], value)
-                aobj.override(value)
                 # replace the bare value by the created accessible
                 setattr(cls, aname, aobj)
             else:

frappy/modules.py

@@ -36,6 +36,7 @@ from .modulebase import Module
 
 class Readable(Module):
     """basic readable module"""
+    # pylint: disable=invalid-name
     Status = Enum('Status',
                   IDLE=StatusType.IDLE,
                   WARN=StatusType.WARN,
@@ -92,7 +93,7 @@ class Drivable(Writable):
 
     @Command(None, result=None)
     def stop(self):
-        """cease driving, go to IDLE state"""
+        """not implemented - this is a no-op"""
 
 
 class Communicator(HasComlog, Module):

frappy/params.py

@@ -57,13 +57,17 @@ class Accessible(HasProperties):
     def as_dict(self):
         return self.propertyValues
 
-    def override(self, value):
+    def create_from_value(self, properties, value):
-        """override with a bare value"""
+        """return a clone with given value and inherited properties"""
+        raise NotImplementedError
+
+    def clone(self, properties, **kwds):
+        """return a clone of ourselfs with inherited properties"""
         raise NotImplementedError
 
     def copy(self):
         """return a (deep) copy of ourselfs"""
-        raise NotImplementedError
+        return self.clone(self.propertyValues)
 
     def updateProperties(self, merged_properties):
         """update merged_properties with our own properties"""
@@ -234,13 +238,15 @@ class Parameter(Accessible):
                 # avoid export=True overrides export=<name>
                 self.ownProperties['export'] = self.export
 
-    def copy(self):
+    def clone(self, properties, **kwds):
-        """return a (deep) copy of ourselfs"""
+        """return a clone of ourselfs with inherited properties"""
-        res = type(self)()
+        res = type(self)(**kwds)
         res.name = self.name
-        res.init(self.propertyValues)
+        res.init(properties)
+        res.init(res.ownProperties)
         if 'datatype' in self.propertyValues:
             res.datatype = res.datatype.copy()
+        res.finish()
         return res
 
     def updateProperties(self, merged_properties):
@@ -253,9 +259,9 @@ class Parameter(Accessible):
                     merged_properties.pop(key)
         merged_properties.update(self.ownProperties)
 
-    def override(self, value):
+    def create_from_value(self, properties, value):
-        """override default"""
+        """return a clone with given value and inherited properties"""
-        self.value = self.datatype(value)
+        return self.clone(properties, value=self.datatype(value))
 
     def merge(self, merged_properties):
         """merge with inherited properties
@@ -390,7 +396,7 @@ class Command(Accessible):
         else:
             # goodie: allow @Command instead of @Command()
             self.func = argument  # this is the wrapped method!
-            if argument.__doc__:
+            if argument.__doc__ is not None:
                 self.description = inspect.cleandoc(argument.__doc__)
             self.name = self.func.__name__  # this is probably not needed
         self._inherit = inherit  # save for __set_name__
@@ -439,38 +445,37 @@ class Command(Accessible):
                                        f' members!: {params} != {members}')
             self.argument.optional = [p for p,v in sig.parameters.items()
                    if v.default is not inspect.Parameter.empty]
-        if 'description' not in self.propertyValues and func.__doc__:
+        if 'description' not in self.ownProperties and func.__doc__ is not None:
             self.description = inspect.cleandoc(func.__doc__)
             self.ownProperties['description'] = self.description
         self.func = func
         return self
 
-    def copy(self):
+    def clone(self, properties, **kwds):
-        """return a (deep) copy of ourselfs"""
+        """return a clone of ourselfs with inherited properties"""
-        res = type(self)()
+        res = type(self)(**kwds)
         res.name = self.name
         res.func = self.func
-        res.init(self.propertyValues)
+        res.init(properties)
+        res.init(res.ownProperties)
         if res.argument:
             res.argument = res.argument.copy()
         if res.result:
             res.result = res.result.copy()
-        self.finish()
+        res.finish()
         return res
 
     def updateProperties(self, merged_properties):
         """update merged_properties with our own properties"""
         merged_properties.update(self.ownProperties)
 
-    def override(self, value):
+    def create_from_value(self, properties, value):
-        """override method
+        """return a clone with given value and inherited properties
 
         this is needed when the @Command is missing on a method overriding a command"""
         if not callable(value):
             raise ProgrammingError(f'{self.name} = {value!r} is overriding a Command')
-        self.func = value
+        return self.clone(properties)(value)
-        if value.__doc__:
-            self.description = inspect.cleandoc(value.__doc__)
 
     def merge(self, merged_properties):
         """merge with inherited properties

frappy/states.py

@@ -239,6 +239,7 @@ class HasStates:
 
     @Command
     def stop(self):
+        """stop state machine"""
         self.stop_machine()
 
     def final_status(self, code=IDLE, text=''):

frappy_demo/modules.py

@@ -120,6 +120,7 @@ class MagneticField(Drivable):
                      )
     heatswitch = Attached(Switch, description='name of heat switch device')
 
+    # pylint: disable=invalid-name
     Status = Enum(Drivable.Status, PERSIST=PERSIST, PREPARE=301, RAMPING=302, FINISH=303)
 
     status = Parameter(datatype=TupleOf(EnumType(Status), StringType()))
@@ -193,6 +194,7 @@ class MagneticField(Drivable):
         self.log.error(self, 'main thread exited unexpectedly!')
 
     def stop(self):
+        """stop at current value"""
         self.write_target(self.read_value())
 
 

frappy_mlz/entangle.py

@@ -641,6 +641,7 @@ class AnalogOutput(PyTangoDevice, Drivable):
             sleep(0.3)
 
     def stop(self):
+        """cease driving, go to IDLE state"""
         self._dev.Stop()
 
 

frappy_psi/motorvalve.py

@@ -198,6 +198,10 @@ class MotorValve(PersistentMixin, Drivable):
 
     @Command
     def stop(self):
+        """stop at current position
+
+        state will probably be undefined
+        """
         self._state.stop()
         self.motor.stop()
 

frappy_psi/ppms.py

@@ -483,6 +483,10 @@ class Temp(PpmsDrivable):
         self._expected_target_time = time.time() + abs(target - self.value) * 60.0 / max(0.1, ramp)
 
     def stop(self):
+        """set setpoint to current value
+
+        but restrict to values between last target and current target
+        """
         if not self.isDriving():
             return
         if self.status[0] != StatusType.STABILIZING:
@@ -612,6 +616,7 @@ class Field(PpmsDrivable):
         # do not execute FIELD command, as this would trigger a ramp up of leads current
 
     def stop(self):
+        """stop at current driven Field"""
         if not self.isDriving():
             return
         newtarget = clamp(self._last_target, self.value, self.target)
@@ -714,6 +719,7 @@ class Position(PpmsDrivable):
         return value  # do not execute MOVE command, as this would trigger an unnecessary move
 
     def stop(self):
+        """stop motor"""
         if not self.isDriving():
             return
         newtarget = clamp(self._last_target, self.value, self.target)

test/test_modules.py

@@ -23,6 +23,8 @@
 
 import sys
 import threading
+import importlib
+from glob import glob
 import pytest
 
 from frappy.datatypes import BoolType, FloatRange, StringType, IntRange, ScaledInteger
@@ -440,12 +442,12 @@ def test_override():
     assert Mod.value.value == 5
     assert Mod.stop.description == "no decorator needed"
 
-    class Mod2(Drivable):
+    class Mod2(Mod):
-        @Command()
         def stop(self):
             pass
 
-    assert Mod2.stop.description == Drivable.stop.description
+    # inherit doc string
+    assert Mod2.stop.description == Mod.stop.description
 
 
 def test_command_config():
@@ -920,3 +922,24 @@ def test_interface_classes(bases, iface_classes):
         pass
     m = Mod('mod', LoggerStub(), {'description': 'test'}, srv)
     assert m.interface_classes == iface_classes
+
+
+all_drivables = set()
+for pyfile in glob('frappy_*/*.py'):
+    module = pyfile[:-3].replace('/', '.')
+    try:
+        importlib.import_module(module)
+    except Exception as e:
+        print(module, e)
+        continue
+    for obj_ in sys.modules[module].__dict__.values():
+        if isinstance(obj_, type) and issubclass(obj_, Drivable):
+            all_drivables.add(obj_)
+
+
+@pytest.mark.parametrize('modcls', all_drivables)
+def test_stop_doc(modcls):
+    # make sure that implemented stop methods have a doc string
+    if (modcls.stop.description == Drivable.stop.description
+            and modcls.stop.func != Drivable.stop.func):
+        assert modcls.stop.func.__doc__  # stop method needs a doc string