Code review comment for lp:~adeuring/launchpad/hwdb-refactor-haldevice

Hi Michael,

thanks for the review!

On 01.10.2009 10:15, Michael Nelson wrote:
> Review: Approve code
>> This branch is a first step to refactor class HALDevice in
>> l/c/l/scripts/hwdbsubmissions.py
>>
> ...
>
> r=me, just a few small copy-n-paste/ordering issues below.
>
> Thanks for the detailed description! Makes much more sense now :) It's
> nice seeing the preparation for the new UDevDevice in a separate small
> branch!
>
>
>> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
>> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-09-29 15:05:09 +0000
>> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-01 07:42:22 +0000
>> @@ -7,7 +7,7 @@
>> data and for the community test submissions.
>> """
>>
>> -
>> +__metaclass__ = type
>> __all__ = [
>> 'SubmissionParser',
>> 'process_pending_submissions',
>> @@ -1235,59 +1235,29 @@
>> root_device.createDBData(submission, None)
>> return True
>>
>> -class HALDevice:
>> - """The representation of a HAL device node."""
>> -
>> - def __init__(self, id, udi, properties, parser):
>> - """HALDevice constructor.
>> -
>> - :param id: The ID of the HAL device in the submission data as
>> - specified in <device id=...>.
>> - :type id: int
>> - :param udi: The UDI of the HAL device.
>> - :type udi: string
>> - :param properties: The HAL properties of the device.
>> - :type properties: dict
>> - :param parser: The parser processing a submission.
>> - :type parser: SubmissionParser
>> - """
>> - self.id = id
>> - self.udi = udi
>> - self.properties = properties
>> +
>> +class BaseDevice:
>> + """A base class to represent device data from HAL and udev."""
>
> There should be a blank line here? (like all the other classes in this file).

Fixed.

>
>> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
>> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-06-25 05:30:52 +0000
>> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-01 07:42:22 +0000
>> @@ -380,6 +380,51 @@
>> 'Unexpected value of HALDevice.parent_udi, '
>> 'when no parent information available.')
>>
>> + def testHALDeviceDeviceId(self):
>
> I'm guessing you're aware that these should be pep8-style names,
>
> https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
>
> but in this case the rest of the file uses camel-case, so for consistency...
>
>> + """Test of HALDevice.device_id."""
>> + properties = {}
>> + parser = SubmissionParser(self.log)
>> + device = HALDevice(1, '/some/udi/path', properties, parser)
>> + self.assertEqual(
>> + '/some/udi/path', device.device_id,
>> + 'Unexpected value of HALDevice.parent_udi')
>
> Should this be 'Unexpected value of HALDevice.device_id (which actually
> returns self.udi not parent_udi?)

oops, sure. Fixed.

>
>> +
>> + def testHALDevicePciClass(self):
>> + """Test of HALDevice.pci_class."""
>> + properties = {
>> + 'pci.device_class': (1, 'int'),
>> + }
>> + parser = SubmissionParser(self.log)
>> + device = HALDevice(1, '/some/udi/path', properties, parser)
>> + self.assertEqual(
>> + device.pci_class, 1,
>
> Hardly worth mentioning, but the expected value should be first like
> your previous test above.

Right. There are two challenges for me: Personally, I prefer the order
exiting value, expected value ordering, and most of the existing tests
use this order... Fixed.

>
>> + 'Unexpected value of HALDevice.device_class.')
>
> Again, HALDevice.pci_class?

fixed.

>
>> +
>> + properties = {}
>> + parser = SubmissionParser(self.log)
>> + device = HALDevice(1, '/some/udi/path', properties, parser)
>> + self.assertEqual(
>> + device.pci_class, None,
>
> ordering...

Fixed.

>
>> + 'Unexpected value of HALDevice.device_class for Non-PCI device.')
>
> and pic_class?

Fixed.

>
>> +
>> + def testHALDevicePciSubClass(self):
>> + """Test of HALDevice.pci_subclass."""
>> + properties = {
>> + 'pci.device_subclass': (1, 'int'),
>> + }
>> + parser = SubmissionParser(self.log)
>> + device = HALDevice(1, '/some/udi/path', properties, parser)
>> + self.assertEqual(
>> + device.pci_subclass, 1,
>
> ordering...

Fixed.

>
>> + 'Unexpected value of HALDevice.device_class.')
>
> and pci_subclass?

Fixed.

>
>> +
>> + properties = {}
>> + parser = SubmissionParser(self.log)
>> + device = HALDevice(1, '/some/udi/path', properties, parser)
>> + self.assertEqual(
>> + device.pci_subclass, None,
>
> ordering...

Fixed.

>
>> + 'Unexpected value of HALDevice.device_class for Non-PCI device.')
>
> and pci_subclass?

Fixed.

Abel

>
>> +
>> def testHalDeviceRawBus(self):
>> """test of HALDevice.raw_bus."""
>> properties = {
>
> Great! All trivialities :)
>
> Thanks Abel.