fix handling of StructOf datatype

- change secop.client.SecopClient to use native types instead of
  strings for its setParameter and execCommand methods.
- secop-gui: for now, setParameter accept strings for complex types.
  this should be changed to use native type in an other change
- fix bugs in parser.py

+ SecopClient: make visible in an error message that the error
  was generated on the SEC node
+ fix a bug when a command is called with 0 as argument

Change-Id: Id87d4678311ef8cf43a25153254d36127e16c6d9
Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/23299
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/client/__init__.py

@@ -520,7 +520,7 @@ class SecopClient(ProxyClient):
         action, _, data = entry[2]  # pylint: disable=unpacking-non-sequence
         if action.startswith(ERRORPREFIX):
             errcls = self.error_map(data[0])
-            raise errcls(data[1])
+            raise errcls('on SEC-Node: ' + data[1])
         return entry[2]  # reply
 
     def request(self, action, ident=None, data=None):
@@ -552,7 +552,7 @@ class SecopClient(ProxyClient):
     def setParameter(self, module, parameter, value):
         self.connect()  # make sure we are connected
         datatype = self.modules[module]['parameters'][parameter]['datatype']
-        value = datatype.export_value(datatype.from_string(value))
+        value = datatype.export_value(value)
         self.request(WRITEREQUEST, self.identifier[module, parameter], value)
         return self.cache[module, parameter]
 
@@ -560,7 +560,7 @@ class SecopClient(ProxyClient):
         self.connect()  # make sure we are connected
         datatype = self.modules[module]['commands'][command]['datatype'].argument
         if datatype:
-            argument = datatype.export_value(datatype.from_string(argument))
+            argument = datatype.export_value(argument)
         else:
             if argument is not None:
                 raise secop.errors.BadValueError('command has no argument')

secop/datatypes.py

@@ -824,9 +824,6 @@ class StructOf(DataType):
             if not isinstance(subtype, DataType):
                 raise ProgrammingError(
                     'StructOf only works with named DataType objs as keyworded arguments!')
-            if not isinstance(name, str):
-                raise ProgrammingError(
-                    'StructOf only works with named DataType objs as keyworded arguments!')
         for name in self.optional:
             if name not in members:
                 raise ProgrammingError(
@@ -850,16 +847,11 @@ class StructOf(DataType):
             ['%s=%s' % (n, repr(st)) for n, st in list(self.members.items())]), opt)
 
     def __call__(self, value):
-        """return the validated value or raise
+        """return the validated value or raise"""
-
-        in principle, we should make it some sort of frozen dict
-        """
         try:
-            # XXX: handle optional elements !!!
+            missing = set(self.members) - set(value) - set(self.optional)
-            if len(list(value.keys())) != len(list(self.members.keys())):
+            if missing:
-                raise BadValueError(
+                raise BadValueError('missing values for keys %r' % list(missing))
-                    'Illegal number of Arguments! Need %d arguments.' %
-                        len(list(self.members.keys())))
             # validate elements and return as dict
             return ImmutableDict((str(k), self.members[k](v))
                                  for k, v in list(value.items()))
@@ -868,21 +860,13 @@ class StructOf(DataType):
 
     def export_value(self, value):
         """returns a python object fit for serialisation"""
-        if len(list(value.keys())) != len(list(self.members.keys())):
+        self(value)  # check validity
-            raise BadValueError(
-                'Illegal number of Arguments! Need %d arguments.' % len(
-                    list(self.members.keys())))
         return dict((str(k), self.members[k].export_value(v))
                     for k, v in list(value.items()))
 
     def import_value(self, value):
         """returns a python object from serialisation"""
-        if len(list(value.keys())) != len(list(self.members.keys())):
+        return self({str(k): self.members[k].import_value(v) for k, v in value.items()})
-            raise BadValueError(
-                'Illegal number of Arguments! Need %d arguments.' % len(
-                    list(self.members.keys())))
-        return ImmutableDict((str(k), self.members[k].import_value(v))
-                             for k, v in list(value.items()))
 
     def from_string(self, text):
         value, rem = Parser.parse(text)
@@ -1143,5 +1127,4 @@ def get_datatype(json, pname=''):
     try:
         return DATATYPES[base](pname=pname, **kwargs)
     except Exception as e:
-        print(base, kwargs, repr(e))
         raise BadValueError('invalid data descriptor: %r (%s)' % (json, str(e)))

secop/gui/mainwindow.py

@@ -75,14 +75,16 @@ class QSECNode(QObject):
         return self.modules[module]['parameters'][parameter]
 
     def setParameter(self, module, parameter, value):
-        self.conn.setParameter(module, parameter, value)
+        # TODO: change the widgets for complex types to no longer use strings
+        datatype = self.conn.modules[module]['parameters'][parameter]['datatype']
+        self.conn.setParameter(module, parameter, datatype.from_string(value))
 
     def getParameter(self, module, parameter):
         return self.conn.getParameter(module, parameter, True)
 
-    def execCommand(self, module, command, arg):
+    def execCommand(self, module, command, argument):
         try:
-            return self.conn.execCommand(module, command, arg)
+            return self.conn.execCommand(module, command, argument)
         except Exception as e:
             return 'ERROR: %r' % e, {}
 

secop/parse.py

@@ -21,7 +21,7 @@
 # *****************************************************************************
 """parser. used for config files and the gui
 
-can't use ast.literal_eval as it can't handle enums :(
+can't use ast.literal_eval as we want less strict syntax (strings without quotes)
 
 parsing rules:
 (...) -> tuple
@@ -34,9 +34,13 @@ text -> string
 'text' -> string
 "text" -> string
 
-further convertions are done by the validator of the datatype....
+further conversions are done by the validator of the datatype....
 """
 
+# TODO: should be refactored to use Exceptions instead of None in return tuple
+#       also it would be better to use functions instead of a class
+
+
 from collections import OrderedDict
 
 
@@ -80,7 +84,7 @@ class Parser:
                 # check escapes!
                 if text[idx - 1] == '\\':
                     continue
-                return text[1:idx], text[idx + 1:]
+                return text[1:idx], text[idx + 1:].strip()
 
         # unquoted strings are terminated by comma or whitespace
         idx = 0
@@ -88,7 +92,7 @@ class Parser:
             if text[idx] in '\x09 ,.;:()[]{}<>-+*/\\!"§$%&=?#~+*\'´`^°|-':
                 break
             idx += 1
-        return text[:idx] or None, text[idx:]
+        return text[:idx] or None, text[idx:].strip()
 
     def parse_tuple(self, orgtext):
         text = orgtext.strip()
@@ -98,17 +102,21 @@ class Parser:
         # convert to closing bracket
         bra = ')]>'['([<'.index(bra)]
         reslist = []
-        # search for cosing bracket, collecting results
+        # search for closing bracket, collecting results
         text = text[1:]
         while text:
             if bra not in text:
                 return None, text
             res, rem = self.parse_sub(text)
             if res is None:
+                print('remtuple %r %r %r' % (rem, text, bra))
+                if rem[0] == bra:
+                    # allow trailing separator
+                    return tuple(reslist), rem[1:].strip()
                 return None, text
             reslist.append(res)
             if rem[0] == bra:
-                return tuple(reslist), rem[1:]
+                return tuple(reslist), rem[1:].strip()
             # eat separator
             if rem[0] in ',;':
                 text = rem[1:]
@@ -122,24 +130,27 @@ class Parser:
             return None, orgtext
         # keep ordering
         result = OrderedDict()
-        # search for cosing bracket, collecting results
+        # search for closing bracket, collecting results
         # watch for key=value or key:value pairs, separated by ,
         text = text[1:]
         while '}' in text:
             # first part is always a string
             key, rem = self.parse_string(text)
-            if not key:
+            if key is None:
+                if rem[0] == '}':
+                    # allow trailing separator
+                    return result, rem[1:].strip()
                 return None, orgtext
             if rem[0] not in ':=':
                 return None, rem
             # eat separator
             text = rem[1:]
             value, rem = self.parse_sub(text)
-            if not value:
+            if value is None:
                 return None, orgtext
             result[key] = value
             if rem[0] == '}':
-                return result, rem[1:]
+                return result, rem[1:].strip()
 
             if rem[0] not in ',;':
                 return None, rem

secop/protocol/dispatcher.py

@@ -212,7 +212,7 @@ class Dispatcher:
         # now call func
         # note: exceptions are handled in handle_request, not here!
         func = getattr(moduleobj, 'do_' + cmdname)
-        res = func(argument) if argument else func()
+        res = func() if argument is None else func(argument)
 
         # pipe through cmdspec.datatype.result
         if cmdspec.datatype.result:

test/test_parse.py

@@ -23,6 +23,7 @@
 
 
 from collections import OrderedDict
+from ast import literal_eval
 
 import pytest
 
@@ -46,7 +47,7 @@ def test_Parser_parse_number(parser):
 def test_Parser_parse_string(parser):
     assert parser.parse_string('%') == (None, '%')
     assert parser.parse_string('a') == ('a', '')
-    assert parser.parse_string('Hello World!') == ('Hello', ' World!')
+    assert parser.parse_string('Hello World!') == ('Hello', 'World!')
     assert parser.parse_string('Hello<World!') == ('Hello', '<World!')
     assert parser.parse_string('"Hello World!\'') == (None, '"Hello World!\'')
     assert parser.parse_string('"Hello World!\\"') == (
@@ -75,7 +76,7 @@ def test_Parser_parse(parser):
     assert parser.parse('1.23e-04:9') == (1.23e-4, ':9')
 
     assert parser.parse('%') == (None, '%')
-    assert parser.parse('Hello World!') == ('Hello', ' World!')
+    assert parser.parse('Hello World!') == ('Hello', 'World!')
     assert parser.parse('Hello<World!') == ('Hello', '<World!')
     assert parser.parse('"Hello World!\'') == (None, '"Hello World!\'')
     assert parser.parse('"Hello World!\\"') == (None, '"Hello World!\\"')
@@ -93,3 +94,18 @@ def test_Parser_parse(parser):
     assert parser.parse('1, 2,a,c') == ((1, 2, 'a', 'c'), '')
 
     assert parser.parse('"\x09 \r"') == ('\t \r', '')
+
+
+# valid python syntax must always be valid
+@pytest.mark.parametrize('string', [
+        "{'a': 0, 'b': 1}",
+        "(1,)",
+        "{'a': 'x' , 'b': 1}",
+        "{'a': 'x' , 'b': 1,}",
+        "{'a':['b' ] ,}",
+        "(1,[1] ,)",
+    ])
+def test_python_literal(parser, string):
+    literal_eval(string)  # prove str is valid
+    _, remaining = parser.parse(string)
+    assert remaining == ''