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

Revision history for this message
Ed Leafe (ed-leafe) wrote :

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

If you add a #TODO to those lines, sure.

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

Trick: what you do in those cases is write it:
    except Exception as e:
        print "EXCEPTION", type(e)
...and then run the test for that code, passing in the bad condition that would trigger the exception. Once it's triggered, you'll know the type, and can change the code to catch the correct exception class.

I still think that you should change the lines (1787 and 1881) from:
    try:
        action_method = getattr(self, action)
    except (AttributeError, TypeError):
        action_method = self.default
to:
    action_method = getattr(self, action, self.default)
After all, that's what the default parameter for getattr() is there for!

« Back to merge proposal