Merge lp:~barry/lazr.config/bug-310619 into lp:~launchpad-pqm/lazr.config/devel

Proposed by Barry Warsaw
Status: Merged
Merged at revision: not available
Proposed branch: lp:~barry/lazr.config/bug-310619
Merge into: lp:~launchpad-pqm/lazr.config/devel
To merge this branch: bzr merge lp:~barry/lazr.config/bug-310619
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Needs Fixing
Review via email: mp+2519@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :

This adds the .master idea as described in the bug.

It also fixes Python 2.6 compatibility. I've tested it with Python 2.4, 2.5 and 2.6.

The one thing that is a bit questionable is the need to add a .clone() method to SectionSchema so that the 'name' could be overridden. Otherwise, there's no way to get the section name from the config instead of the schema (i.e. the section.master name).

Revision history for this message
Barry Warsaw (barry) wrote :

This branch has some problems. I'm going to continue working on it, but I'd appreciate any comments or suggestions you might have.

Revision history for this message
Barry Warsaw (barry) wrote :

Never mind. I think the branch /is/ okay. I added another test case to prove
it to myself. See attached incremental.

=== modified file 'src/lazr/config/README.txt'
--- src/lazr/config/README.txt 2008-12-23 02:09:06 +0000
+++ src/lazr/config/README.txt 2008-12-23 14:30:37 +0000
@@ -791,6 +791,24 @@
     key4 : F&#028c;k yeah!
     key5 :

+push() can also be used to extend master sections.
+
+ >>> for section in master_conf.getByCategory('bar'):
+ ... print section.name, section.baz
+ bar.master 1
+
+ >>> master_conf.push('override', """
+ ... [bar.two]
+ ... baz: 2
+ ...
+ ... [bar.three]
+ ... baz:3
+ ... """)
+ >>> for section in master_conf.getByCategory('bar'):
+ ... print section.name, section.baz
+ bar.two 2
+ bar.three 3
+

 pop()
 =====

=== modified file 'src/lazr/config/testdata/master.conf'
--- src/lazr/config/testdata/master.conf 2008-12-23 01:59:49 +0000
+++ src/lazr/config/testdata/master.conf 2008-12-23 14:30:02 +0000
@@ -1,3 +1,6 @@
 # This section defines a category master.
 [thing.master]
 foo: 0
+
+[bar.master]
+baz: 1

Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (5.6 KiB)

> === 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.

> categ...

Read more...

review: Needs Fixing (code)
lp:~barry/lazr.config/bug-310619 updated
4. By Barry Warsaw

Added another test case.

5. By Barry Warsaw

Several fixes and responses to reviewer's comments. Specifically, fix the
problem where a push of a .master override lost the keys for any .master
sections defined in the schema instead of in the config.

As per Curtis's comments, remove some obsolete workaround by James H.

6. By Barry Warsaw

Treat .master sections the same as .template and .optional sections when they
are defined in configs. That way, if we ever decide to raise exceptions for
that case, we'll do it for all three types.

Remove a redundant test, but assert it any way. Trust, but verify.

Revision history for this message
Barry Warsaw (barry) wrote :
Download full text (14.4 KiB)

Sadly, my email response never made it through. Here then is a cut/paste of
that message.

Note that I have a megamerge branch that encompasses the three outstanding
branches and fixes a few other things in this branch.

> Review: Needs Fixing code
>> =3D=3D=3D 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 =20
>> contain
>> +sections that extend the .master section. These are like =20
>> categories with
>> +templates except that the section names extending .master need not =20=

>> 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.
>
> }}

Good point, done.

>> +
>> + >>> master_schema_conf =3D path.join(testfiles_dir, =20
>> 'master.conf')
>> + >>> master_local_conf =3D path.join(testfiles_dir, 'master-=20
>> local.conf')
>> + >>> master_schema =3D ConfigSchema(master_schema_conf)
>> + >>> len(master_schema.getByCategory('thing'))
>> + 0
>> + >>> master_conf =3D master_schema.load(master_local_conf)
>> + >>> sections =3D 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'

It does! (now <wink>).

> ...
>
> =3D=3D=3D 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.

Gone.

> ...
>
>> @@ -288,6 +296,7 @@
>> name_parts =3D name.split('.')
>> is_template =3D name_parts[-1] =3D=3D 'template'
>> is_optional =3D name_parts[-1] =3D=3D 'optional'
>> + is_master =3D name_parts[-1] =3D=3D 'master'
>
> In my design, .optional and .template were mutually exclusive. I =20
> believe
> this is true with .master.

Right.

> Your design implies that the schema may define a section from the =20
> master.
> I think this is practical, given that the application may require at =20=

> least
> one section defined for a process, but allow for additional =20
> processes to
> be defined in a config.

Yep. I had some problems with that, but I've now fixed it.

>
> How does a section from a .master category interact wi...

Revision history for this message
Barry Warsaw (barry) wrote :

Subscribers

People subscribed via source and target branches