Code review comment for lp:~gundlach/nova/controllers-in-api

Some style nits and questions:

1. Please kill extra newlines at:

nova/api/ec2/__init__.py lines:

95, 102, 144

nova/api/ec2/apirequest.py

493, 525

nova/api/tests/api_unittest.py

1773

nova/api/ec2/cloud.py

948, 989

2. Line up the arguments inside the parens in all the added methods in nova/image/service.py. Additionally it doesn't look like the "qs" function exists that file in the branch I am looking at, are there tests written for these methods?.

e.g.

query_args = qs({'image_id': image_id})
response = self._conn().make_request(method='GET',
                                     bucket='_images',
                                     query_args=query_args)

3. Same as #2 but for nova/api/ec2/images.py, however it _does_ have a qs function defined, which seems like it should be replaced by urllib.urlencode anyway.

4. Extra newline at nova/api/ec2/context.py line 1020

Crap, the lines mentioned next to this have nothing to do with the files, please refer to them as per the diff displayed by launchpad and hope it doesn't change, I suppose. -sigh-

5. In nova/api/ec2/cloud.py please make the formatting in create_volume match that of the other calls in that file, namely moving the {"method"... section to the next line and double indenting it.

6. Also in nova/api/ec2/cloud.py please line up the indentation in create_key_pair's return statement.

7. Please include your name in the TODO added in APIRequest's send method.

e.g. TODO(gundlach): foo

Unless this is just a renamed file being treated as added, in which case I guess
we leave it as an unclaimed TODO.

8. Please format your docstring for the Router, Authorizer, Executor class with the format

"""Single line summary of this class.

Additional details about this class.
"""

or just

"""Single line summary of this class."""

9. If the args in Authenticate's __call__ method could all fit indented to the end of manager.AuthManager().authenticate( that would be grand, but if they can't could you double indent them and make the last parens either be on the last line or lined up with rest of the words (as it stands it looks like the end of the "(user" parens.

Aaaaanyway, besides all that minor stuff it looks pretty stellar, please take the time to make sure that the stuff in #2 is being covered by tests, because it seems like any test using it would be failing since qs is not defined.

Also consider replacing qs with urllib.urlencode.

Really happy to be getting rid of Tornado and having better names for our stuff :)