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

Revision history for this message
Brian Waldon (bcwaldon) wrote :

> type() has known limitations with old-style classes; isinstance() does not.
> Additionally, type() check does not take into account subclassing;
> isinstance() does. There is no clean way to check for a string object with
> type() due to types.StringType and types.UnicodeType; you can use
> isinstance(val, basestring) to check for all string types.

So the best thing to do in this case would be to use the built-in interfaces. First assume the 'data' variable is a dict and attempt to use it as such. When that doesn't work out, assume it is a list. When that doesn't work we end up just adding it as an xml text node. I'm going to stand by my point that I don't want to touch the code you are referring to since it is serving its purpose without issue. If someone wants to refactor the actual xml-specific serializer, I welcome it :)

I went to add a TODO on the third use of type(), and realized there is already a note there mentioning a need to refactor.

« Back to merge proposal