[MERGE] Remove manual notification of transaction finishing on versioned files.

[Martin Pool]

On Tue, Apr 8, 2008 at 7:51 AM, Robert Collins
<robertc at robertcollins.net> wrote:
>
> On Mon, 2008-04-07 at 15:43 +1000, Martin Pool wrote:
> > None of the occurrences of get_scope have a docstring, and the news entry
> > is not particularly illuminating either.  There should be something
> > somewhere.  For instance what is get_scope meant to return, and should it
> > be passed to the constructor or assigned into the instance.
> >
> > If we are introducing a new top level concept maybe it should have a
> > paragraph or three in the glossary of the developer guide.
> > Like
>
> >   A scope controls when an in-memory object is valid.  The scope itself
> >   is an opaque token, assigned by the caller.
>
> I don't think it should be in the guide; I should have added docstrings
> for the use of it as parameters.

Well, it comes down to whether this is introducing a new general
pattern, in which case it should be in the developer guide, or whether
it's something specific to this case.

> > +        if None in (access_method, index):
> > +            raise NotImplementedError, "No default access_method or
> > index any more"
> >
> > You should use NotImplementedError(...) syntax, and this would
> > probably more correctly be ValueError.
>
> Is that a python 3K preparation thing ? ValueError is something folk can
> plausibly catch; I'd much rather use NotImplementedError for the
> interim.

The comma syntax I believe is deprecated by pep8 or elsewhere, and
anyhow inconsistent with what we normally use.  ValueError is the
correct error for a bad parameter value, and NotImplemented seems
strange because we normally use it to mean "not implemented _yet_".  I
don't think people would be catching ValueError around this method for
any particular purpose.  (It is reasonable to catch it around say
"int(v)")

> > Separately declaring a function that's later used by being bound in as a
> > static method that itself is sometimes treated as a class with class
> > variables is unusual enough that I at least wonder if it should have a
> > comment.
>
> Its for compatability with the existing interface - the one I am working
> on deprecating/removing.

It's fine, but a comment would be sensible.

-- 
Martin <http://launchpad.net/~mbp/>