Hi Michael, thanks for your review! > Great! The addition of the is_root_device property makes the code much > more readable. > > As usual, just one or two comments below, in particular, one change for which > I couldn't see the cause... > > Thanks! > > > === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py' > > --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 > 13:22:14 +0000 > > +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 > 13:59:33 +0000 > > @@ -1644,7 +1644,7 @@ > > return None > > pci_subclass = scsi_controller.pci_subclass > > return self.pci_storage_subclass_hwbus.get(pci_subclass) > > - elif scsi_controller_bus == 'usb': > > + elif scsi_controller_bus in ('usb', 'usb_interface'): > > # USB storage devices have the following HAL device hierarchy: > > # - HAL node for the USB device. info.bus == 'usb_device', > > # device class == 0, device subclass == 0 > > @@ -1703,6 +1703,12 @@ > > return HWBus.PCI > > > > @property > > + def is_root_device(self): > > + """Return True is this is the root node of all devicese, else > False. > > + """ > > + raise NotImplementedError > > + > > + @property > > def raw_bus(self): > > """Return the device bus as specified by HAL or udev.""" > > raise NotImplementedError > > @@ -1719,11 +1725,11 @@ > > if result is not None: > > return result > > > > - if device_bus == 'scsi': > > + if device_bus in ('scsi', 'scsi_device'): > > return self.translateScsiBus() > > elif device_bus == 'pci': > > return self.translatePciBus() > > - elif self.udi == ROOT_UDI: > > + elif self.is_root_device: > > # The computer itself. In Hardy, HAL provides no info.bus > > # for the machine itself; older versions set info.bus to > > # 'unknown', hence it is better to use the machine's > > @@ -1731,7 +1737,7 @@ > > return HWBus.SYSTEM > > else: > > self.parser._logWarning( > > - 'Unknown bus %r for device %s' % (device_bus, self.udi)) > > + 'Unknown bus %r for device %s' % (device_bus, > self.device_id)) > > return None > > > > @property > > @@ -2275,6 +2281,11 @@ > > return result > > return self.getProperty('info.subsystem') > > > > + @property > > + def is_root_device(self): > > + """See `BaseDevice`.""" > > + return self.udi == ROOT_UDI > > + > > def getVendorOrProduct(self, type_): > > """Return the vendor or product of this device. > > > > @@ -2547,7 +2558,7 @@ > > # DEVTYPE. DEVTYPE is preferable. > > # The root device has the subsystem/bus value "acpi", which > > # is a bit nonsensical. > > - if self.device_id == UDEV_ROOT_PATH: > > + if self.is_root_device: > > return None > > properties = self.udev['E'] > > devtype = properties.get('DEVTYPE') > > @@ -2555,6 +2566,11 @@ > > return devtype > > return properties.get('SUBSYSTEM') > > > > + @property > > + def is_root_device(self): > > + """See `BaseDevice`.""" > > + return self.udev['P'] == UDEV_ROOT_PATH > > + > > def getVendorOrProduct(self, type_): > > """Return the vendor or product of this device. > > > > @@ -2565,7 +2581,7 @@ > > 'Unexpected value of type_: %r' % type_) > > > > bus = self.raw_bus > > - if self.device_id == UDEV_ROOT_PATH: > > + if self.is_root_device: > > # udev does not known about any product information for > > # the root device. We use DMI data instead. > > return self.root_device_ids[type_] > > @@ -2605,7 +2621,7 @@ > > 'Unexpected value of type_: %r' % type_) > > > > bus = self.raw_bus > > - if self.device_id == UDEV_ROOT_PATH: > > + if self.is_root_device: > > # udev does not known about any product information for > > # the root device. We use DMI data instead. > > if type_ == 'vendor': > > > > === modified file > 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py' > > --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py > 2009-10-15 13:22:14 +0000 > > +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py > 2009-10-15 13:59:33 +0000 > > @@ -1173,6 +1173,51 @@ > > parser.submission_key, > > "Unknown bus 'nonsense' for device " + self.UDI_PCCARD_DEVICE) > > > > + def testHALDevice_is_root_device_for_root_device(self): > > A few tests above use the style: > > def test_HALDevice_scsi_controller_no_parent(self): right, fixed. > > (I think from a previous review?). This seems more consistant with > > https://dev.launchpad.net/TestsStyleGuide#Python Test Cases > > > + """Test of HALDevice.is_root_device for the root device.""" > > + devices = [ > > + { > > + 'id': 1, > > + 'udi': self.UDI_COMPUTER, > > + 'properties': {}, > > + }, > > + ] > > + parsed_data = { > > + 'hardware': { > > + 'hal': { > > + 'devices': devices, > > + }, > > + }, > > + } > > + > > + parser = SubmissionParser() > > + parser.submission_key = 'Test of HALDevice.is_root_device' > > + parser.buildDeviceList(parsed_data) > > + > self.assertTrue(parser.hal_devices[self.UDI_COMPUTER].is_root_device) > > + > > + def testHALDevice_is_root_device_for_non_root_device(self): > > + """Test of HALDevice.is_root_device for a non-root device.""" > > + devices = [ > > + { > > + 'id': 1, > > + 'udi': self.UDI_PCCARD_DEVICE, > > + 'properties': {}, > > + }, > > + ] > > + parsed_data = { > > + 'hardware': { > > + 'hal': { > > + 'devices': devices, > > + }, > > + }, > > + } > > + > > + parser = SubmissionParser() > > + parser.submission_key = 'Test of HALDevice.is_root_device' > > + parser.buildDeviceList(parsed_data) > > + self.assertFalse( > > + parser.hal_devices[self.UDI_PCCARD_DEVICE].is_root_device) > > + > > def renameInfoBusToInfoSubsystem(self, devices): > > """Rename the property info.bus in a device list to info.subsystem. > > > > @@ -2837,7 +2882,19 @@ > > }, > > } > > > > - self.pci_scsi_controller_data = { > > + self.pci_pccard_bridge = { > > + 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0', > > + 'E': { > > + 'DRIVER': 'yenta_cardbus', > > + 'PCI_CLASS': '60700', > > + 'PCI_ID': '1217:7134', > > + 'PCI_SUBSYS_ID': '10CF:131E', > > + 'PCI_SLOT_NAME': '0000:08:03.0', > > + 'SUBSYSTEM': 'pci', > > + } > > + } > > + > > + self.pccard_scsi_controller_data = { > > 'P': > '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0', > > 'E': { > > 'DRIVER': 'aic7xxx', > > @@ -2857,6 +2914,12 @@ > > }, > > } > > > > + self.pci_bridge_pccard_hierarchy_data = [ > > + {'udev_data': self.root_device}, > > + {'udev_data': self.pci_pccard_bridge}, > > + {'udev_data': self.pccard_scsi_controller_data}, > > + ] > > + > > self.pci_scsi_controller_scsi_side_2 = { > > 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/' > > '0000:09:00.0/host6/scsi_host/host6'), > > @@ -2890,7 +2953,7 @@ > > } > > > > self.scsi_device_hierarchy_data = [ > > - {'udev_data': self.pci_scsi_controller_data}, > > + {'udev_data': self.pccard_scsi_controller_data}, > > Was this another problem with the test data? or something you updated > for an updated test? I changed the name because the device is indeed a PCCard, an I use it also in a new test of translatePciBus(), so I thought that the test data should get a more precise name. Abel @@ -1173,7 +1173,7 @@ parser.submission_key, "Unknown bus 'nonsense' for device " + self.UDI_PCCARD_DEVICE) - def testHALDevice_is_root_device_for_root_device(self): + def test_HALDevice_is_root_device_for_root_device(self): """Test of HALDevice.is_root_device for the root device.""" devices = [ { @@ -1195,7 +1195,7 @@ parser.buildDeviceList(parsed_data) self.assertTrue(parser.hal_devices[self.UDI_COMPUTER].is_root_device) - def testHALDevice_is_root_device_for_non_root_device(self): + def test_HALDevice_is_root_device_for_non_root_device(self): """Test of HALDevice.is_root_device for a non-root device.""" devices = [ {