launchpad-reviewers team mailing list archive

[Michael Nelson <michael.nelson@xxxxxxxxxxxxx>]

On Mon, Aug 30, 2010 at 7:18 PM, Aaron Bentley <aaron@xxxxxxxxxxxxx> wrote:
> Why are source_version and parent_source_version returning ''?  I would have expected them to return None if there was no such value.

They were originally returning None (as you saw by the test comment
below), but I'd thought for comparisons etc., it would be more
consistent if these properties always resulted in a string - an empty
one if there is no version. Anyway, I've switched them back to return
None.

>
> Why not implement getComments directly instead of indirecting through IDistroSeriesDifferenceCommentSource?

Because there's no reason why DistroSeriesDifference needs to know
about the implementation of DistroSeriesDifferenceComment ("Program to
interfaces, not implementations", "Depend on abstractions, do not
depend on concrete classes", etc.)

Currently DistroSeriesDifference doesn't import any model code, nor
does it do any storm queries directly. I thought that was a good
thing, but can change it if you want.

>
> Your test comments say None, but test for the empty string.  Please fix either the test or the comment.

Tests updated to expect None.

>
> Why assertIs(True, was_updated) instead of assertTrue(was_updated)?  We should only care about whether the returned value evaluates to True.  We should definitely not care about its identity.

My bad.

>
> Why assertIs(True, was_updated) instead of assertTrue(ds_diff.updateStatusAndType())?  It is shorter, so easier to read.

Ditto.

Thanks Aaron.

-- 
https://code.launchpad.net/~michael.nelson/launchpad/distro-series-difference-model/+merge/34086
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/distro-series-difference-model into lp:launchpad.

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2010-08-30 15:09:17 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2010-08-31 07:16:47 +0000
@@ -128,14 +128,14 @@
         """See `IDistroSeriesDifference`."""
         if self.source_pub:
             return self.source_pub.source_package_version
-        return ''
+        return None
 
     @property
     def parent_source_version(self):
         """See `IDistroSeriesDifference`."""
         if self.parent_source_pub:
             return self.parent_source_pub.source_package_version
-        return ''
+        return None
 
     def updateStatusAndType(self):
         """See `IDistroSeriesDifference`."""

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2010-08-30 15:09:17 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2010-08-31 07:36:32 +0000
@@ -130,7 +130,7 @@
             difference_type=(
                 DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES))
 
-        self.assertEqual('', ds_diff.source_version)
+        self.assertEqual(None, ds_diff.source_version)
 
     def test_updateStatusAndType_resolves_difference(self):
         # Status is set to resolved when versions match.
@@ -148,7 +148,7 @@
 
         was_updated = ds_diff.updateStatusAndType()
 
-        self.assertIs(True, was_updated)
+        self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceStatus.RESOLVED,
             ds_diff.status)
@@ -171,7 +171,7 @@
 
         was_updated = ds_diff.updateStatusAndType()
 
-        self.assertIs(True, was_updated)
+        self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             ds_diff.status)
@@ -195,7 +195,7 @@
 
         was_updated = ds_diff.updateStatusAndType()
 
-        self.assertIs(False, was_updated)
+        self.assertFalse(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             ds_diff.status)
@@ -220,7 +220,7 @@
 
         was_updated = ds_diff.updateStatusAndType()
 
-        self.assertIs(True, was_updated)
+        self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
             ds_diff.difference_type)