Improvements and fixes in CIM object tomof() methods

* General changes:
  - Added a new internal scalar_to_mof() function that generates MOF syntax
    for a scalar value.
  - Changed the MOF_INDENT variable from 4 to 3 (the general intentation
    level) for consistency with the formatting of the CIM Schema MOF.
  - Fixed an error in the internal _indent_str() function, which returned a
    minimum of one space even when called with indent=0. It now returns an
    empty string when called with indent=0.
  - The line break after the qualifier list generated by _makequalifiers()
    is now added inside of the function, avoiding the empty line created
    previously with an empty qualifier list.

* CIMClass.tomof():
  - WIP: Converted testcases to py.test and added testcases.

* CIMParameter.tomof():
  - Converted testcases to py.test (from CIMClassMethodToMOF) and added
    testcases.
  - Added a default value of 0 for the 'indent' argument of tomof().

* CIMQualifier.tomof():
  - Added testcases (they did not exist previously).
  - Fixed that qualifier values were generated incorrectly.
    Previously, a half-baked local function valstr() was used, causing for
    example strings or datetime without double quotes in some cases, or "None"
    for "NULL".
    Now, the new scalar_to_mof() function is used.
  - Ensured there is a single space inside of parenthesis or curly braces
    around values.

* CIMQualifierDeclaration.tomof():
  - Added testcases (they did not exist previously).
  - Fixed that default qualifier values were generated incorrectly.
    Previously, atomic_to_cimxml(tocimobj(value)) was used, causing for example
    strings without double quotes.
    Now, the new scalar_to_mof() function is used.
  - Now generating the scope list in a defined order, consistent with
    qualifiers in CIM schema.
  - Now generating a flavor only if it is defined. A value of `None` for the
    corresponding attribute causes the flavor not to be  generated.
  - Added a newline at the end of the generated MOF string (it ended without a
    newline, so far).

Signed-off-by: Andreas Maier <maiera@de.ibm.com>

docs/changes.rst

@@ -179,6 +179,9 @@ Enhancements
   exceptions are surfaced by WBEMConnection methods as a
   pywbem.ConnectionError (issue #916).
 
+* In the `tomof()` methods of the CIM object classes, changed the formatting
+  of the generated MOF to be more consistent with the CIM Schema MOF.
+
 * Docs: Editorial improvements in the documentation (links, typos, formatting).
 
 * Docs: Clarifications and small fixes in the documentation of the

pywbem/cim_obj.py

@@ -78,7 +78,8 @@
 from .config import DEBUG_WARNING_ORIGIN
 from .cim_types import _CIMComparisonMixin, type_from_name, cimtype, \
     atomic_to_cim_xml, CIMType, CIMDateTime, Uint8, Sint8, Uint16, Sint16, \
-    Uint32, Sint32, Uint64, Sint64, Real32, Real64, number_types
+    Uint32, Sint32, Uint64, Sint64, Real32, Real64, number_types, CIMInt, \
+    CIMFloat, _Longint
 
 if six.PY2:
     # pylint: disable=wrong-import-order
@@ -93,7 +94,7 @@
            'cimvalue']
 
 # Constants for MOF formatting output
-MOF_INDENT = 4
+MOF_INDENT = 3
 MAX_MOF_LINE = 79   # use 79 because comma separator sometimes runs over
 
 
@@ -561,22 +562,24 @@ def _makequalifiers(qualifiers, indent):
 
       qualifiers (list): List of qualifiers to format.
 
-      indent (:term:`integer`): Indent level for this set of qualifiers.
+      indent (:term:`integer`): Indent level for this set of qualifiers. This
+        is the number of spaces to the opening square bracket of the list.
     """
     if not qualifiers:
         return ''
-    qual_list = [qualifiers[qn].tomof(indent + 2)
+    qual_list = [qualifiers[qn].tomof(indent + 1 + MOF_INDENT)
                  for qn in sorted(qualifiers.keys())]
-    qual_str = ',\n '.ljust(indent + 2).join(qual_list)
-    return '%s[%s]' % (_indent_str(indent), qual_str)
+    delim = ',\n' + _indent_str(indent + 1)
+    qual_str = delim.join(qual_list)
+    return '%s[%s]\n' % (_indent_str(indent), qual_str)
 
 
 def _indent_str(indent):
     """
     Return a MOF indent pad string from the indent integer variable
     that defines number of spaces to indent. Used to format MOF output
     """
-    return ' '.ljust(indent, ' ')
+    return ''.ljust(indent, ' ')
 
 
 def mofstr(strvalue, indent=MOF_INDENT, maxline=MAX_MOF_LINE):
@@ -734,6 +737,62 @@ def moftype(cim_type, refclass):
     return (refclass + ' REF') if cim_type == 'reference' else cim_type
 
 
+def scalar_to_mof(value, type, indent=0):
+    # pylint: disable=line-too-long,redefined-builtin
+    """
+    Convert an "atomic" scalar value to a MOF string and return that string.
+
+    Parameters:
+
+      value (:term:`CIM data type`, :term:`number`, :class:`~pywbem.CIMInstance`, :class:`~pywbem.CIMClass`):
+        The "atomic" input value. May be `None`.
+
+        Must not be an array/list/tuple. Must not be a :ref:`CIM object` other
+        than those listed.
+
+      type (string): CIM data type name.
+
+      indent (:term:`integer`): Number of spaces to indent any spilled over
+        strings (i.e. not used for the first line of a string, and not
+        for any other types).
+
+    Returns:
+
+        A :term:`unicode string` in MOF format representing the input value.
+        `"NULL"`, if the input value is `None`.
+    """  # noqa: E501
+    if value is None:
+        return u'NULL'
+    elif type == 'string':
+        if isinstance(value, six.string_types):
+            return mofstr(value, indent=indent)
+        elif isinstance(value, CIMInstance):
+            # embedded instance
+            return value.tomof()
+        elif isinstance(value, CIMClass):
+            # embedded class
+            return value.tomof()
+        else:
+            raise ValueError("Value %r has invalid type %s for a CIM string "
+                             "type" % (value, type(value)))
+    elif type == 'char16':
+        return mofstr(value, indent=indent)
+        # TODO 01/18 AM Return char16 with single quotes
+    elif type == 'boolean':
+        return u'true' if value else u'false'
+    elif type == 'datetime':
+        return u'"%s"' % value
+    elif type == 'reference':
+        return u'"%s"' % value.to_wbem_uri()
+    elif isinstance(value, (CIMInt, int, _Longint)):
+        return six.text_type(value)
+    elif isinstance(value, (CIMFloat, float)):
+        return six.text_type(value)
+    else:
+        raise TypeError("Value %r has invalid type %s for conversion to a "
+                        "MOF string" % (value, type(value)))
+
+
 def _stacklevel_above_module(mod_name):
     """
     Return the stack level (with 1 = caller of this function) of the first
@@ -2774,8 +2833,7 @@ class definition represented by the :class:`~pywbem.CIMClass`
         indent = MOF_INDENT
 
         # Qualifiers definition or empty line
-        ret_str = '%s\n' % (_makequalifiers(self.qualifiers,
-                                            indent))
+        ret_str = _makequalifiers(self.qualifiers, indent)
 
         ret_str += 'class %s ' % self.classname
 
@@ -3494,8 +3552,7 @@ class MOF.
 
             mof = '\n'
             if self.qualifiers:
-                mof += '%s\n' % ((_makequalifiers(self.qualifiers,
-                                                  (indent + MOF_INDENT))))
+                mof += _makequalifiers(self.qualifiers, indent + MOF_INDENT)
 
             mof += '%s%s %s%s' % (_indent_str(indent),
                                   moftype(self.type,
@@ -3963,8 +4020,7 @@ def tomof(self, indent):
         ret_str = ''
 
         if self.qualifiers:
-            ret_str += '%s\n' % (_makequalifiers(self.qualifiers,
-                                                 (indent + MOF_INDENT)))
+            ret_str += _makequalifiers(self.qualifiers, indent + MOF_INDENT)
 
         ret_str += _indent_str(indent)
 
@@ -4403,7 +4459,7 @@ def tocimxmlstr(self, indent=None):
         """
         return tocimxmlstr(self, indent)
 
-    def tomof(self, indent):
+    def tomof(self, indent=0):
         """
         Return a :term:`unicode string` that is a MOF fragment with the
         parameter definition represented by the :class:`~pywbem.CIMParameter`
@@ -4413,22 +4469,23 @@ def tomof(self, indent):
             indent (:term:`integer`): Number of spaces to indent each parameter
         """
 
+        ret = []
+
+        if self.qualifiers:
+            ret.append(_makequalifiers(self.qualifiers, indent + MOF_INDENT))
+
+        ret.append(_indent_str(indent))
+        ret.append(moftype(self.type, self.reference_class))
+        ret.append(' ')
+        ret.append(self.name)
+
         if self.is_array:
+            ret.append('[')
             if self.array_size is not None:
-                array_str = "[%s]" % self.array_size
-            else:
-                array_str = "[]"
-        else:
-            array_str = ''
+                ret.append(str(self.array_size))
+            ret.append(']')
 
-        rtn_str = ''
-        if self.qualifiers:
-            rtn_str = '%s\n' % (_makequalifiers(self.qualifiers,
-                                                indent + 2))
-        rtn_str += '%s%s %s%s' % (_indent_str(indent),
-                                  moftype(self.type, self.reference_class),
-                                  self.name, array_str)
-        return rtn_str
+        return u''.join(ret)
 
 
 # pylint: disable=too-many-instance-attributes
@@ -4889,27 +4946,28 @@ def tomof(self, indent=MOF_INDENT):
         qualifier value represented by the :class:`~pywbem.CIMQualifier`
         object.
 
+        The items of array values are tried to keep on the same line. If the
+        generated line would exceed the maximum MOF line length, the value is
+        split into multiple lines, on array item boundaries, and/or within long
+        strings on word boundaries.
+
+        If a string value (of a scalar value, or of an array item) is split
+        into multiple lines, the first line of the value is put onto a line on
+        its own.
+
         Parameters:
-            indent (:term:`integer`): Number of spaces to indent the second and
-              subsequent lines of a multi-line result. The first line is not
-              indented.
+            indent (:term:`integer`): For a multi-line result, the number of
+              spaces to indent each line except the first line (on which the
+              qualifier name appears).
         """
 
-        def valstr(value):
-            """
-            Return a string that is the MOF literal representing a value.
-            """
-            if isinstance(value, six.string_types):
-                return mofstr(value, indent)
-            return str(value)
-
         if isinstance(self.value, list):
             line_pos = indent + len(self.name) + 4
             values = ''
             for i, val in enumerate(self.value):
                 if i != 0:
                     values += ','
-                nextval = valstr(val)
+                nextval = scalar_to_mof(val, self.type, indent)
                 if (line_pos + len(nextval) + 3) > MAX_MOF_LINE:
                     sep = '\n' + _indent_str(indent)
                     line_pos = len(_indent_str(indent)) + 4
@@ -4919,15 +4977,16 @@ def valstr(value):
                 line_pos += (len(nextval) + 2)
                 values += sep + nextval
 
-            mof = '%s {%s%s' % (self.name, values, '}')
+            mof = u'%s {%s%s' % (self.name, values, ' }')
 
         else:
-            val = valstr(self.value)
-            if len(val) + indent + 4 >= MAX_MOF_LINE:
-                mof = '%s (\n%s%s)' % (self.name, _indent_str(indent),
-                                       val)
+            val = scalar_to_mof(self.value, self.type, indent)
+            # If more than one line, spill the first as well
+            if '\n' in val:
+                mof = u'%s (\n%s%s )' % (self.name, _indent_str(indent), val)
             else:
-                mof = '%s (%s)' % (self.name, val)
+                mof = u'%s ( %s )' % (self.name, val)
+
         return mof
 
 
@@ -4972,6 +5031,11 @@ class CIMQualifierDeclaration(_CIMComparisonMixin):
     default values.
     """
 
+    # Order of scopes when externalizing the qualifier declaration
+    ordered_scopes = ["CLASS", "ASSOCIATION", "INDICATION",
+                      "PROPERTY", "REFERENCE", "METHOD", "PARAMETER",
+                      "ANY"]
+
     # pylint: disable=too-many-arguments
     def __init__(self, name, type, value=None, is_array=False,
                  array_size=None, scopes=None,
@@ -5223,8 +5287,8 @@ def scopes(self):
           qualifier has that scope (i.e. can be applied to a CIM element of
           that kind).
 
-        Valid scope names are "CLASS", "ASSOCIATION", "REFERENCE",
-        "PROPERTY", "METHOD", "PARAMETER", "INDICATION", and "ANY".
+        Valid scope names are "CLASS", "ASSOCIATION", "INDICATION",
+        "PROPERTY", "REFERENCE", "METHOD", "PARAMETER", and "ANY".
 
         Will not be `None`.
 
@@ -5450,36 +5514,55 @@ def tomof(self):
         :class:`~pywbem.CIMQualifierDeclaration` object.
         """
         mof = 'Qualifier %s : %s' % (self.name, self.type)
+
         if self.is_array:
             mof += '['
             if self.array_size is not None:
                 mof += str(self.array_size)
             mof += ']'
+
         if self.value is not None:
             if isinstance(self.value, list):
-                mof += ' = {'
-                mof += ', '.join([atomic_to_cim_xml(
-                    tocimobj(self.type, x)) for x in self.value])
-                mof += '}'
+                mof += ' = { '
+                mof_values = []
+                for v in self.value:
+                    mof_values.append(scalar_to_mof(v, self.type))
+                    # TODO 01/18 AM Add support for right margin in MOF
+                mof += ', '.join(mof_values)
+                mof += ' }'
             else:
-                mof += ' = %s' % atomic_to_cim_xml(
-                    tocimobj(self.type, self.value))
+                mof += ' = %s' % scalar_to_mof(self.value, self.type)
+
+        mof += ',\n%sScope(' % _indent_str(MOF_INDENT + 1)
+        mof_scopes = []
+        for scope in self.ordered_scopes:
+            if self.scopes.get(scope, False):
+                mof_scopes.append(scope.lower())
+        mof += ', '.join(mof_scopes)
+        mof += ')'
 
-        mof += ',\n%sScope(' % _indent_str(MOF_INDENT)
-        mof += ', '.join([x.lower() for x, y in self.scopes.items() if y]) + ')'
         # toinstance flavor not included here because not part of DSP0004
-        if not self.overridable and not self.tosubclass \
-                and not self.translatable:
-            mof += ';'
-            return mof
-
-        mof += ',\n%sFlavor(' % _indent_str(MOF_INDENT)
-        mof += self.overridable and 'EnableOverride' or 'DisableOverride'
-        mof += ', '
-        mof += self.tosubclass and 'ToSubclass' or 'Restricted'
+        mof_flavors = []
+
+        if self.overridable is True:
+            mof_flavors.append('EnableOverride')
+        elif self.overridable is False:
+            mof_flavors.append('DisableOverride')
+
+        if self.tosubclass is True:
+            mof_flavors.append('ToSubclass')
+        elif self.tosubclass is False:
+            mof_flavors.append('Restricted')
+
         if self.translatable:
-            mof += ', Translatable'
-        mof += ');'
+            mof_flavors.append('Translatable')
+
+        if mof_flavors:
+            mof += ',\n%sFlavor(' % _indent_str(MOF_INDENT + 1)
+            mof += ', '.join(mof_flavors)
+            mof += ')'
+
+        mof += ';\n'
         return mof