[MERGE] Added PluginBranchConfig object, which allows a generalized mechanism for setting config options which travel along with the branch.
John Arbash Meinel
john at arbash-meinel.com
Mon Feb 4 19:17:19 GMT 2008
Monty Taylor wrote:
> Hey all,
>
> Here's a patch to add a new Config object. I'm using in this in a couple
> of different plugins I'm using internally. It's based in large part on
> code from bzr-builddeb, and hopefully just allows that to happen in a
> general manner.
>
> I'd love feedback on this... and also any tips on how we might want to
> write unit tests...
>
> Monty
>
>
There are a few stylistic things that would need to be updated. For
example, we always put 2 blank spaces between top-level entries
(classes/functions/etc), and your docstrings need a small amount of
cleanup. They should generally start with a single sentence, which fits
on a single line without wrapping. And we usually put the trailing """
on a line by itself. Also, we always indent 4 spaces, no tabs.
So this:
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2007-12-02 18:59:28 +0000
+++ bzrlib/config.py 2008-02-03 05:28:40 +0000
@@ -1125,3 +1125,168 @@
def decode_password(self, credentials, encoding):
return credentials
+
+class PluginBranchConfig(object):
+ """Holds the configuration settings for branch. These are taken from
+ a hierarchy of config files:
+ locations.conf
+ .bzr-<plugin-name>/default.conf
+ .bzr/branch/branch.conf
+ bazaar.conf
+ ~/.bazaar/<plugin-name>.conf
+
+ The value is taken from the first file in which it is specified."""
+
+ def __init__(self, plugin_name, branch=None, version=None):
+ """
+ Creates a config to read from config files in a hierarchy.
+
becomes:
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2007-12-02 18:59:28 +0000
+++ bzrlib/config.py 2008-02-03 05:28:40 +0000
@@ -1125,3 +1125,168 @@
def decode_password(self, credentials, encoding):
return credentials
+
+
+class PluginBranchConfig(object):
+ """Holds the configuration settings for branch.
+
+ These are taken from a hierarchy of config files:
+ locations.conf
+ .bzr-<plugin-name>/default.conf
+ .bzr/branch/branch.conf
+ bazaar.conf
+ ~/.bazaar/<plugin-name>.conf
+
+ The value is taken from the first file in which it is specified.
+ """
+
+ def __init__(self, plugin_name, branch=None, version=None):
+ """Creates a config to read from config files in a hierarchy.
+
+ locations.conf
+ .bzr-<plugin-name>/default.conf
+ .bzr/branch/branch.conf
+ bazaar.conf
+ ~/.bazaar/<plugin-name>.conf
The way these are listed is a bit confusing. As I'm pretty sure
"locations.conf" is talking about ~/.bazaar/locations.conf, and I would
guess "bazaar.conf" is as well, but you explicitly enumerate
~/.bazaar/plugin-name.conf
So I would probably say:
~/.bazaar/locations.conf
.bzr-<plugin-name>/default.conf
.bzr/branch/branch.conf
~/.bazaar/bazaar.conf
~/.bazaar/<plugin-name>.conf
I also find the ordering a bit weird. I know we usually put
"locations.conf" before a specific branch.conf so that users can
override upstreams settings on their local machine. (When I access
http://bazaar-vcs.org/bzr/bzr.dev, I should have the right to say how it
should act on *my* machine.)
However, I don't really see why the global ~/.bazaar/bazaar.conf would
override the plugin-specific ~/.bazaar/plugin-name.conf
I don't really like having ".bzr-plugin-name/default.conf". Something
more like ".bzr/plugins/plugin-name.conf" I think is a better layout.
Unless the goal is to actually version the defaults. Which is a
possibility. We should take care when advocating it. One of the reason
we generally *don't* is for security purposes. As we don't want merging
an unknown branch to start automatically spawning processes that you
might not want.
Certainly you wouldn't want "bzr log http://other/branch" to allow
spawning an arbitrary command on your local machine. Your changes may
not introduce it, but we should keep it in mind.
As for writing unit tests, this is showing up in "bzrlib/config.py" so
you should add some tests in "bzrlib/tests/test_config.py"
Make sure that you can get options out of files in the appropriate
places, that if you set a value, it updates the "correct" file, etc.
Which brings up another point, you have a lot of "get_*" functions, but
no "set_*" functions. Our BranchConfig and GlobalConfig both provide
ways to have the program set values, which seems like something you
would want here. So we would want to define where settings go, and how
to get them to go elsewhere.
John
=:->
More information about the bazaar
mailing list