Issue 6552 - Support arbitrary manifest values

chainedconfigparser.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
 import io
 import ConfigParser
 from StringIO import StringIO
 
 
@@ skipping 139 matching lines @@
 
     def option_source(self, section, option):
         option = self.optionxform(option)
         try:
             return self._origin[(section, option)]
         except KeyError:
             if not self.has_section(section):
                 raise ConfigParser.NoSectionError(section)
             raise ConfigParser.NoOptionError(option, section)
 
-    def section_as_dict(self, section, base):
-        """Parse a given section into a dictionary.
+    def serialize_section_if_present(self, section, base):

[Vasily Kuznetsov, 2018/04/19 14:40:10]

What this method does seems more like parsing than serialization. Perhaps it
should be called "parse_section_if_present".

[Sebastian Noack, 2018/04/19 15:02:59]

In the end it gets serialized (as JSON). But you might be right that what we do
here is parsing it from the config. I guess, its too late now. Though, if
anybody feeling strong about this and submits a Noissue patch, I would take it.

+        """Serialize a given section as a dictionary into `base`.
 
         Parse arbitrary key/value pairs from 'section' of the current
         configuration into a dictionary and deep merge it into `base`.
 
         The following rules need to be considered:
 
         * An option's key may be declared as a series of nested dictionary keys,
           seperated by '.'.
         * Declaring an option's value in a new line (even if only one is given)
-          will define the option's value as an array.

[Sebastian Noack, 2018/04/19 10:50:56]

Nit: Talking about "arrays" made sense initially when we used JSON terminology
here. But since we no longer do, as this function is agnostic of JSON, we also
might want to change "array" to "list" (Python).

[tlucas, 2018/04/19 11:08:13]

Done.

-        * When an option's value is defined as an array, no other nested
+          will define the option's value as a list.
+        * When an option's value is defined as a list, no other nested
           objects may follow.
-        * An array is expandable by the ConfigParser's '+=' token (Note: A
-          previously declared string will be converted into an array).
+        * A list is expandable by the ConfigParser's '+=' token (Note: A
+          previously declared string will be converted into a list).
         * Values may be marked as `number` or `bool` by prefixing them
-          accordingly (this also applies to values in an array):
+          accordingly (this also applies to values in a list):
           * bool:<value>
           * number:<value>
 
         Example:
                                     {
         foo = foo                     "foo": "foo",
         asd =                         "asd": ["asd"],
-            asd                       "bar": {

[Sebastian Noack, 2018/04/19 10:50:57]

Nit: For consistency, we might want to use two space indentation on the
left-hand side here as well.

[tlucas, 2018/04/19 11:08:14]

Done.

+          asd                         "bar": {
         bar.baz = a                     "baz": ["a", "c", "d"]
         baz.foo = a                   },
         baz.z =                       "baz": {
-            bar                         "foo": "a",
-            bool:true           ===>    "z": ["bar", true]
+          bar                           "foo": "a",
+          bool:true             ===>    "z": ["bar", true]
         bar.baz +=                    },
-            c                         "bad": true,
-            d                         "good": false,
+          c                           "bad": true,
+          d                           "good": false,
         bad = bool:true               "is": {
         good = bool:false               "integer": 1,
         is.integer = number:1           "float": 1.4
         is.float = number:1.4         }
                                     }
         """
-        def parse_values(v):

[Sebastian Noack, 2018/04/19 10:50:57]

Nit; since this function is only called with a single value now, we might want
to use singular: "parse_values" => "parse_value"

[tlucas, 2018/04/19 11:08:13]

Done.

-            if isinstance(v, list):
-                return [parse_values(x) for x in v]

[Sebastian Noack, 2018/04/19 10:50:57]

This code path can be removed now, since we no longer this function with a list.

[tlucas, 2018/04/19 11:08:14]

Done.

-
+        def parse_value(v):
             if v.startswith('number:'):
                 v = v.split(':', 1)[1]
                 try:
                     return int(v)
                 except ValueError:
                     return float(v)
             if v == 'bool:true':
                 return True
             if v == 'bool:false':
                 return False
             return v
 
-        data = self.items(section)

[Sebastian Noack, 2018/04/19 10:50:56]

Nit: this variable is only used once, perhaps inline it below?

[tlucas, 2018/04/19 11:08:14]

Done.

One note: Not catching a NoSectionError here would explicitly force [manifest]
to be present in the metadata.*, even if it's empty.

I might check for presence first, what do you think?

[tlucas, 2018/04/19 11:45:15]

Done.

-
-        for k, v in data:
-

[Sebastian Noack, 2018/04/19 10:50:58]

Nit: Is the blank line here intended? It looks weird, I don't recall seeing
blank lines after a loop header anywhere else in our code.

[tlucas, 2018/04/19 11:08:15]

Done.

-            parents = k.split('.')
-            tail = parents.pop()
-            current = base
-            for name in parents:
-                current = base.setdefault(name, {})
-
-            if '\n' in v:
-                current[tail] = [parse_values(x) for x in v.splitlines() if x]
-            else:
-                current[tail] = parse_values(v)
-
-        return base

[Sebastian Noack, 2018/04/19 10:50:57]

Nit: Returning the dictionary seems redundant. It's the same object passed in by
the calling code, hence the calling code already has it. You merely can chain
the function call in a single line, but therefore you have one more line here.
On the other hand, if this function doesn't return it there is no potential
confusion about the object being modified in-place. However, then we might want
to rename this function to add_section_to_dict() or serialize_section().

[tlucas, 2018/04/19 11:08:14]

I wouldn't mind either solution - but we also might want to rename the method,
when we're not returning anything anymore into e.g. merge_section_into().

What's your favorite? 

[Sebastian Noack, 2018/04/19 11:12:41]

That is exactly what I said. My suggestions above were add_section_to_dict() and
serialize_section(); merge_section_into() is good to.

[tlucas, 2018/04/19 11:45:15]

Done.

+        if self.has_section(section):
+            for k, v in self.items(section):
+                parents = k.split('.')
+                tail = parents.pop()
+                current = base
+                for name in parents:
+                    current = base.setdefault(name, {})
+
+                if '\n' in v:
+                    current[tail] = [parse_value(x) for x in v.splitlines() if x]
+                else:
+                    current[tail] = parse_value(v)
 
     def readfp(self, fp, filename=None):
         raise NotImplementedError
 
     def set(self, section, option, value=None):
         raise NotImplementedError
 
     def add_section(self, section):
         raise NotImplementedError
 
     def remove_option(self, section, option):
         raise NotImplementedError
 
     def remove_section(self, section):
         raise NotImplementedError