[MERGE] improvements to the docstrings of bzrlib.smart and contents.

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Michael Hudson wrote:
bb:resubmit

> +At the bottom level there the protocol consists of bytes,

Two things: "there the protocol" looks grammatically wrong.  And I don't
think the original intent was to call the raw bytes a protocol.

 which are
> +transmitted over a socket, a pair of pipes, or an HTTP request/response.  We
> +call this layer the *medium*.

This makes it sound like the bytes are the medium, not the method used
to transmit the bytes.  So this actually reduces clarity IMHO.


> These different options have different

Let's call them "media" or "mediums", not "options".

> +behaviors:
> +
> + - You can pass multiple requests over a socket and you get a read error when
> +   the other side has shutdown.

I'm not sure about "shutdown", since it's not a normal word.  We could
say "has shut down".  Or we could say "done a shutdown" if we want to
telegraph the fact that we're referring to the socket function.

> + - When the other end of a read pipe has gone away and you try to read from
> +   it, you get a zero bytes which indicates 'end-of-file'.

In Python terms, it might make more sense to say "empty string" than
"zero bytes".

> +
> + - For HTTP, each request is independent and so there no concpet of
> +   'end-of-stream'.

"concept" misspelled.

>  So we need a wrapper around pipes and sockets to seperate out requests from

"separate" misspelled.

> -On top of the medium is the *protocol*.  This is the layer that deserialises
> -bytes into the structured data that requests and responses consist of.
> +On top of the medium is the *protocol*.  This is the layer that deserializes
> +bytes into the structured data that make up requests and responses.

As a Canadian, I get to pick and choose between US and UK spellings, and
I prefer "serialize".

> -PROTOCOL  (serialization, deserialization)  accepts structured data for one
> + PROTOCOL (serialization, deserialization)  accepts structured data for one

At least be consistent about whether it's "deserialization" or
"deserialisation".


> @@ -83,7 +83,8 @@
>          """Initialize a bzrdir at path.
>  
>          The default format of the server is used.
> -        :return: SmartServerResponse(('ok', ))
> +
> +        :return: ``SmartServerResponse(('ok', ))``

We don't normally quote return values when they refer to objects.  I
would really hate to start putting double-backticks around all our
objects.  It's not returning the string "SmartServerResponse(('ok', ))"
or anything.

> === modified file 'bzrlib/smart/medium.py'
> --- bzrlib/smart/medium.py	2007-10-05 14:52:02 +0000
> +++ bzrlib/smart/medium.py	2007-10-25 12:03:45 +0000
> @@ -21,7 +21,7 @@
>  
>  Media carry the bytes of the requests somehow (e.g. via TCP, wrapped in HTTP, or
>  over SSH), and pass them to and from the protocol logic.  See the overview in
> -bzrlib/transport/smart/__init__.py.
> +`bzrlib.smart`.

Single backticks are definitely wrong, unless you're trying to make it
italic.

> -    The stream may be a pipe connected to sshd, or a tcp socket, or an
> -    in-process fifo for testing.
> +    The stream may be a pipe connected to sshd, or a tcp socket, or an in-process
> +    fifo for testing.

This is just changing the line-wrapping, right?

> -    The server passes requests through to an underlying backing transport, 
> -    which will typically be a LocalTransport looking at the server's filesystem.
> +    The server passes requests through to an underlying backing transport, which
> +    will typically be a `bzrlib.transport.local.LocalTransport` looking at the server's
> +    filesystem.

Augh, why all these backticks?

> -        :returns: a SmartServerRequestProtocol.
> +        :returns: a `SmartServerRequestProtocol`.

Why?

> -        """Construct a SmartClientMediumRequest for the medium medium."""
> +        """Construct a `SmartClientMediumRequest` for the medium ``medium``."""

I really do object to these backticks.  In the context of API
documentation, you shouldn't have to write anything special when
referring to parts of the API.

>  class SmartClientMedium(object):
> -    """Smart client is a medium for sending smart protocol requests over."""
> +    """A SmartClientMedium is a medium over which smart protocol requests can be sent."""

Dangling participles are frequently less awkward than the alternative
"grammatical" construction.

>          The body is encoded with one line per readv offset pair. The numbers in
> -        each pair are separated by a comma, and no trailing \n is emitted.
> +        each pair are separated by a comma, and no trailing ``\\n`` is emitted.
>          """

Molesting the \n like this reduces clarity.  One might easily think that
 the string is a backslash, followed by a line feed.

>      def cancel_read_body(self):
> -        """After expecting a body, a response code may indicate one otherwise.
> +        """After expecting a body, a response code may indicate otherwise.

Neither the original nor the new phrasing is very clear to me.

>      def execute(self, *args):
>          """Public entry point to execute this request.
>  
> -        It will return a SmartServerResponse if the command does not expect a
> +        It will return a `SmartServerResponse` if the command does not expect a
>          body.
>  
> -        :param *args: the arguments of the request.
> +        :param \*args: the arguments of the request.

I think I'd rather omit the asterisk than introduce a backslash.  The
asterisk isn't really part of the variable name.

> @@ -173,8 +173,8 @@
>      def _run_handler_code(self, callable, args, kwargs):
>          """Run some handler specific code 'callable'.
>  
> -        If a result is returned, it is considered to be the commands response,
> -        and finished_reading is set true, and its assigned to self.response.
> +        If a result is returned, it is considered to be the commands response;

Should be "command's response", I think.

> +        finished_reading is set true and its assigned to self.response.

Should be "it's assigned".

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHI5si0F+nu1YWqI0RAmopAJ9J/Jm6JjJGYij++3x7TTV6O4LfUgCcDjyT
VolP7ydXrSPrA0Erd8cBsno=
=hbzr
-----END PGP SIGNATURE-----