Allow metadata file to inherit values from another configuration file

Allow metadata file to inherit values from another configuration file

[Wladimir Palant, 2013-11-05 11:33:32]

I've added Sebastian as reviewer. This is an old change but IMHO it would still
help if a second pair of eyes looked over it.

[Sebastian Noack, 2013-11-05 12:19:38]

http://codereview.adblockplus.org/9190147/diff/1/chainedconfigparser.py
File chainedconfigparser.py (right):

http://codereview.adblockplus.org/9190147/diff/1/chainedconfigparser.py#newco...
chainedconfigparser.py:23: class ChainedConfigParser:
I find this approach slightly overkill. I would prefer something as simple as
following instead:


def read_with_inheritance(path):
  files = []
  basedir = os.path.dirname(path)

  try:
    while True:
      file = codecs.open(path, 'rb', encoding='utf-8')
      files.insert(0, file)

      parser = ConfigParser.SafeConfigParser()
      parser.readfp(path)

      try:
        path = parser.get('default', 'inherit')
      except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
        file.close()
        files.remove(file)
        break

      path = os.path.join(basedir, *path.split('/'))

    for file in files:
      file.seek(0)
      parser.readfp(file)
  finally:
    for file in files:
      file.close()

  return parser

[Wladimir Palant, 2013-11-05 12:45:38]

http://codereview.adblockplus.org/9190147/diff/1/chainedconfigparser.py
File chainedconfigparser.py (right):

http://codereview.adblockplus.org/9190147/diff/1/chainedconfigparser.py#newco...
chainedconfigparser.py:23: class ChainedConfigParser:
On 2013/11/05 12:19:38, sebastian wrote:
> I find this approach slightly overkill. I would prefer something as simple as
> following instead:

That was a consideration of course. However, we need to be able to resolve
relative paths in configs - basedir isn't fixed, it's rather dependent on the
config file. Consider the following file foo/metadata:

  [default]
  inherit = bar/metadata
  [mapping]
  icon.png = icon64.png

And foo/bar/metadata (typically inside a subrepository):

  [mapping]
  contentScript.js = default/contentScript.js

Whoever writes foo/bar/metadata file shouldn't need to think about files that
will inherit from it - it's obvious that the relative path
default/contentScript.js here should be resolved relative to the foo/bar/
directory. On the other hand, the relative path icon64.png in foo/metadata file
should be resolved relative to the foo/ directory.

I see now that the change that actually allowed retrieving the source directory
for each setting was part of a different review. I've added this change here but
packager.py is no longer part of the patchset because of that.

[Sebastian Noack, 2013-11-05 13:13:25]

http://codereview.adblockplus.org/9190147/diff/1/chainedconfigparser.py
File chainedconfigparser.py (right):

http://codereview.adblockplus.org/9190147/diff/1/chainedconfigparser.py#newco...
chainedconfigparser.py:23: class ChainedConfigParser:
On 2013/11/05 12:45:38, Wladimir Palant wrote:
> On 2013/11/05 12:19:38, sebastian wrote:
> > I find this approach slightly overkill. I would prefer something as simple
as
> > following instead:
> 
> That was a consideration of course. However, we need to be able to resolve
> relative paths in configs - basedir isn't fixed, it's rather dependent on the
> config file. Consider the following file foo/metadata:
> 
>   [default]
>   inherit = bar/metadata
>   [mapping]
>   icon.png = icon64.png
> 
> And foo/bar/metadata (typically inside a subrepository):
> 
>   [mapping]
>   contentScript.js = default/contentScript.js
> 
> Whoever writes foo/bar/metadata file shouldn't need to think about files that
> will inherit from it - it's obvious that the relative path
> default/contentScript.js here should be resolved relative to the foo/bar/
> directory. On the other hand, the relative path icon64.png in foo/metadata
file
> should be resolved relative to the foo/ directory.
> 
> I see now that the change that actually allowed retrieving the source
directory
> for each setting was part of a different review. I've added this change here
but
> packager.py is no longer part of the patchset because of that.

Fair enough, LGTM.