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

> 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.

I would factor this out a bit more if it were clear exactly how to do it. Now, I could make a method called inject_location_and_checksum_headers, but I don't see much value in that. I already factored out quite a bit with regards to these two functions. If you still feel strongly about this, I'll do it for you.

> > 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:

I cleaned it up with a longer if/elif block, while keeping the same logic. This is a boolean check, so I didn't want to throw an exception.

> > 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)

How about I just assume I can serialize it, and if there is an exception I will just return the original results? I checked in a possible implementation of this. Tell me what you think.

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

Good catch. Fixed.

> > 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')

I added AttributeError to the except statement. I can't use 'default' as a third argument there because it expects a callable. I would have to pass in obj.default, and I can't assume that exists.

I also caught some issues in the wsgi.Resource docstring and cleaned that up.