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

[Robert Collins]

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.


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

> +
> +def get_suffixes():
> +    """Return the suffixes used by file based knits."""
> +    return [DATA_SUFFIX, INDEX_SUFFIX]
> +make_file_knit.get_suffixes = get_suffixes
> 
> There should be two blank lines between top level declarations.

"""
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).
"""

I was grouping the related one liner with the class definition (We
routinely do this in class scope with properties, and I think this is no
different).

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

> +        if self.get_scope() != self.scope:
> +            raise errors.OutSideTransaction()
> 
> To the new reader this class seems to have one conceptual property,
> 'scope', available as either a getter method or a public member -- and yet
> they are different.  Smells bad.
> 
> Also, if this is not part of the public api, why does it have public
> names?
>
> How about instead calling them say
> 
>   _scope
>   _current_scope()
> 
> I realize they're only used in fairly internal classes, but I think it
> still conveys something different if they have names that look like public
> names.

Made private. (Side issue: I'd really like us to find a better way to
express visbility; renaming bindings to make them more or less visible
is deeply unsatisfying).

> +    def __init__(self, _format, a_bzrdir, control_files,
> _revision_store, control_store, text_store):
> +        super(MetaDirVersionedFileRepository, self).__init__(_format,
> +                                                a_bzrdir,
> +                                                control_files,
> +                                                _revision_store,
> +                                                control_store,
> +                                                text_store)
> 
> Too long, and too widely indented.

Feh, moved code; I'll reindent.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080408/f2e45eb8/attachment.pgp