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

> I think that I would prefer that the ImageSerializer class NOT be responsible
> for:
>
> a) creating a webob.Response object

I moved this into wsgi.Resource

> b) modifying any headers of the Response object other than Content-Type

I'm going to take some time to figure this out. I'll have another commit for you later today.

> Tiny Nits/Fixes
> ---------------
>
> Here are some little nits to correct before an approve.
>
> 1) docstring stuff
>
> 445 + """Base Nova Exception
> 446 +
> 447 + To correctly use this class, inherit from it and define
> 448 + a 'message' property. That message will get printf'd
> 449 + with the keyword arguments provided to the constructor.
> 450 +
> 451 + """
>
> Please use this style instead:
>
> """
> Base Nova Exception
>
> To correctly use this class, inherit from it and define
> a 'message' property. That message will get printf'd
> with the keyword arguments provided to the constructor.
> """
>
> No need for extra newline at the end, and please put the short description on
> a new line after the opening """
>
> Some of the new docstrings are correct, others aren't... please do a quick
> run-through to check they are all consistent like the above style.
>
> Also, we're Glance, not Nova ;P

Copy/paste error. Sorry for that. I fixed the problems I introduced. There are a bunch of docstrings that don't follow your suggestion in registry/server.py (probably elsewhere as well) that I did not fix because I did not introduce them in this merge prop. Maybe a style cleanup is in order?

> 2)
>
> 475 import datetime
> 476 -
> 477 import eventlet
>
> eventlet is a 3rd party library. We separate stdlib modules from 3rd party
> modules with a single newline.

Ah, I thought it was just separation of non-glance and glance modules. Fixed that.

> 3) few little typos
>
> 67 + :retval similiar to 'show' method but without image_data
>
> s/similiar/similar

Fixed. I do that all the time :(

> 4)
>
> 133 + if 'location' in image_meta and image_meta['location'] is not None:
> 264 + if 'location' in image_meta and image_meta['location'] is not None:
>
> can be shortened to:
>
> if image_meta.get('location'):

Fixed. I took it a step farther and stored location in a variable before the conditional. We needed to use it again immediately anyways, so I removed the duplication of reading from the image_meta dict.

> 5)
>
> 342 + def create(self, request):
> 343 + result = {}
> 344 + result['image_meta'] = utils.get_image_meta_from_headers(request)
> 345 + data = request.body if self.has_body(request) else None
> 346 + result['image_data'] = data
> 347 + return result
> 348 +
> 349 + def update(self, request):
> 350 + result = {}
> 351 + result['image_meta'] = utils.get_image_meta_from_headers(request)
> 352 + data = request.body if self.has_body(request) else None
> 353 + result['image_data'] = data
> 354 + return result
>
> Little bit of DRY cleanup needed there... something like this would do:

Good catch. I didn't even notice that.

> 6) the wsgi.Resource constructor
>
> 665 + def __init__(self, deserializer, controller, serializer):
>
> Could you change the param order to (controller, serializer, deserializer)?
> You list the params in the docstring that way, and I think it makes more sense
> that way...

I changed the order in the constructor because that is the logical flow of a request through the object. I switched it back because we could make serializer/deserializer optional kwargs in the future. I did keep deserializer before serializer, though. I hope that's okay with you.

> Also, have you thought about how you will deal with multiple serializers? How
> do you plan to support a resource that can be translated into both JSON and
> XML?

Yep, we will implement a content-type map (similar to Nova wsgi) on the (de)serializer objects that map to content-type specific body deserializers. We'll take care of that in a future xml-support MP.