Code review comment for lp:~barry/lazr.config/bug-310619

> === modified file 'src/lazr/config/README.txt'
> --- src/lazr/config/README.txt 2008-12-15 22:22:12 +0000
> +++ src/lazr/config/README.txt 2008-12-23 15:24:02 +0000
> @@ -368,6 +368,23 @@
> # Accept the default values for the optional section-5.
> [section-5]
>
> +If the schema defines .master sections, then the conf file can contain
> +sections that extend the .master section. These are like categories with
> +templates except that the section names extending .master need not be named in
> +the schema file.

I think we should explain why there is a master.

{{

The .master section allows admins to define configurations for an
an arbitrary number of processes.

}}

> +
> + >>> master_schema_conf = path.join(testfiles_dir, 'master.conf')
> + >>> master_local_conf = path.join(testfiles_dir, 'master-local.conf')
> + >>> master_schema = ConfigSchema(master_schema_conf)
> + >>> len(master_schema.getByCategory('thing'))
> + 0
> + >>> master_conf = master_schema.load(master_local_conf)
> + >>> sections = master_conf.getByCategory('thing')
> + >>> sorted(section.name for section in sections)
> + ['thing.one', 'thing.two']
> + >>> sorted(section.foo for section in sections)
> + ['1', '2']
> +

Does this work?

    >>> master_conf.thing.one.name
    'thing.one'

...

=== modified file 'src/lazr/config/__init__.py'
--- src/lazr/config/__init__.py 2008-12-15 22:22:12 +0000
+++ src/lazr/config/__init__.py 2008-12-23 15:24:02 +0000
...

> @@ -87,9 +88,15 @@
> else:
> return (None, self.name)
>
> + def clone(self):
> + """Return a copy of this section schema."""
> + return self.__class__(self.name, self._options.copy(),
> + self.optional, self.master)
> +

This looks familiar

/me looks.

Oops! I see a nuance. This clone is fine. The clone I was thinking
of still has James H's hack for mutable sections. They are no mutable,
so the comment and sanity check can be removed.

...

> @@ -288,6 +296,7 @@
> name_parts = name.split('.')
> is_template = name_parts[-1] == 'template'
> is_optional = name_parts[-1] == 'optional'
> + is_master = name_parts[-1] == 'master'

In my design, .optional and .template were mutually exclusive. I believe
this is true with .master.

Your design implies that the schema may define a section from the master.
I think this is practical, given that the application may require at least
one section defined for a process, but allow for additional processes to
be defined in a config.

How does a section from a .master category interact with .optional? May
sections defined from a .master in a schema be optional? There
are no tests for this. I believe this is legitimate in a schema

{{{

[whatsit.master]
key: default

[whatsit.always]

[whatsit.sometimes.optional]

}}}

...

> @@ -344,6 +354,8 @@
> section_schemas = []
> for key in self._section_schemas:
> section = self._section_schemas[key]
> + if section.master:
> + continue

I don't understand this. I expect .master sections to behave like .template
sections.

> category, dummy = section.category_and_section_names
> if name == category:
> section_schemas.append(section)

...

> @@ -561,15 +573,34 @@
> errors = list(self.data._errors)
> errors.extend(encoding_errors)
> extends = None
> + masters = set()
> for section_name in parser.sections():
> if section_name == 'meta':
> extends, meta_errors = self._loadMetaData(parser)
> errors.extend(meta_errors)
> continue
> - if (section_name.endswith('.template')
> - or section_name.endswith('.optional')):
> + if (section_name.endswith('.template') or
> + section_name.endswith('.optional')):
> # This section is a schema directive.
> continue

I have long wondered if this was bad. I now think it is better for
and error to be collected when a config contains schema instructions.

Regardless, of how we treat this error, .master should also be in the
condition--defining a .master outside of the schema is insane because
the application code must understand the primal definition.

> + # Check for sections which extend .masters.
> + if '.' in section_name:
> + category, section = section_name.split('.')
> + master_name = category + '.master'
> + try:
> + section_schema = self.schema[master_name]
> + except NoSectionError:
> + # There's no master for this section.
> + pass

This comment is incomplete. The section must be defined from a template

Oh! If I were to define a section from a .master in the schema, is it
lost? Should there be a sanity check that the section is not already defined?
Does this mean that schema's cannot have sections defined from .masters?

> + else:
> + if section_schema.master:

Is this test redundant? master name is like 'thing.master', so a section
will be created with master set to True.

> + schema = section_schema.clone()
> + schema.name = section_name
> + section = self.schema.section_factory(schema)
> + section.update(parser.items(section_name))
> + sections[section_name] = section
> + masters.add(master_name)
> + continue

...