Code review comment for lp:~rackspace-titan/glance/wsgi-refactor

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

Overall looks good. Some thoughts:

> 425 + self._inject_location_header(response, image_meta)
> 426 + self._inject_checksum_header(response, image_meta)
> 427 + return response

Super nit-picky here, but might be worth DRY'ing up some of this code since
it's repeated quite a bit.

> 582 + return (request.content_length and request.content_length > 0) \
> 583 + or 'transfer-encoding' in request.headers

This statement was a little hard to read at-first. Once I understood it, it
was clear enough. That said, given that we're, for the most part, optimizing
for readability, maybe it would be clearer as:

  if 'transfer-encoding' in request.headers:
    return True
  elif request.content_length is None:
    raise Exception("Content-Length was not specified and we're not "
                    "Transfer-Encoding")

  return request.content_length > 0:

> 683 + if type(action_result) is dict:

Duck-typing might be preferable here. Do you know specifically what dict
methods the serializer is using; if you're not sure, you could always check
for presence of `keys()`

  if hasattr(action_result, 'keys'):
    blah

(This is a really small nit, but if we ever need to do something tricky by
subclassing `dict`, this would keep the code working)

> + """Find action-spefic method on self and call it."""
s/spefic/specific/

> 695 + except Exception:
> 696 + method = getattr(obj, 'default')

Should this handler be this broad? Would AttributeError work here?

If that's the case, we could do:

  method = getattr(obj, action, 'default')

« Back to merge proposal