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

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

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

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

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

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

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

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.