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

Revision history for this message
Martin Pool (mbp) wrote :

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

That looks nice, and it makes this easier to review.

My main question from what you have documented there is, when you set a value, in what scope is it set?

(It would be really cool, later, to actually do scriptrunner tests within the user guide; somehow we would need to make sure the setup/check code was not too intrusive to people reading the document.)

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

That's true the test itself is not Python syntax. However, it is embedded within a Python file, and the rule there is that things are indented by scope. Even though technically the indent whitespace appears within the triplequoted string, the effect we want to achieve is that the whole thing is shifted right.

docstrings or (in other programs) inline sql are not Python either but they're normally indented; <https://dev.launchpad.net/PythonStyleGuide#Multi-line%20SQL> gives a weak precedent.

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

The main reason I want them indented is so that scanning over the file, I can easily see the classes and the test modules within them. Having internals of the tests flush left messes that up.

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

I think the output needs to make some things more clear to the user:

1- wtf is 'locations'? :-)
2- which value is actually active? which of these is most important?
3- which location section is matching that this matters?

We could perhaps make that more obvious with

# per-location-config from /home/mbp/.bazaar/locations.conf

(I'm open to tweaking this after landing)

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

Shell scripting?

I think you have a reasonable point about the default explaining what's going on.

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

+1 for experimenting.

I want to get away from mixins used to provide scenarios, and from mixins that have magic special effects. Mixins that merely provide new methods that you _can_ call are pretty harmless in my view, but perhaps still not the best solution.

You don't seem to have actually put this into whatsnew yet?

Aside from that, I think this is a step forward and ok even without these steps. Perhaps someone would give a second opinion?

review: Needs Fixing

« Back to merge proposal