Code review comment for lp:~jaypipes/glance/bug713154

On Tue, Jul 26, 2011 at 11:34 AM, Brian Lamar <email address hidden> wrote:
> Review: Approve
> These are notes really, rather than a review, because the code looks good to me and the tests look awesome:
>
> 309     +        from boto.s3.connection import S3Connection
> 354     +        from boto.s3.connection import S3Connection
>
> Any reason for these imports to be where it is? Waldon says this is probably a dependency management thing (you don't want to require boto if they're not using S3, right?), but I think there should be a blueprint for better optional importing in both Nova and Glance if this is the case because it's done several different ways.

Yup, and they get cleaned up in my refactor-stores branch...

> 252     +        try:
> 253     +            while True:
> 254     +                chunk = self.fp.read(ChunkedFile.CHUNKSIZE)
> 255     +                if chunk:
> 256     +                    yield chunk
> 257     +                else:
> 258     +                    break
> 259     +        finally:
> 260     +            self.close()
>
> Might consider shorter "with" syntax here, I think it would just be:
>
> with self.fp:
>    yield self.fp.read(ChunkedFile.CHUNKSIZE)

Noted. I'll clean that up in the refactor-stores branch, too :)

Cheers!
jay