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

On 12 October 2010 19:06, Vincent Ladeuil <email address hidden> wrote:
>> 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.

I think it should at least address the new command, even if the
description is imperfect or if some uncertainty remains. For example
the user guide ought to mention the existence of this command.

>
>>
>> > 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 almost agree. I think they should only be used in ways that are
parallel to locations. For instance I think the authentication.conf
thing is basically ok, because parallel to locations.conf the sections
are used to find which settings are relevant.

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

Maybe we can bikeshed a bit :-). I don't see these as inlined files.
I know you just added test-script but I don't think they normally are
files on disk; they're primarily bits of a Python program. In all the
editors we use it's easy to indent/dedent things.

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

I wouldn't necessarily send it into a file, I just don't think it
should be gratuituously different.

So is

  locations:
     file = locations

a valid configobject file? It's not the form we tend to use, which
has [] sections.

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

I feel there ought to be _a_ mode which says: show me the value of
this variable which is actually active in this scope.

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

Perhaps this would be better as a mixin? I wouldn't veto this section
but I think we're still groping towards getting the right pattern
here.

--
Martin