)]}'
{"nova/virt/libvirt/config.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"bf6e01130a20dc94024941424e160585a7aeeb2e","unresolved":false,"context_lines":[{"line_number":893,"context_line":"                self.shareable \u003d True"},{"line_number":894,"context_line":"            elif c.tag \u003d\u003d \u0027address\u0027:"},{"line_number":895,"context_line":"                obj \u003d LibvirtConfigGuestDeviceAddress()"},{"line_number":896,"context_line":"                self.device_addr \u003d obj.parse_dom(c)"},{"line_number":897,"context_line":""},{"line_number":898,"context_line":""},{"line_number":899,"context_line":"class LibvirtConfigGuestDiskBackingStore(LibvirtConfigObject):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a18252c_d05c86ef","line":896,"updated":"2016-04-13 14:51:40.000000000","message":"The code is would be more something like:\n\n  if c.type \u003d\u003d \u0027pci\u0027\n    p \u003d LibvirtConfigGuestDeviceAddressPCI()\n    p.parse_dom(c)\n    self.device_addr \u003d p\n  else:\n    ....","commit_id":"0d54bc26a7becdea14ae96e2308a0827d26e6781"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"bf6e01130a20dc94024941424e160585a7aeeb2e","unresolved":false,"context_lines":[{"line_number":1136,"context_line":"                                                       **kwargs)"},{"line_number":1137,"context_line":""},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"class LibvirtConfigGuestDeviceAddress(LibvirtConfigObject):"},{"line_number":1140,"context_line":"    def __init__(self, **kwargs):"},{"line_number":1141,"context_line":"        super(LibvirtConfigGuestDeviceAddress, self).\\"},{"line_number":1142,"context_line":"                __init__(mode\u003d\u0027address\u0027, type\u003d\u0027pci\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a18252c_35604c01","line":1139,"updated":"2016-04-13 14:51:40.000000000","message":"I\u0027m not sure that is useful, you use that object as a factory method but it does not reflect a real config, also the method parse_dom should to be used to \"build\" the object itself (by convention of the other config objects).","commit_id":"0d54bc26a7becdea14ae96e2308a0827d26e6781"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"bf6e01130a20dc94024941424e160585a7aeeb2e","unresolved":false,"context_lines":[{"line_number":1144,"context_line":""},{"line_number":1145,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":1146,"context_line":"        addr_type \u003d xmldoc.get(\u0027type\u0027)"},{"line_number":1147,"context_line":"        if addr_type \u003d\u003d \u0027pci\u0027:"},{"line_number":1148,"context_line":"            dev_addr \u003d LibvirtConfigGuestDeviceAddressPCI()"},{"line_number":1149,"context_line":"            dev_addr.parse_dom(xmldoc)"},{"line_number":1150,"context_line":"        elif addr_type \u003d\u003d \u0027drive\u0027:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a18252c_500076aa","line":1147,"updated":"2016-04-13 14:51:40.000000000","message":"This logic should probably moved line 894 and 1402, at least an object here is not necessary.","commit_id":"0d54bc26a7becdea14ae96e2308a0827d26e6781"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"30ab314abb7e7ec350871344df26efff1ec2006d","unresolved":false,"context_lines":[{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"class LibvirtConfigGuestDeviceAddress(LibvirtConfigObject):"},{"line_number":1141,"context_line":""},{"line_number":1142,"context_line":"    @staticmethod"},{"line_number":1143,"context_line":"    def factory(xmldoc):"},{"line_number":1144,"context_line":"        obj \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a18252c_899d8d68","line":1141,"updated":"2016-04-14 13:12:03.000000000","message":"Hum... so the root_name here is \u0027address\u0027, you should to define the constructor like:\n\n  class LibvirtConfigGuestDeviceAddress(LibvirtConfigObject)\n    def __init__(self, type\u003dNone, **kwargs):\n      super(LibvirtConfigGuestDeviceAddress, self).__init__(\n        root_name\u003d\u0027address\u0027, **kwargs)\n      self.type \u003d type\n\nNow the children should to look like:\n\n  class LibvirtConfigGuestDeviceAddressDrive(LibvirtConfigGuestDeviceAddress):\n    def __init__(self, **kwargs):\n      super(LibvirtConfigGuestDeviceAddressDrive, self).__init__(\n        type\u003d\u0027drive\u0027, **kwargs)\n      \n      self.controller \u003d None\n      ...","commit_id":"667d63db20dcc31fa387c2cf72c95996011aed65"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"30ab314abb7e7ec350871344df26efff1ec2006d","unresolved":false,"context_lines":[{"line_number":1147,"context_line":"            obj \u003d LibvirtConfigGuestDeviceAddressPCI()"},{"line_number":1148,"context_line":"        elif addr_type \u003d\u003d \u0027drive\u0027:"},{"line_number":1149,"context_line":"            obj \u003d LibvirtConfigGuestDeviceAddressDrive()"},{"line_number":1150,"context_line":"        obj.addr_type \u003d addr_type"},{"line_number":1151,"context_line":"        return obj"},{"line_number":1152,"context_line":""},{"line_number":1153,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5a18252c_21f8d348","line":1150,"updated":"2016-04-14 13:12:03.000000000","message":"What is this attribute \u0027addr_type\u0027 you are setting to the object ? That should be not necessary","commit_id":"667d63db20dcc31fa387c2cf72c95996011aed65"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"f025b959a2815c700f3f19dd2c0bac7f5b65008f","unresolved":false,"context_lines":[{"line_number":1395,"context_line":"                        if sub.get(\u0027peak\u0027):"},{"line_number":1396,"context_line":"                            self.vif_outbound_peak \u003d int(sub.get(\u0027peak\u0027))"},{"line_number":1397,"context_line":"            elif c.tag \u003d\u003d \u0027address\u0027:"},{"line_number":1398,"context_line":"                obj \u003d LibvirtConfigGuestDeviceAddress.factory(c)"},{"line_number":1399,"context_line":"                obj.parse_dom(c)"},{"line_number":1400,"context_line":"                self.device_addr \u003d obj"},{"line_number":1401,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_0d815f8f","line":1398,"updated":"2016-05-23 04:46:54.000000000","message":"Honestly, the whole factory() thing just obfuscates the code for no real reason. Why not just create put the type parsing into this block here, just like the other elements are done above? That way all parsing logic is contained within this function. Something like this?\n\n elif c.tag \u003d \u0027address\u0027:\n     addr_type \u003d c.get(\u0027type\u0027)\n     if addr_type \u003d\u003d \u0027drive\u0027:\n         self.device_addr \u003d LibvirtConfigGuestDeviceAddressDrive()\n         self.device_addr.parse_dom(c)\n     elif addr_type \u003d\u003d \u0027pci\u0027:\n         self.device_addr \u003d LibvirtConfigGuestDeviceAddressPCI()\n         self.device_addr.parse_dom(c)","commit_id":"f005ae424f8626b48bf91b6f69ae406df71e0a4e"},{"author":{"_account_id":8802,"name":"Vladik Romanovsky","email":"vromanso@redhat.com","username":"vladikr"},"change_message_id":"30d73b3894099d1e75127b686842bc83a89bd6cb","unresolved":false,"context_lines":[{"line_number":1395,"context_line":"                        if sub.get(\u0027peak\u0027):"},{"line_number":1396,"context_line":"                            self.vif_outbound_peak \u003d int(sub.get(\u0027peak\u0027))"},{"line_number":1397,"context_line":"            elif c.tag \u003d\u003d \u0027address\u0027:"},{"line_number":1398,"context_line":"                obj \u003d LibvirtConfigGuestDeviceAddress.factory(c)"},{"line_number":1399,"context_line":"                obj.parse_dom(c)"},{"line_number":1400,"context_line":"                self.device_addr \u003d obj"},{"line_number":1401,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_e320f1f9","line":1398,"in_reply_to":"bab6814e_0d815f8f","updated":"2016-05-25 02:41:48.000000000","message":"We will just need to do the same on line 893 as well..\nIt looks to me that this factory is really straightforward.","commit_id":"f005ae424f8626b48bf91b6f69ae406df71e0a4e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"194b0ad805f0bbc85c81a14aa6be61c134345b06","unresolved":false,"context_lines":[{"line_number":1395,"context_line":"                        if sub.get(\u0027peak\u0027):"},{"line_number":1396,"context_line":"                            self.vif_outbound_peak \u003d int(sub.get(\u0027peak\u0027))"},{"line_number":1397,"context_line":"            elif c.tag \u003d\u003d \u0027address\u0027:"},{"line_number":1398,"context_line":"                obj \u003d LibvirtConfigGuestDeviceAddress.factory(c)"},{"line_number":1399,"context_line":"                obj.parse_dom(c)"},{"line_number":1400,"context_line":"                self.device_addr \u003d obj"},{"line_number":1401,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_a08696d8","line":1398,"in_reply_to":"bab6814e_e320f1f9","updated":"2016-05-31 14:20:27.000000000","message":"Yeah, I\u0027m not sure I get why the factory is not desirable.","commit_id":"f005ae424f8626b48bf91b6f69ae406df71e0a4e"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"eb2f13e273877e401093b5d88640077a6c5a745c","unresolved":false,"context_lines":[{"line_number":1143,"context_line":"        self.type \u003d type"},{"line_number":1144,"context_line":""},{"line_number":1145,"context_line":"    @staticmethod"},{"line_number":1146,"context_line":"    def factory(xmldoc):"},{"line_number":1147,"context_line":"        addr_type \u003d xmldoc.get(\u0027type\u0027)"},{"line_number":1148,"context_line":"        if addr_type \u003d\u003d \u0027pci\u0027:"},{"line_number":1149,"context_line":"            return LibvirtConfigGuestDeviceAddressPCI()"}],"source_content_type":"text/x-python","patch_set":14,"id":"9abb7d3a_57c02450","line":1146,"range":{"start_line":1146,"start_character":22,"end_line":1146,"end_character":24},"updated":"2016-06-01 09:07:38.000000000","message":"I think the design pattern is fine, but can we just call it \u0027create_from_dom\u0027 - \u0027factory\u0027 is just a meaningless buzzword.\n\nAlso, we should have explicit test cases for the device address parsing code - not merely test it as a side effect of parsing the \u003cdisk\u003e xml.","commit_id":"7b6f7ecfa6c7190d9136ed9287cfd86bdd5023d7"}]}