[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