Code review comment for lp:~vila/bzr/doc-new-config

Revision history for this message
Andrew Bennetts (spiv) wrote :

As far as the proposed implementation goes, my thoughts are:

 * there's lots of room for bikeshedding over syntax and even features... but this seems reasonable to me
 * I'm a bit worried about performance: we should make sure the actual implementation imposes as little cost to the startup time as possible. In particular I wonder about the costs of evaluating definitions that refer to other definitions while guarding against cycles etc. It would be nice to have a configuration format that in principle doesn't require any more data to be read, parsed and validated than is relevant to the current operation.
 * To a lesser extent I'm also concerned about the performance impact of adding more kinds of config file. It'll probably be fine, but we should measure.
 * How will we manage the transition from the current config files to your proposed files?
 * I'd like more detail about what replacements for appendpath will look like. Is there going to be a magic {rest-of-path] interpolation value, or something like that?
 * In general it would be good to have more examples, perhaps even start the proposal with an example configuration file.
 * I'm not sure how the proposed name spaces work: e.g. will users need to write “bzr.debug_flags” rather than just “debug_flags” everywhere?

As far as landing this branch goes: I think land it. It should be discussed on the list, but there's no reason not to land this branch on trunk in the meantime.

review: Approve

« Back to merge proposal