new syntax for parameter/commands/properties

New Syntax:

- define properties and parameters as class attributes directly
  instead of items in class attribute dicts
- define commands with decorator @usercommand(...)
- old syntax is still supported for now

still to do (with decreasing priority):
- turn parameters into descriptors (vs. creating getters/setters)
- migrate all existing code to new syntax
- get rid of or reduce code in metaclasses using __set_name__ and
  __init_subclass__ instead, including a fix for allowing py < 3.6

Change-Id: Id47e0f89c506f50c40fa518b01822c6e5bbf4e98
Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/24991
Tested-by: Jenkins Automated Tests <pedersen+jenkins@frm2.tum.de>
Reviewed-by: Enrico Faulhaber <enrico.faulhaber@frm2.tum.de>
Reviewed-by: Markus Zolliker <markus.zolliker@psi.ch>

secop/core.py

@@ -31,7 +31,7 @@ from secop.datatypes import FloatRange, IntRange, ScaledInteger, \
 from secop.lib.enum import Enum
 from secop.modules import Module, Readable, Writable, Drivable, Communicator, Attached
 from secop.properties import Property
-from secop.params import Parameter, Command, Override
+from secop.params import Parameter, Command, Override, usercommand
 from secop.metaclass import Done
 from secop.iohandler import IOHandler, IOHandlerBase
 from secop.stringio import StringIO, HasIodev

secop/metaclass.py

@@ -26,9 +26,9 @@
 from collections import OrderedDict
 
 from secop.errors import ProgrammingError, BadValueError
-from secop.params import Command, Override, Parameter
+from secop.params import Command, Override, Parameter, Accessible, usercommand
 from secop.datatypes import EnumType
-from secop.properties import PropertyMeta
+from secop.properties import PropertyMeta, flatten_dict, Property
 
 
 class Done:
@@ -48,7 +48,14 @@ class ModuleMeta(PropertyMeta):
     and wraps read_*/write_* methods
     (so the dispatcher will get notfied of changed values)
     """
-    def __new__(cls, name, bases, attrs):
+    def __new__(cls, name, bases, attrs):  # pylint: disable=too-many-branches
+        # allow to declare accessibles directly as class attribute
+        # all these attributes are removed
+        flatten_dict('parameters', Parameter, attrs)
+        # do not remove commands from attrs, they are kept as descriptors
+        flatten_dict('commands', usercommand, attrs, remove=False)
+        flatten_dict('properties', Property, attrs)
+
         commands = attrs.pop('commands', {})
         parameters = attrs.pop('parameters', {})
         overrides = attrs.pop('overrides', {})
@@ -77,20 +84,19 @@ class ModuleMeta(PropertyMeta):
                     obj = obj.apply(accessibles[key])
                     accessibles[key] = obj
                 else:
-                    if key in accessibles:
+                    aobj = accessibles.get(key)
-                        # for now, accept redefinitions:
+                    if aobj:
-                        print("WARNING: module %s: %s should not be redefined"
+                        if obj.kwds is not None:  # obj may be used for override
-                              % (name, key))
+                            if isinstance(obj, Command) != isinstance(obj, Command):
-                        # raise ProgrammingError("module %s: %s must not be redefined"
+                                raise ProgrammingError("module %s.%s: can not override a %s with a %s!"
-                        #               % (name, key))
+                                                       % (name, key, aobj.__class_.name, obj.__class_.name, ))
-                    if isinstance(obj, Parameter):
+                            obj = aobj.override(obj)
-                        accessibles[key] = obj
+                            accessibles[key] = obj
-                    elif isinstance(obj, Command):
+                            setattr(newtype, key, obj)
-                        # XXX: convert to param with datatype=CommandType???
+                    if not isinstance(obj, (Parameter, Command)):
-                        accessibles[key] = obj
-                    else:
                         raise ProgrammingError('%r: accessibles entry %r should be a '
                                                'Parameter or Command object!' % (name, key))
+                    accessibles[key] = obj
 
         # Correct naming of EnumTypes
         for k, v in accessibles.items():
@@ -105,12 +111,22 @@ class ModuleMeta(PropertyMeta):
         # check for attributes overriding parameter values
         for pname, pobj in newtype.accessibles.items():
             if pname in attrs:
-                try:
+                value = attrs[pname]
-                    value = pobj.datatype(attrs[pname])
+                if isinstance(value, (Accessible, Override)):
-                except BadValueError:
+                    continue
-                    raise ProgrammingError('parameter %s can not be set to %r'
+                if isinstance(pobj, Parameter):
-                                           % (pname, attrs[pname]))
+                    try:
-                newtype.accessibles[pname] = Override(default=value).apply(pobj)
+                        value = pobj.datatype(attrs[pname])
+                    except BadValueError:
+                        raise ProgrammingError('parameter %r can not be set to %r'
+                                               % (pname, attrs[pname]))
+                    newtype.accessibles[pname] = pobj.override(default=value)
+                elif isinstance(pobj, usercommand):
+                    if not callable(attrs[pname]):
+                        raise ProgrammingError('%s.%s overwrites a command'
+                                               % (newtype.__name__, pname))
+                    pobj = pobj.override(func=attrs[name])
+                    newtype.accessibles[pname] = pobj
 
         # check validity of Parameter entries
         for pname, pobj in newtype.accessibles.items():
@@ -118,7 +134,11 @@ class ModuleMeta(PropertyMeta):
 
             # wrap of reading/writing funcs
             if isinstance(pobj, Command):
-                # skip commands for now
+                if isinstance(pobj, usercommand):
+                    do_name = 'do_' + pname
+                    # create additional method do_<pname> for backwards compatibility
+                    if do_name not in attrs:
+                        setattr(newtype, do_name, pobj)
                 continue
             rfunc = attrs.get('read_' + pname, None)
             rfunc_handler = pobj.handler.get_read_func(newtype, pname) if pobj.handler else None

secop/modules.py

@@ -208,6 +208,7 @@ class Module(HasProperties, metaclass=ModuleMeta):
             if pname in cfgdict:
                 if not pobj.readonly and pobj.initwrite is not False:
                     # parameters given in cfgdict have to call write_<pname>
+                    # TODO: not sure about readonly (why not a parameter which can only be written from config?)
                     try:
                         pobj.value = pobj.datatype(cfgdict[pname])
                     except BadValueError as e:
@@ -216,7 +217,7 @@ class Module(HasProperties, metaclass=ModuleMeta):
             else:
                 if pobj.default is None:
                     if pobj.needscfg:
-                        raise ConfigError('Module %s: Parameter %r has no default '
+                        raise ConfigError('Parameter %s.%s has no default '
                                           'value and was not given in config!' %
                                           (self.name, pname))
                     # we do not want to call the setter for this parameter for now,
@@ -231,9 +232,10 @@ class Module(HasProperties, metaclass=ModuleMeta):
                     except BadValueError as e:
                         raise ProgrammingError('bad default for %s.%s: %s'
                                                % (name, pname, e))
-                    if pobj.initwrite:
+                    if pobj.initwrite and not pobj.readonly:
                         # we will need to call write_<pname>
                         # if this is not desired, the default must not be given
+                        # TODO: not sure about readonly (why not a parameter which can only be written from config?)
                         pobj.value = value
                         self.writeDict[pname] = value
                     else:
@@ -542,6 +544,14 @@ class Communicator(Module):
                            ),
     }
 
+    def do_communicate(self, command):
+        """communicate command
+
+        :param command: the command to be sent
+        :return: the reply
+        """
+        raise NotImplementedError()
+
 
 class Attached(Property):
     # we can not put this to properties.py, as it needs datatypes

secop/params.py

@@ -27,7 +27,7 @@ from collections import OrderedDict
 import itertools
 
 from secop.datatypes import CommandType, DataType, StringType, BoolType, EnumType, DataTypeType, ValueType, OrType, \
-    NoneOr, TextType, IntRange
+    NoneOr, TextType, IntRange, TupleOf
 from secop.errors import ProgrammingError, BadValueError
 from secop.properties import HasProperties, Property
 
@@ -36,9 +36,10 @@ object_counter = itertools.count(1)
 
 
 class Accessible(HasProperties):
-    '''base class for Parameter and Command'''
+    """base class for Parameter and Command"""
 
     properties = {}
+    kwds = None  # is a dict if it might be used as Override
 
     def __init__(self, ctr, **kwds):
         self.ctr = ctr or next(object_counter)
@@ -49,16 +50,31 @@ class Accessible(HasProperties):
             self.setProperty(k, v)
 
     def __repr__(self):
-        return '%s(%s, ctr=%d)' % (self.__class__.__name__, ',\n\t'.join(
+        props = []
-            ['%s=%r' % (k, self.properties.get(k, v.default)) for k, v in sorted(self.__class__.properties.items())]),
+        for k, prop in sorted(self.__class__.properties.items()):
-            self.ctr)
+            v = self.properties.get(k, prop.default)
+            if v != prop.default:
+                props.append('%s=%r' % (k, v))
+        return '%s(%s, ctr=%d)' % (self.__class__.__name__, ', '.join(props), self.ctr)
+
+    def as_dict(self):
+        return self.properties
+
+    def override(self, from_object=None, **kwds):
+        """return a copy of ourselfs, modified by <other>"""
+        props = dict(self.properties, ctr=self.ctr)
+        if from_object:
+            props.update(from_object.kwds)
+        props.update(kwds)
+        props['datatype'] = props['datatype'].copy()
+        return type(self)(inherit=False, internally_called=True, **props)
 
     def copy(self):
-        # return a copy of ourselfs
+        """return a copy of ourselfs"""
         props = dict(self.properties, ctr=self.ctr)
         # deep copy, as datatype might be altered from config
         props['datatype'] = props['datatype'].copy()
-        return type(self)(**props)
+        return type(self)(inherit=False, internally_called=True, **props)
 
     def for_export(self):
         """prepare for serialisation"""
@@ -68,6 +84,14 @@ class Accessible(HasProperties):
 class Parameter(Accessible):
     """storage for Parameter settings + value + qualifiers
 
+    :param description: description
+    :param datatype: the datatype
+    :param inherit: whether properties not given should be inherited.
+      defaults to True when datatype or description is missing, else to False
+    :param ctr: inherited ctr
+    :param internally_called: True when called internally, else called from a definition
+    :param kwds: optional properties
+
     if readonly is False, the value can be changed (by code, or remote)
     if no default is given, the parameter MUST be specified in the configfile
     during startup, value is initialized with the default value or
@@ -96,7 +120,7 @@ class Parameter(Accessible):
         'datatype':    Property('Datatype of the Parameter', DataTypeType(),
                                  extname='datainfo', mandatory=True),
         'readonly':    Property('Is the Parameter readonly? (vs. changeable via SECoP)', BoolType(),
-                                 extname='readonly', mandatory=True),
+                                 extname='readonly', default=True),
         'group':       Property('Optional parameter group this parameter belongs to', StringType(),
                                  extname='group', default=''),
         'visibility':  Property('Optional visibility hint', EnumType('visibility', user=1, advanced=2, expert=3),
@@ -118,31 +142,44 @@ class Parameter(Accessible):
                                  NoneOr(BoolType()), export=False, default=None, mandatory=False, settable=False),
     }
 
-    def __init__(self, description, datatype, *, ctr=None, unit=None, **kwds):
+    def __init__(self, description=None, datatype=None, inherit=True, *,
+                 ctr=None, internally_called=False, reorder=False, **kwds):
+        if datatype is not None:
+            if not isinstance(datatype, DataType):
+                if isinstance(datatype, type) and issubclass(datatype, DataType):
+                    # goodie: make an instance from a class (forgotten ()???)
+                    datatype = datatype()
+                else:
+                    raise ProgrammingError(
+                        'datatype MUST be derived from class DataType!')
+            kwds['datatype'] = datatype
+        if description is not None:
+            kwds['description'] = description
 
-        if not isinstance(datatype, DataType):
+        unit = kwds.pop('unit', None)
-            if issubclass(datatype, DataType):
+        if unit is not None:   # for legacy code only
-                # goodie: make an instance from a class (forgotten ()???)
-                datatype = datatype()
-            else:
-                raise ProgrammingError(
-                    'datatype MUST be derived from class DataType!')
-
-        kwds['description'] = description
-        kwds['datatype'] = datatype
-        kwds['readonly'] = kwds.get('readonly', True) # for frappy optional, for SECoP mandatory
-        if unit is not None: # for legacy code only
             datatype.setProperty('unit', unit)
-        super(Parameter, self).__init__(ctr, **kwds)
-        if self.initwrite and self.readonly:
-            raise ProgrammingError('can not have both readonly and initwrite!')
 
-        if self.constant is not None:
+        constant = kwds.get('constant')
-            self.properties['readonly'] = True
+        if constant is not None:
+            constant = datatype(constant)
             # The value of the `constant` property should be the
             # serialised version of the constant, or unset
-            constant = self.datatype(kwds['constant'])
+            kwds['constant'] = datatype.export_value(constant)
-            self.properties['constant'] = self.datatype.export_value(constant)
+            kwds['readonly'] = True
+        if internally_called:  # fixes in case datatype has changed
+            default = kwds.get('default')
+            if default is not None:
+                try:
+                    datatype(default)
+                except BadValueError:
+                    # clear default, if it does not match datatype
+                    kwds['default'] = None
+        super().__init__(ctr, **kwds)
+        if inherit:
+            if reorder:
+                kwds['ctr'] = next(object_counter)
+            self.kwds = kwds  # contains only the items which must be overwritten
 
         # internal caching: value and timestamp of last change...
         self.value = self.default
@@ -204,13 +241,6 @@ class Parameters(OrderedDict):
         return super(Parameters, self).__getitem__(self.exported.get(item, item))
 
 
-class ParamValue:
-    __slots__ = ['value', 'timestamp']
-    def __init__(self, value, timestamp=0):
-        self.value = value
-        self.timestamp = timestamp
-
-
 class Commands(Parameters):
     """class storage for Commands"""
 
@@ -236,27 +266,7 @@ class Override:
             ['%s=%r' % (k, v) for k, v in sorted(self.kwds.items())]))
 
     def apply(self, obj):
-        if isinstance(obj, Accessible):
+        return obj.override(self)
-            props = obj.properties.copy()
-            props['datatype'] = props['datatype'].copy()
-            if isinstance(obj, Parameter):
-                if 'constant' in self.kwds:
-                    constant = obj.datatype(self.kwds.pop('constant'))
-                    self.kwds['constant'] = obj.datatype.export_value(constant)
-                    self.kwds['readonly'] = True
-                if 'datatype' in self.kwds and 'default' not in self.kwds:
-                    try:
-                        self.kwds['datatype'](obj.default)
-                    except BadValueError:
-                        # clear default, if it does not match datatype
-                        props['default'] = None
-            props['ctr'] = obj.ctr  # take ctr from inherited param except when overridden by self.kwds
-            props.update(self.kwds)
-            return type(obj)(**props)
-
-        raise ProgrammingError(
-            "Overrides can only be applied to Accessibles, %r is none!" %
-            obj)
 
 
 class Command(Accessible):
@@ -281,10 +291,30 @@ class Command(Accessible):
                               NoneOr(DataTypeType()), export=False, mandatory=True),
     }
 
-    def __init__(self, description, ctr=None, **kwds):
+    def __init__(self, description=None, *, ctr=None, inherit=True,
-        kwds['description'] = description
+                 internally_called=False, reorder=False, **kwds):
-        kwds['datatype'] = CommandType(kwds.get('argument', None), kwds.get('result', None))
+        if internally_called:
-        super(Command, self).__init__(ctr, **kwds)
+            inherit = False
+        # make sure either all or no datatype info is in kwds
+        if 'argument' in kwds or 'result' in kwds:
+            datatype = CommandType(kwds.get('argument'), kwds.get('result'))
+        else:
+            datatype = kwds.get('datatype')
+        datainfo = {}
+        datainfo['datatype'] = datatype or CommandType()
+        datainfo['argument'] = datainfo['datatype'].argument
+        datainfo['result'] = datainfo['datatype'].result
+        if datatype:
+            kwds.update(datainfo)
+        if description is not None:
+            kwds['description'] = description
+        if datatype:
+            datainfo = {}
+        super(Command, self).__init__(ctr, **datainfo, **kwds)
+        if inherit:
+            if reorder:
+                kwds['ctr'] = next(object_counter)
+            self.kwds = kwds
 
     @property
     def argument(self):
@@ -295,6 +325,56 @@ class Command(Accessible):
         return self.datatype.result
 
 
+class usercommand(Command):
+    """decorator to turn a method into a command"""
+
+    func = None
+
+    def __init__(self, arg0=False, result=None, inherit=True, *, internally_called=False, **kwds):
+        if result or kwds or isinstance(arg0, DataType) or not callable(arg0):
+            argument = kwds.pop('argument', arg0)  # normal case
+            self.func = None
+            if argument is False and result:
+                argument = None
+            if argument is not False:
+                if isinstance(argument, (tuple, list)):
+                    argument = TupleOf(*argument)
+                kwds['argument'] = argument
+                kwds['result'] = result
+            self.kwds = kwds
+        else:
+            # goodie: allow @usercommand instead of @usercommand()
+            self.func = arg0  # this is the wrapped method!
+            if arg0.__doc__ is not None:
+                kwds['description'] = arg0.__doc__
+            self.name = self.func.__name__
+        super().__init__(kwds.pop('description', ''), inherit=inherit, **kwds)
+
+    def override(self, from_object=None, **kwds):
+        result = super().override(from_object, **kwds)
+        func = kwds.pop('func', from_object.func if from_object else None)
+        if func:
+            result(func)  # pylint: disable=not-callable
+        return result
+
+    def __set_name__(self, owner, name):
+        self.name = name
+
+    def __get__(self, obj, owner=None):
+        if obj is None:
+            return self
+        if not self.func:
+            raise ProgrammingError('usercommand %s not properly configured' % self.name)
+        return self.func.__get__(obj, owner)
+
+    def __call__(self, fun):
+        description = self.kwds.get('description') or fun.__doc__
+        self.properties['description'] = self.kwds['description'] = description
+        self.name = fun.__name__
+        self.func = fun
+        return self
+
+
 # list of predefined accessibles with their type
 PREDEFINED_ACCESSIBLES = dict(
     value = Parameter,

secop/properties.py

@@ -28,6 +28,23 @@ from collections import OrderedDict
 from secop.errors import ProgrammingError, ConfigError, BadValueError
 
 
+def flatten_dict(dictname, itemcls, attrs, remove=True):
+    properties = {}
+    # allow to declare properties directly as class attribute
+    # all these attributes are removed
+    for k, v in attrs.items():
+        if isinstance(v, tuple) and v and isinstance(v[0], itemcls):
+            # this might happen when migrating from old to new style
+            raise ProgrammingError('declared %r with trailing comma' % k)
+        if isinstance(v, itemcls):
+            properties[k] = v
+    if remove:
+        for k in properties:
+            attrs.pop(k)
+    properties.update(attrs.get(dictname, {}))
+    attrs[dictname] = properties
+
+
 # storage for 'properties of a property'
 class Property:
     '''base class holding info about a property
@@ -90,6 +107,7 @@ class PropertyMeta(type):
         if '__constructed__' in attrs:
             return newtype
 
+        flatten_dict('properties', Property, attrs)
         newtype = cls.__join_properties__(newtype, name, bases, attrs)
 
         attrs['__constructed__'] = True
@@ -112,7 +130,7 @@ class PropertyMeta(type):
                 val = self.__class__.properties[pname].default
                 return self.properties.get(pname, val)
 
-            if k in attrs and not isinstance(attrs[k], property):
+            if k in attrs and not isinstance(attrs[k], (property, Property)):
                 if callable(attrs[k]):
                     raise ProgrammingError('%r: property %r collides with method'
                                         % (newtype, k))
@@ -150,7 +168,7 @@ class HasProperties(metaclass=PropertyMeta):
         for pn, po in self.__class__.properties.items():
             if po.export and po.mandatory:
                 if pn not in self.properties:
-                    name = getattr(self, 'name', repr(self))
+                    name = getattr(self, 'name', self.__class__.__name__)
                     raise ConfigError('Property %r of %s needs a value of type %r!' % (pn, name, po.datatype))
                 # apply validator (which may complain further)
                 self.properties[pn] = po.datatype(self.properties[pn])

secop/server.py

@@ -244,6 +244,8 @@ class Server:
         for modname, modobj in self.modules.items():
             modobj.initModule()
 
+        if self._testonly:
+            return
         start_events = []
         for modname, modobj in self.modules.items():
             event = threading.Event()

test/test_modules.py

@@ -28,7 +28,7 @@
 import threading
 from secop.datatypes import BoolType, FloatRange, StringType
 from secop.modules import Communicator, Drivable, Module
-from secop.params import Command, Override, Parameter
+from secop.params import Command, Override, Parameter, usercommand
 from secop.poller import BasicPoller
 
 
@@ -131,6 +131,39 @@ def test_ModuleMeta():
     sortcheck2 = ['status', 'target', 'pollinterval',
                   'param1', 'param2', 'cmd', 'a2', 'cmd2', 'value', 'a1', 'b2']
 
+    # check consistency of new syntax:
+    class Testclass1(Drivable):
+        pollinterval = Parameter(reorder=True)
+        param1 = Parameter('param1', datatype=BoolType(), default=False)
+        param2 = Parameter('param2', datatype=FloatRange(unit='Ohm'), default=True)
+
+        @usercommand(BoolType(), BoolType())
+        def cmd(self, arg):
+            """stuff"""
+            return not arg
+
+        a1 = Parameter('a1', datatype=BoolType(), default=False)
+        a2 = Parameter('a2', datatype=BoolType(), default=True)
+        value = Parameter(datatype=StringType(), default='first')
+
+        @usercommand(BoolType(), BoolType())
+        def cmd2(self, arg):
+            """another stuff"""
+            return not arg
+
+    class Testclass2(Testclass1):
+        cmd2 = Command('another stuff')
+        value = Parameter(datatype=FloatRange(unit='deg'), reorder=True)
+        a1 = Parameter(datatype=FloatRange(unit='$/s'), reorder=True, readonly=False)
+        b2 = Parameter('<b2>', datatype=BoolType(), default=True,
+                       poll=True, readonly=False, initwrite=True)
+
+    for old, new in (Newclass1, Testclass1), (Newclass2, Testclass2):
+        assert len(old.accessibles) == len(new.accessibles)
+        for (oname, oobj), (nname, nobj) in zip(old.accessibles.items(), new.accessibles.items()):
+            assert oname == nname
+            assert oobj.for_export() == nobj.for_export()
+
     logger = LoggerStub()
     updates = {}
     srv = ServerStub(updates)

test/test_params.py

@@ -57,7 +57,7 @@ def test_Parameter():
     assert p1 != p2
     assert p1.ctr != p2.ctr
     with pytest.raises(ProgrammingError):
-        Parameter(None, datatype=float)
+        Parameter(None, datatype=float, inherit=False)
     p3 = p1.copy()
     assert p1.ctr == p3.ctr
     p3.ctr = p1.ctr # manipulate ctr for next line