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 as_json_object(self, section):
+        r"""Parse a given section into a JSON object.
+
+        Parse arbitrary key/value pairs from 'section' of the current
+        configuration into a nested JSON object.
+
+        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
+
+        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"
+                                        }
+        """
+        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])
+            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
+
     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