Code review comment for lp:~rackspace-titan/nova/osapi-serialization

> The try/except structure in diff lines 1865-68 is awkward: only the lines that
> could cause the exception should be in the block. Also, there is a mix of the
> try/except approach and the 'if' test approach for checking keys. I recommend
> sticking with try/except, using something more like:
>
> def get_action_args(self, request_environment):
> """Parse dictionary created by routes library."""
> args = request_environment['wsgiorg.routing_args'][1].copy()
> try:
> del args['format']
> except KeyError:
> pass
> try:
> del args['controller']
> except KeyError:
> return {}
> return args

Consistency is definitely a good thing to have. I updated the conditionals you mentioned with try/except blocks.

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> Line 1878: Bare excepts and/or catching the base Exception class should not be
> used. Catch the specific exception type, which in this case is probably an
> AttrbuteError. If that's correct, the whole structure could be written more
> simply as:
> action_method = getattr(self, action, self.default)

Good point. I typically use Exception when I don't know what exactly I may need to catch. Like you said, in this case I will only have to catch AttributeError. I changed the "except Exception" cases to be more specific across the module.

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> Diff lines 1928 & 1946 should not be comparing type(x) to a type; instead, use
> isinstance().

So these are "legacy" conditionals that have been working here since before I was on the project. This branch is targeted at refactoring the organization of the wsgi-related objects, not rewriting our generic xml serialization. After hearing that, are you okay with me leaving it?

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> Diff line 2003: should be catching KeyError, not a base Exception.

Fixed.

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> Diff line 2034: multiple format placeholders should use the mapping style. See
> https://bugs.launchpad.net/nova/+bug/703041 for explanation.

Well this string isn't i18n'd, but I did it for you anyways :)

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> 2045: use isinstance for type checking

This is another one of those legacy conditionals. I would love to solve this more elegantly in a future branch, possibly once we decide if we are going towards objects for our data models (instead of the dict it is checking for now).

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> 2994: grammar nit: change to: "well and have your controller be a controller
> that will route"

I should have caught that. Cleaned it up a bit.

> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>
> A more general comment about formatting: there are multiple places where
> formatting seems more apropos to Javascript or C++ than Python; one example:
>
> def test_get_action_args(self):
> env = {
> 'wsgiorg.routing_args': [None, {
> 'controller': None,
> 'format': None,
> 'action': 'update',
> 'id': 12,
> }],
> }
>
> Normally continued lines are to be minimized, and when necessary, should be at
> a consistent level of indentation. Most typical is 2 levels of indentation,
> but there are other 'standards', and it's most important to be consistent to
> make the code readable. Outdenting closing brackets/parens is discouraged in
> Python. You don't need to change the code, since this style is so prevalent in
> these scripts, but in the future we should strive for a more Pythonic and less
> Javascript-y style.

Definitely a great point. I think that example wouldn't look too bad if the list (that starts with [None,...) didn't have to be there. I think it is very ugly and I felt doing that was at least the lesser of two evils. I tend to keep my code rather clean (notice the whole branch is aimed at refactoring/cleaning things up), but you did catch me there :)

Thanks again for the thorough review. I hope I hit everything you mentioned.