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

My point about using a camelcaps name in the api is

1- the expression 'GlobalConfig()' should construct a new GlobalConfig object
2- we don't want to build new objects every time someone wants to look
up a value
therefore
3- the expression to look up a value should not include something that
looks like object construction

you also mention

> config.Global.get('blah')

which looks like access to a class method, and I think we don't want
that either, because it won't scale when we need access to a
particular instance of a type of configuration.

That's why I come back to saying there should be something like a
function that gives you the stack relevant to particular scopes.

Martin

On 8 April 2011 18:12, Vincent Ladeuil <email address hidden> wrote:
>> I'd like to talk about this; istm if we have the right expression to put in config-using code we will know some aspects of the infrastructure are also right (or can be gradually made right.)
>
> +1
>
> My plan is to make small proposals on top of this one to try various approaches.
>
> Things like:
> - adding helpers to define Option with policies (boolean or registry values)
> - new SectionMatchers (I find the current LocationMatcher hard to understand for users in several edge cases, istm that real PathMatcher or GlobMatcher would be clearer)
> - the new locking scheme described in the devnotes
>
>
>> 'FooBar()' should almost always construct an object of that class; cases where we've fudged that to actually construct something else cause unclearness.  If it might need to do something else (either make a different class, or return an existing object), we should use a factory function.
>
> I'm not sure what you're targeting here, but yeah, some registries are still missing in the proposed design (because they weren't required to reach an ~equivalent implementation).
>
>> In these cases we generally want to be reusing existing objects, not making new ones.
>
> If by that you mean making sure we don't have to create a ConfigStack each time we want to get an option value, I fully agree.
>
>> If this is a new api maybe it should hang off the library_state object; though there can be a convenient caller interface that doesn't explicitly need to do so.
>
> For GlobalStack and LocationStack, that's indeed the plan.
>
> Also note that it should be possible to add some sort of cloning facility at this level without needing to re-read any config file: say you have a LocationStack and you want one but for a different location.
>
>> I guess 'global' here means (eventually): from the command line, environment variables, user config, system-wide config, built in defaults.  Perhaps others.  Callers ought to be expressing essentially constraints on what values could be relevant, without knowing the policy of how that's matched up.
>
> Roughly yes. Since os.environ and command-line options are *not* supported today, the proposal didn't mention them.
>
> But the idea is that they should be added at the stack level (just look at the concrete stacks there, adding them is just adding the right dict (yes, the python object) into the stack.sections list at the right place.
>
>> Making it easy to share and reuse these facilities when creating different stacks is not implemented here but shouldn't be hard either.
>
>> The more interesting case there is something more restricted than global.  I was contemplating perhaps having them say
>
>> config_stack_for([branch, working_tree, destination_transport])
>
>> where they pass in a list of objects that are relevant to their current scope, and then the config mechanism works out which sections/sources to use from that.
>
> Yes, that's the idea, I don't know yet how this will be implemented but some patterns are already known and mentioned in the devnotes, the relevant ones should naturally emerge as we deploy.
>
>
> --
> https://code.launchpad.net/~vila/bzr/new-config/+merge/56887
> Your team bzr-core is requested to review the proposed merge of lp:~vila/bzr/new-config into lp:bzr.
>