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.

Ok.

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

Exactly, I didn't mention authentication.conf yet, but it re-implements too much things already in IniConfig and I did that at the time because I felt it wasn't fitting well 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.

Hmm, I'd argue that their syntax has nothing to do with python.

> In all the
> editors we use it's easy to indent/dedent things.

But even in python the indentation is meaningful (I agree that in this case the initial indentations are meaningless but we are still using dedent as a syntactic sugar and mostly because we can and because we apply a python rule).

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

Rhaaa, of course not, I was referring to ' file = locations' not the config id line.

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

Right, so I worked from the assumption that 'bzr config <glob>' will be used when a user encounter a problem and need to check why his expectations are not met. As such, displaying every definition of one or several options should be the default mode.

Now, I can an option like '--first' or '--current' or '--active' but since that's the first occurrence displayed I think displaying it *without* the file it's coming from is not that useful.

What use case do you have in mind there ?

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

Right, that's exactly my purpose: we want to get away from mixins (or did I get *that* wrong too ?) so I'm looking at the alternatives and a first result is that using such helper makes it already easier to reuse them (easier than mixins IMHO).