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

Epic cover letter. What a shame it's not updating the user documentation. :-) Perhaps you could paste and update some of it into there: we need the documentation, and checking how it's explained to the user is a good way to find out if the ui is actually sane.

This should also be mentioned in whatsnew.

And, I don't want to hijack your mp, but perhaps some of the other text could go into a developer document giving an overview of configurations?

> Section handling

That seems like a good description of the situation. I think locations.conf is on the right track by reserving the section headers to match _which_ configuration applies, not as part of the settings.

> I chose 'config' over 'configuration' as the later is already an help topic. Cheap shot :)

Probably the topic needs to be updated then.

> + ('cmd_config', ['conf'], 'bzrlib.config'),

I don't think people will type this often enough that saving two characters is worth it.

-from fnmatch import fnmatch
+import fnmatch

Not this again :-)

+ def id(self):
+ return 'bazaar'

'id' is of course a Python builtin. It's legal and not shadowing anything to put it as an instance attribute but I wonder if it's a good idea; maybe config_id would be better.

+ commands.Option('force', help='Force the configuration file',
+ short_name='f', type=unicode),

Force it to do what? :)

If it's going to be "config --force ~/.bazaar/bazaar.conf" that's a bit strange and inconsistent with other uses of --force, which take no argument. Maybe just --file?

OK, I see later on this is actually passing a kind of configuration scope.

+ if matching is None:
+ matching = '*'
+ self._show_config('*', directory)

Why assign to 'matching' in here?

+ else:
+ if remove:
+ self._remove_config_option(matching, directory, force)
+ else:
+ pos = matching.find('=')
+ if pos == -1:
+ self._show_config(matching, directory)
+ else:
+ self._set_config_option(matching[:pos], matching[pos+1:],
+ directory, force)
+

I would unfold the 'remove' to one if/elif/else.

This seems like a very good case for wanting both a cli command and another layer that's just below the cli but not specific to that, that can be reused by other front ends or people scripting bzrlib in Python. I'd like to have a standard pattern for this. This code is actually fairly close, and seems to indicate perhaps we should keep that on Command objects and just make sure they're easy to use other than through run_bzr.

+ def test_locations_config_outside_branch(self):
+ self.bazaar_config.set_user_option('hello', 'world')
+ self.locations_config.set_user_option('hello', 'world')
+ script.run_script(self, '''
+$ bzr config
+bazaar:
+ hello = world
+''')
+

Now that scripts automatically strip consistent indentation, I think we should normally keep them indented inside the block, so that they don't mess up the Python structure.

I wonder if the output from config when showing more than one variable should be something that looks like a config file?

istm that if you ask just for the value of a variable, you should by default just get that one variable, and only with say -v be told where it comes from.

+def create_configs(test):
+ """Create configuration files for a given test.
+
+ This requires creating a tree (and populate the ``test.tree`` attribute and
+ its associated branch and will populate the following attributes:
+
+ - branch_config: A BranchConfig for the associated branch.
+
+ - locations_config : A LocationConfig for the associated branch
+
+ - bazaar_config: A GlobalConfig.
+
+ The tree and branch are created in a 'tree' subdirectory so the tests can
+ still use the test directory to stay outside of the branch.
+ """
+ tree = test.make_branch_and_tree('tree')
+ test.tree = tree
+ test.branch_config = config.BranchConfig(tree.branch)
+ test.locations_config = config.LocationConfig(tree.basedir)
+ test.bazaar_config = config.GlobalConfig()
+

These seem so tightly coupled to the test class they would be better off as methods on it.