Code review comment for lp:~rconradharris/glance/cached-images-middleware

Revision history for this message
Rick Harris (rconradharris) wrote :

Thanks for the comments, Ed.

> Haven't gone through all the code, but I noticed several places where str(e)
> is used. Since error strings can be localized, it is reasonable to expect
> non-ASCII characters to appear in them. Either unicode(e) or "%s" % e should
> be used instead.

Looking around, I see a few other places where we do `str(e)` as well. I'm
hesitant to fix those at the moment (perhaps those should be a cleanup patch?),
but in this case, I don't see anything wrong with using one of the forms
you've suggested.

Look for a fix shortly.

> In the open() method of the ImageCache class, you test for the presence of 'r' or 'w' in the mode param. This means that any mode that contains either of the two characters is valid, and if it contains both, then 'w' is assumed. Should this be more discriminating? e.g.:

Nice catch, if the file was opened for `rw`, it would correctly call the
`_open_write` handler, but it wouldn't bump the hit-count.

`rw` mode isn't really used (nor should it since image-data is immutable), so,
I think it's safe to "tighten" up the switch statement to be something like:

    if mode == 'r': do_read
    elif mode == 'w': do_write
    else: raise not supported

« Back to merge proposal