Better error message

Summary

This very small MR just adds two more verbose error messages for two mistakes that can appear during cfood development.

Please see the in-code comments for details.

Test Environment

There are unit tests.

Check List for the Author

Please, prepare your MR for a review. Be sure to write a summary and a focus and create gitlab comments for the reviewer. They should guide the reviewer through the changes, explain your changes and also point out open questions. For further good practices have a look at our review guidelines

• All automated tests pass
• Reference related issues
• Up-to-date CHANGELOG.md (or not necessary)
• Up-to-date JSON schema (or not necessary)
• Appropriate user and developer documentation (or not necessary)
  • How do I use the software? Assume "stupid" users.
  • How do I develop or debug the software? Assume novice developers.
• Annotations in code (Gitlab comments)
  • Intent of new code
  • Problems with old code
  • Why this implementation?

Check List for the Reviewer

• I understand the intent of this MR
• All automated tests pass
• Up-to-date CHANGELOG.md (or not necessary)
• Appropriate user and developer documentation (or not necessary)
• The test environment setup works and the intended behavior is reproducible in the test environment
• In-code documentation and comments are up-to-date.
• Check: Are there specifications? Are they satisfied?

For further good practices have a look at our review guidelines.

src/caoscrawler/converters/converters.py

@@ -169,64 +169,84 @@ Parameters
 ----------
 
 value: Union[dict, str, list]
-  - If *str*, the value to be interpreted. E.g. "4", "hello" or "$a" etc.
-  - If *dict*, must have keys ``value`` and ``collection_mode``. The returned tuple is directly
-    created from the corresponding values.
-  - If *list*, each element is checked for replacement and the resulting list will be used
-    as (list) value for the property
+  - If *str*, the value to be interpreted. E.g. "4", "hello" or "$a"
+    etc. No unit is set and collection mode is determined from the
+    first character:
+    - '+' corresponds to "list"
+    - '*' corresponds to "multiproperty"
+    - everything else is "single"
+  - If *dict*, it must have a ``value`` key and may ``unit``, and
+    ``collection_mode``. The returned tuple is directly created from
+    the corresponding values if they are given; ``unit`` defaults to
+    None and ``collection_mode`` is determined from ``value`` as
+    explained for the str case above, i.e.,
+    - if it starts with '+', collection mode is "list",
+    - in case of '*', collection mode is "multiproperty",
+    - and everything else is "single".
+  - If *list*, each element is checked for variable replacement and the
+    resulting list will be used as (list) value for the property
 
 Returns
 -------
 
 out: tuple
   - the final value of the property; variable names contained in `values` are replaced.
+  - the final unit of the property; variable names contained in `values` are replaced.
   - the collection mode (can be single, list or multiproperty)
     """
     # @review Florian Spreckelsen 2022-05-13
 
+    propunit = None
+    propvalue = None
+    collection_mode = None
     if isinstance(value, dict):
         if "value" not in value:
             # TODO: how do we handle this case? Just ignore?
             #       or disallow?
             raise NotImplementedError(f"This definition has no \"value\": {value}")
         propvalue = value["value"]
+        if "unit" in value:
+            propunit = replace_variables(value["unit"], values)
         # can be "single", "list" or "multiproperty"
-        collection_mode = value["collection_mode"]
-    elif isinstance(value, str):
-        propvalue = value
-        collection_mode = "single"
-        if propvalue.startswith("+"):
-            collection_mode = "list"
-            propvalue = propvalue[1:]
-        elif propvalue.startswith("*"):
-            collection_mode = "multiproperty"
-            propvalue = propvalue[1:]
-    elif isinstance(value, list):
-        # TODO: (for review)
-        #       This is a bit dirty right now and needed for
-        #       being able to directly set list values. Semantics is, however, a bit
-        #       different from the two cases above.
-        collection_mode = "single"
-
-        # variables replacement:
-        propvalue = list()
-        for element in value:
-            # Do the element-wise replacement only, when its type is string:
-            if isinstance(element, str):
-                propvalue.append(replace_variables(element, values))
-            else:
-                propvalue.append(element)
-
-        return (propvalue, collection_mode)
+        if "collection_mode" in value:
+            collection_mode = value["collection_mode"]
     else:
-        # value is another simple type
-        collection_mode = "single"
         propvalue = value
-        # Return it immediately, otherwise variable substitution would be done and fail:
-        return (propvalue, collection_mode)
+    if collection_mode is None:
+        if isinstance(propvalue, str):
+            # Determine collection mode from string value
+            collection_mode = "single"
+            if propvalue.startswith("+"):
+                collection_mode = "list"
+                propvalue = propvalue[1:]
+            elif propvalue.startswith("*"):
+                collection_mode = "multiproperty"
+                propvalue = propvalue[1:]
+        elif isinstance(propvalue, list):
+            # TODO: (for review)
+            #       This is a bit dirty right now and needed for
+            #       being able to directly set list values. Semantics is, however, a bit
+            #       different from the two cases above.
+            collection_mode = "single"
+
+            # variables replacement:
+            returnvalue = list()
+            for element in propvalue:
+                # Do the element-wise replacement only, when its type is string:
+                if isinstance(element, str):
+                    returnvalue.append(replace_variables(element, values))
+                else:
+                    returnvalue.append(element)
+
+            return (returnvalue, propunit, collection_mode)
+        else:
+            # value is another simple type
+            collection_mode = "single"
+            # Return it immediately, otherwise variable substitution would be done and fail:
+            return (propvalue, propunit, collection_mode)
 
     propvalue = replace_variables(propvalue, values)
-    return (propvalue, collection_mode)
+    return (propvalue, propunit, collection_mode)
 
 
 def create_records(values: GeneralStore, records: RecordStore, def_records: dict):
@@ -280,7 +300,7 @@ def create_records(values: GeneralStore, records: RecordStore, def_records: dict
             key = key_template.safe_substitute(**values.get_storage())
 
             keys_modified.append((name, key))
-            propvalue, collection_mode = handle_value(value, values)
+            propvalue, propunit, collection_mode = handle_value(value, values)
 
             if key.lower() in SPECIAL_PROPERTIES:
                 # e.g. description, name, etc.
@@ -294,17 +314,26 @@ def create_records(values: GeneralStore, records: RecordStore, def_records: dict
             else:
                 if c_record.get_property(key) is None:
                     if collection_mode == "list":
-                        c_record.add_property(name=key, value=[propvalue])
+                        c_record.add_property(name=key, value=[propvalue], unit=propunit)
                     elif (collection_mode == "multiproperty" or
                           collection_mode == "single"):
-                        c_record.add_property(name=key, value=propvalue)
+                        c_record.add_property(name=key, value=propvalue, unit=propunit)
                 else:
                     if collection_mode == "list":
+                        if propunit and c_record.get_property(key).unit and propunit != c_record.get_property(key).unit:
+                            raise RuntimeError(
+                                f"Property '{key}' has contradictory units: "
+                                f"{propunit} and {c_record.get_property(key).unit}"
+                            )
                         c_record.get_property(key).value.append(propvalue)
+                        if propunit and not c_record.get_property(key).unit:
+                            c_record.get_property(key).unit = propunit
                     elif collection_mode == "multiproperty":
-                        c_record.add_property(name=key, value=propvalue)
+                        c_record.add_property(name=key, value=propvalue, unit=propunit)
                     elif collection_mode == "single":
                         c_record.get_property(key).value = propvalue
+                        if propunit:
+                            c_record.get_property(key).unit = propunit
 
         # no matter whether the record existed in the record store or not,
         # parents will be added when they aren't present in the record yet: