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.