[PATCH] configuration policies

[James Henstridge]

On 10/11/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
> James Henstridge wrote:
> ...
>
> >
> > Attached is an updated version of the patch, based on feedback from
> > Aaron and John on IRC.  It now uses one key per option to specify
> > policy rather than one key per policy.
>
>
> Here are some code level comments on your patch.
>
>
> v- Just to check, did you make sure that the name 'config' wasn't used
> elsewhere in the file? it is one of those names that is likely already
> taken by other variables. So it might be better to do this as 'config as
> _mod_config'.

Yeah, there were some local variables named "config", so I've made this change.


> > === modified file bzrlib/branch.py // last-changed:james at jamesh.id.au-200611070
> > ... 71558-f39bb86f8ef5de7c
> > --- bzrlib/branch.py
> > +++ bzrlib/branch.py
> > @@ -27,6 +27,7 @@
> >  from bzrlib import (
> >          bzrdir,
> >          cache_utf8,
> > +        config,
> >          errors,
> >          lockdir,
> >          lockable_files,
> > @@ -1230,8 +1231,8 @@
> >
> >      def set_push_location(self, location):
> >          """See Branch.set_push_location."""
> > -        self.get_config().set_user_option('push_location', location,
> > -                                          local=True)
> > +        self.get_config().set_user_option(
> > +            'push_location', location, store=config.STORE_LOCATION_NORECURSE)
> >
> >      @needs_write_lock
> >      def set_parent(self, url):
> >
>
> v- This should probably be a deprecation warning using
> symbol_versioning.warn, and include 'as of bzr 0.13'. You'll then want
> to add a test that uses TestCase.callDeprecated() which can make sure
> that the warning is issued, but that the function still does what you
> want it to.

I've made this change too, changing the
test_set_user_option_recurse_false_section() test to use
callDeprecated().


> > +            warning('The recurse option in section %s of %s '
> > +                    'has been converted to a policy_norecurse option'
> > +                    % (section, self._get_filename()))
> > +            del self._get_parser()[section]['recurse']
> > +            for key in self._get_parser()[section].keys():
> > +                if not key.endswith(':policy'):
> > +                    self._get_parser()[section]['%s:policy'%key] = 'norecurse'
>
> ^- ['%s:policy' % key]
> v- '%s:policy' % option_name
>
> Though I might prefer [key + ':policy'], it is clearer to me, but maybe
> not others.

That does look a bit cleaner, so I've made the change.

[snip]
> So a couple small changes, and you should add a test that the deprecated
> path raises a warning, but actually does what you have requested.
>
> I'm thinking it should actually set the recurse=False flag, because
> otherwise new entries won't get the information. Which is a different
> behavior.
>
> We can still consider it deprecated, though.

The idea of this patch was to move the policy from section level to
the option level.  Do people expect options written by Bazaar to pick
up whatever policy happens to be on the section?

I've attached an updated bundle.

James.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr-config-policy-3.patch
Type: text/x-patch
Size: 75280 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061117/00e1e19b/attachment.bin