Launchpad itself

Code review comment for lp:~michael.nelson/launchpad/distro-series-difference-model

  1. distro-series-difference-model
  2. Merge into db-devel
Revision history for this message
Michael Nelson (michael.nelson) wrote :

On Mon, Aug 30, 2010 at 7:18 PM, Aaron Bentley <email address hidden> 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.

changes_aaron.diff
1=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
2--- lib/lp/registry/model/distroseriesdifference.py 2010-08-30 15:09:17 +0000
3+++ lib/lp/registry/model/distroseriesdifference.py 2010-08-31 07:16:47 +0000
4@@ -128,14 +128,14 @@
5 """See `IDistroSeriesDifference`."""
6 if self.source_pub:
7 return self.source_pub.source_package_version
8- return ''
9+ return None
10
11 @property
12 def parent_source_version(self):
13 """See `IDistroSeriesDifference`."""
14 if self.parent_source_pub:
15 return self.parent_source_pub.source_package_version
16- return ''
17+ return None
18
19 def updateStatusAndType(self):
20 """See `IDistroSeriesDifference`."""
21
22=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
23--- lib/lp/registry/tests/test_distroseriesdifference.py 2010-08-30 15:09:17 +0000
24+++ lib/lp/registry/tests/test_distroseriesdifference.py 2010-08-31 07:36:32 +0000
25@@ -130,7 +130,7 @@
26 difference_type=(
27 DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES))
28
29- self.assertEqual('', ds_diff.source_version)
30+ self.assertEqual(None, ds_diff.source_version)
31
32 def test_updateStatusAndType_resolves_difference(self):
33 # Status is set to resolved when versions match.
34@@ -148,7 +148,7 @@
35
36 was_updated = ds_diff.updateStatusAndType()
37
38- self.assertIs(True, was_updated)
39+ self.assertTrue(was_updated)
40 self.assertEqual(
41 DistroSeriesDifferenceStatus.RESOLVED,
42 ds_diff.status)
43@@ -171,7 +171,7 @@
44
45 was_updated = ds_diff.updateStatusAndType()
46
47- self.assertIs(True, was_updated)
48+ self.assertTrue(was_updated)
49 self.assertEqual(
50 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
51 ds_diff.status)
52@@ -195,7 +195,7 @@
53
54 was_updated = ds_diff.updateStatusAndType()
55
56- self.assertIs(False, was_updated)
57+ self.assertFalse(was_updated)
58 self.assertEqual(
59 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
60 ds_diff.status)
61@@ -220,7 +220,7 @@
62
63 was_updated = ds_diff.updateStatusAndType()
64
65- self.assertIs(True, was_updated)
66+ self.assertTrue(was_updated)
67 self.assertEqual(
68 DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
69 ds_diff.difference_type)

« Back to merge proposal