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.

Well, basically I'm raising problems I want to address but since I don't have answers so far, I can't (yet) update the user documentation.

>
> This should also be mentioned in whatsnew.

Right, I can do that.

>
> 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?

Please, let's not hijack it indeed, nothing has changed in this regard, the problems are still here and we don't provide (yet) a clearly defined set of features.

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

Right, and from there, do you agree that sections should not be used *at all* in the other configuration files.

I realise we will need to address the ALIASES and BOOKMARKS cases, but we must first agree on that.

>
>
> > 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 :-)

That's a different case, the former makes it *impossible* to use fnmatch.translate !

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

I can do that, it will probably go away in the end and be controlled via the registry (when it's implemented).

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

Left over, good catch.

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

And unconditionally do the find ?

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

I'm glad you noticed that ;) Yes, I think Command objects are still a good candidate for that.

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

Well, I'm never comfortable with inlined files that differ from what they would be on disk. I forgot the '\' at the end of the first line for that to be true though.

Should we bikeshed on that or is it ok to have different povs there ?

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

This output is a *valid* content for a config file.

At least for the actual tests ;)

Things may become more complicated if we want to ensure that tough:
- comments are not displayed,
- strings are not quoted,
- multi-lines strings are not even considered.

And why would you use such output to put into 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.

Well, that would quickly be confusing since you want to see the multiple definitions as soon as there is more than one without having to specify -v, remember that you can use 'bzr config' without specifying any variable at all.

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

Well, it's a first step towards defining test fixtures that are independent from the test classes. Defining them as methods is where I'm coming from :)

But it's already a good step as it allows to use them in test classes that inherit from different base classes without forcing the use of a mixin.

Adding attributes to the test instance seems more tester-friendly than returning values (especially here where there are natural dependencies between the configs and the branch and the tree).

Arguably using 'make_branch_and_tree' puts a constraint on the test instance... but since this is also something that should become a test fixture I chose to punt on this one.