[MERGE][bug #173010] initial branch over http: can show no ui activity for a very long time

Andrew Bennetts andrew at canonical.com
Sun Dec 9 23:49:40 GMT 2007


Vincent Ladeuil wrote:
> Thanks for your tweak vote and your inspiring review, but I think
> the further modifications are worth a second review, so here is
> the new patch.
> 
> Most significant points:
> 
> - tests redesigned and completed (thanks to --coverage even all
>   the error handling is now covered),

Excellent!  The tests are a pleasure to read now.

(Hmm, if someone wanted to write a plugin/script that could intersect a diff
with a coverage report, so that you could see if any lines you've added aren't
run by the test suite, that'd be really cool.)

Here's my follow-up review.  Mainly I just found a few more typos :)

bb:tweak

I'm looking forward to having this work in bzr.dev!

[...]
> 
>     Andrew> If size is <= 0, why would calling
>     Andrew> self._file.read(size) be sensible?  That looks odd to
>     Andrew> me.
> 
> I don't get it, isn't -1 the idiom to read until EOF for
> file-like objects ?

Oh, so it is.  What an ugly C-ism.  I always just call read with no args, which
seems more pythonic to me.  (Both calling read no args or a negative number are
documented to mean “read until EOF” according to
http://docs.python.org/lib/bltin-file-objects.html)

I guess that bit is fine then.  A brief “Read until EOF” comment to make it
clear what the size < 0 case is intended to do (and to remind readers like
myself ;).

Do you test the behaviour of read(0), btw?  Probably nothing tries to do that,
but it's an edge case that might do funny things if we're not careful.  For that
matter, f.seek(f.tell())/f.seek(0, 1) might be another interesting case (a seek
to the current position).

> +class TestRangeFileMixin(object):
> +    """Tests for accessing the first range in a RangeFile."""
> +
> +    # A simple string used to represent a file part (also called a range), in
> +    # which offsets are easy to calculate for test writers. It's used as a
> +    # building block with slight variations but basically 'a' if the first char

Typo: “if” should be “is”.

The test parameterisation through test case inheritance works much better than
what you had before, well done!

I wonder a little if we'll eventually decide to use the same test
parameterisation framework as for e.g. repository implementations, but this is
quite readable so I have no qualms about landing it as is.

[...]
> +class TestRangeFilMultipleRanges(tests.TestCase, TestRangeFileMixin):
> +    """Test a RangeFile for multiple ranges."""

I think a little more detail in this docstring (or maybe setUp's docstring)
about the exact layout of this RangeFile would be good; the code in this setUp
is just long enough that it's hard to see what self._file is like at a glance.

> === modified file 'bzrlib/tests/test_transport_implementations.py'
[...]
> +        # Use itertools.imap() instead of use zip() or map(), since they fully
> +        # evaluate their inputs, the transport requests should be issued and
> +        # handled sequentially (we don't want to force transport to buffer).
> +        for content, f in itertools.izip(contents, content_f):
>              self.assertEqual(content, f.read())
>  
>          content_f = t.get_multi(iter(files))
> -        for content, f in zip(contents, content_f):
> +        # Use itertools.imap() for the same reason
> +        for content, f in itertools.izip(contents, content_f):
>              self.assertEqual(content, f.read())

You need to update the comments about imap to match the code, which is using
izip.

> === modified file 'bzrlib/transport/http/__init__.py'
[...]
> +
> +        def get_and_yield(relpath, coalesced):
> +            if coalesced:
> +                # Note that the _get below may raise
> +                # errors.InvalidHttpRange. It's the caller's responsability to

“responsibility”


> +            if self._range_hint == 'multi':
> +                max_ranges = self._max_get_ranges
> +            else: # self._range_hint == 'single'
> +                max_ranges = total

Is it worth doing:

        elif self._range_hint = 'single':
            max_ranges = total
        else:
            raise AssertionError("Unknown _range_hint %r" % (self._range_hint,))

?

[...]
> === modified file 'bzrlib/transport/http/response.py'
[...]
> +# A RangeFile epxects the following grammar (simplified to outline the

“expects”

[...]
> +    def set_boundary(self, boundary):
> +        """Define the boundary used in a multi parts message.
> +        
> +        The file should be at the beggining of the body, the first range
> +        definition is read and taken into account.
> +        """

“beginning”

-Andrew.




More information about the bazaar mailing list