Code review comment for lp:~thomir-deactivatedaccount/bzr/add-global-config

Hi Michael,

I'd like to get this right, so I'll probably bug you later this evening if I get time...

> On naming:
>
> We definitely shouldn't change the meaning of an existing name like
> GlobalStore, even if it's not intended to be exposed. It's much
> better for out-of-date code to get eg a NameError than to run the
> wrong thing.
>

The branch as it currently stands will give you that behavior. Nobody should be creating the Store classes directly (nobody is within bzrlib, I don't know about external projects), and now if they try they'll get a NameError.

> If the class really has actually different behaviour, like
> BranchStore, it deserves to be a real different class. But if it's
> basically the same class just with different constructor parameters, I
> wonder if it's worthwhile.
So how about this, for the branch store: replace the BranchStore class with:

def _get_branch_store(branch):
    class BranchStore(IniStore)
        # otherstuff in here from BranchStore class
    return BranchStore(branch)

This has the advantage that: A) it keeps it inline with the other factory functions. B) it hides the different implementation of the BranchStore class, and marks the BranchStore object as being a private thing that other code shouldn't use.

> Perhaps for simplicity it is better just
> to leave them all as classes. I may be getting a bit bikesheddy here
> so don't block on it.
>
Well, I'd like to get it right - I'm preparing to do some more things with sloecode & Bazaar, so learning the bzr code is helpful to me.

What do you think?