[PATCH] configuration policies

[John Arbash Meinel]

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

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

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

> +
> +        policy_key = '%s:policy'%option_name
> +        policy_name = _policy_name[option_policy]
> +        if policy_name is not None:
> +            self._get_parser()[section][policy_key] = policy_name
> +        else:
> +            if policy_key in self._get_parser()[section]:
> +                del self._get_parser()[section][policy_key]
> +


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.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061110/ffcddec2/attachment.pgp