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, additional={}):
-        """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 merge it with the optional
-        parameter `additional`.
+        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.
-        * 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": {
+          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):
-            if isinstance(v, list):
-                return [parse_values(x) for x in v]

[Sebastian Noack, 2018/04/18 15:59:00]

Checking for lists and recursively calling this function is unnecessary. The
logic is simpler if you just move it where you check for the newline:

  if '\n' in v:
      ... = [parse_value(x) for x in v.splitlines() if x]
  else:
      ... = parse_value(v)

[tlucas, 2018/04/19 10:01:39]

Done.

-
+        def parse_value(v):
             if v.startswith('number:'):
                 v = v.split(':', 1)[1]
                 try:
-                    v = int(v)

[Sebastian Noack, 2018/04/18 15:58:59]

Nit: I wouldn't reassign v, but just return the parsed values right away. (Then
you can also turn the "elif"s below into "if"s.)

[tlucas, 2018/04/19 10:01:39]

Done.

+                    return int(v)
                 except ValueError:
-                    v = float(v)
-            elif v == 'bool:true':
-                v = True
-            elif v == 'bool:false':
-                v = False
+                    return float(v)
+            if v == 'bool:true':
+                return True
+            if v == 'bool:false':
+                return False
             return v
 
-        def setdefault_recursive(target, keys, value):
-            if len(keys) == 1:
-                target.setdefault(keys[0], parse_values(value))
-            else:
-                current = target.setdefault(keys[0], {})
-                setdefault_recursive(current, keys[1:], value)
-
-        data = self.items(section)
-        result = {}
-
-        for k, v in data:
-            if '\n' in v:
-                v = [x for x in v.splitlines() if x]
-
-            setdefault_recursive(result, k.split('.'), v)

[Sebastian Noack, 2018/04/18 15:59:00]

The recursion here is unnecessary:

  parents = k.split('.')
  tail = parents.pop()
  current = result
  for name in parents:
      current = current.setdefault(name, {})
  current[tail] = ...

[tlucas, 2018/04/19 10:01:39]

Done.

-
-        result.update(additional)

[Sebastian Noack, 2018/04/18 15:58:59]

Well, the point of passing the generated part of the document into this function
was to properly (deep) merge it with the content from the "manifest" section.

So essentially this line should not exist, neither should the line that creates
the new dictionary above (and assigns it to "result"). Instead you should just
add stuff to the dictionary passed to this function.

Here is a realistic example where this would be useful: The browser_action
sub-document is generated like this (copied from the actual build):

  "browser_action": {
    "default_icon": {
      "16": "icons/abp-16.png",
      "19": "icons/abp-19.png",
      "20": "icons/abp-20.png",
      "32": "icons/abp-32.png",
      "38": "icons/abp-38.png",
      "40": "icons/abp-40.png"
    },
    "default_title": "__MSG_name__"
  },

Now somebody wants to use the browser_action.browser_style property and could
just add it to the "manifest" section and it gets added to the generated output.

[tlucas, 2018/04/19 10:01:40]

Done.

-
-        return result
+        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