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 75 matching lines @@
         is_addition = option.endswith('+')
         is_diff = is_addition or option.endswith('-')
 
         if is_diff:
             option = option[:-1].rstrip()
             try:
                 orig_value = self.get(section, option)
             except ConfigParser.NoOptionError:
                 raise DiffForUnknownOptionError(option, section)
 
-            orig_values = orig_value.split()
-            diff_values = value.split()
+            orig_values = orig_value.splitlines()
+            diff_values = value.splitlines()
 
             if is_addition:
                 new_values = orig_values + [v for v in diff_values if v not in orig_values]
             else:
                 new_values = [v for v in orig_values if v not in diff_values]
 
-            value = ' '.join(new_values)
+            value = '\n'.join(new_values)
 
         return is_diff, option, value
 
     def _process_parsers(self, parsers):
         for parser, filename in parsers:
             for section in parser.sections():
                 if not self.has_section(section):
                     try:
                         ConfigParser.SafeConfigParser.add_section(self, section)
                     except ValueError:
@@ skipping 35 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 as_json_object(self, section):
-        r"""Parse a given section into a JSON object.
+    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 nested JSON object.
+        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 '.'.
-        * An option's key must end with '[]', to mark this option as an array
-        * When an option is marked as an array, no other nested objects may
-          follow
-        * An array is expandable by the ConfigParser's '+=' token.
-        * Values may be marked as special types by appending '\{code}':
-          * \b - boolean
-          * \i - integer
-          * \f - float
+        * Declaring an option's value in a new line (even if only one is given)
+          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.
+        * 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 a list):
+          * bool:<value>
+          * number:<value>
 
         Example:
-
-                                        {
-                                          "good": false,
-                                          "bar": {
-                                            "baz": ["bar", "bazinga"]
-        foo = foo                         },
-        bar.baz[] = bar                   "is": {
-        baz.foo = a                         "integer": 1,
-        baz.z[] = b                         "float": 1.4
-        bar.baz[] += bazinga    ==>       },
-        bad = true\b                      "baz": {
-        good = f\b                          "foo": "a",
-        is.integer = 1\i                    "z": ["b"]
-        is.float = 1.4\f                  },
-                                          "bad": true
-                                          "foo": "foo"
-                                        }
+                                    {
+        foo = foo                     "foo": "foo",
+        asd =                         "asd": ["asd"],
+          asd                         "bar": {
+        bar.baz = a                     "baz": ["a", "c", "d"]
+        baz.foo = a                   },
+        baz.z =                       "baz": {
+          bar                           "foo": "a",
+          bool:true             ===>    "z": ["bar", true]
+        bar.baz +=                    },
+          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):
-            import distutils
-            if isinstance(v, list):
-                return [parse_values(x) for x in v]
-
-            mapping = {
-                '\\b': lambda x: bool(distutils.util.strtobool(x)),
-                '\\i': int,
-                '\\f': float,
-            }
-            if '\\' in v:
-                return mapping[v[-2:]](v[:-2])
+        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

[Sebastian Noack, 2018/04/12 12:19:23]

* I cannot think of any relevant manifest key where "true", "false", etc. or
anything that parses as a number would be to be interpreted as a string. So I
would go with more implicit semantics in favor of less obscure syntax.
* In particular the distinction between integer and float is superfluous since
all numbers in (JavaScript and) JSON are treated as double-precision floating
point numbers. However, Python's "json" module is not compliant with the JSON
standard as it treat numbers without floating point character as integers.
* There are typed ConfigParser.get*() methods.

So how about this:

  try:
    self.getint(...)
  except ValueError:
      try:
          self.getfloat(...)
      except ValueError:
          try:
              self.getboolean(...)
          except ValueError:
              self.get(...)

[tlucas, 2018/04/12 13:47:39]

I do not fully oppose your thoughts, but what about the following examples:

 * an extension renders it's version via this machinery into the manifest.json,
where the version only consists of 2 digits (e.g. 1.3) -> the version would get
returned as a float, while at least Chrome expects this value to be a string
(FWIW, this currently is the case for our "compat"-key, which also might endup
in [manifest])
 * Arrays can not be handled by the typed get*() methods, so all arrays would be
restricted to contain strings only

If we agree that those are justifiable restrictions, i will change the code
accordingly - however, i'm currently thinking that those restrictions are not
necessary.

[Sebastian Noack, 2018/04/12 14:39:29]

I was thinking about the version number, but its not meant to be set through the
"manifest" section as it needs some processing (for the build number in the
development builds). Similar for the version numbers in the "compat" section.
But I might see a case for a more explicit format, if you are still convinced it
makes sense. However, I'm not too happy with the particular format you chose:

* Appending \X to the end of the value is rather obscure. When I first looked at
the test configuration (before I looked at the implementation), I was sure that
this was a typo. :D
* Still I don't think it makes sense to distinguish between int and float. Maybe
we don't even have to distinguish between numbers and bool (dependent on the
format we go with).

So (if we go with a strictly typed format), here are two suggestions for an IMO
more appropriate annotation format:

  foo = number:42
  foo = number:4.2
  foo = bool:true

  foo# = 42
  foo# = 4.2
  foo# = true 
  foo[#] =
    42
    24
Ah right, never mind then. I didn't consider that we cannot use it for nested
values. Still I'm not sure if using distutils here is appropriate. Simply
supporting "true" and "false" (no variations) should be good enough anyway.

[tlucas, 2018/04/13 09:14:59]

Done.

 
-        def setdefault_recursive(target, arr):
-            if len(arr) == 2:
-                target.setdefault(arr[0], parse_values(arr[1]))
-            else:
-                current = target.setdefault(arr[0], {})

[Sebastian Noack, 2018/04/12 12:19:23]

You might want to use an OrderedDict to make sure the keys remain in the same
order as in the JSON file, to preserve the readability of the generated
mainfest.json, but also to make sure that the expected output for the tests
don't depend on the behavior of Python internals.

[tlucas, 2018/04/12 13:47:38]

Good point, done.

-                setdefault_recursive(current, arr[1:])
-
-        data = self.items(section)
-        result = {}

[Sebastian Noack, 2018/04/12 12:19:23]

Same here.

[tlucas, 2018/04/12 13:47:38]

See above, done.

-
-        for k, v in data:
-            is_array = k.endswith('[]')
-
-            sub_keys = (k if not is_array else k[:-2]).split('.')
-            value = v.split() if is_array else v

[Sebastian Noack, 2018/04/12 12:19:23]

It seems the logic here can be simplified:

  if k.endswith('[]'):
      k = k[:-2]
      v = v.split()

  setdefault_recursive(result, k.split('.') + [value])

[tlucas, 2018/04/12 13:47:39]

Done.

-
-            setdefault_recursive(result, sub_keys + [value])
-
-        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