Code review comment for lp:~al-maisan/launchpad/unembargo-443075

Abel Deuring wrote:
> Review: Approve
> Hi Muharem,
>
> nice work, just a few cosmetic remarks, see below.

Hello Abel,

I am thanking you for the prompt review. All your suggestion were
accommodated except for one. Please see the enclosed incremental diff as
well as my replies below for more detail.

>
> Abel
>
>> === modified file 'lib/lp/soyuz/model/queue.py'
>> --- lib/lp/soyuz/model/queue.py 2009-10-01 07:05:22 +0000
>> +++ lib/lp/soyuz/model/queue.py 2009-10-09 09:50:27 +0000
>> @@ -367,15 +367,6 @@
>> assert self.sources.count() == 1, (
>> 'Source is mandatory for delayed copies.')
>> self.setAccepted()
>> - # The second assert guarantees that we'll actually have a SPR.
>> - spr = self.mySourcePackageRelease()
>> - # Use the changesfile of the original upload.
>> - changes_file_object = StringIO.StringIO(
>> - spr.package_upload.changesfile.read())
>> - self.notify(
>> - announce_list=self.distroseries.changeslist,
>> - changes_file_object=changes_file_object, allow_unsigned=True)
>> - self.syncUpdate()
>>
>> def rejectFromQueue(self, logger=None, dry_run=False):
>> """See `IPackageUpload`."""
>> @@ -496,6 +487,7 @@
>> else:
>> return None
>>
>> + @property
>> def mySourcePackageRelease(self):
>> """The source package release related to this queue item.
>
> As a Property, this should be named my_source_package_release

Fixed.

>
>
>> === modified file 'lib/lp/soyuz/tests/test_packageupload.py'
>> --- lib/lp/soyuz/tests/test_packageupload.py 2009-09-29 17:16:01 +0000
>> +++ lib/lp/soyuz/tests/test_packageupload.py 2009-10-09 09:50:27 +0000
>> @@ -198,6 +198,9 @@
>> self.assertEquals(
>> PackageUploadStatus.ACCEPTED, delayed_copy.status)
>>
>> + # Make sure no announcement email was sent at this point.
>> + self.assertEquals(len(stub.test_emails), 0)
>> +
>> self.layer.txn.commit()
>> self.layer.switchDbUser(self.dbuser)
>>
>> @@ -210,22 +213,26 @@
>>
>> # Check the announcement email.
>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
>> - # This is now a MIMEMultipart message.
>> msg = message_from_string(raw_msg)
>> body = msg.get_payload(0)
>> body = body.get_payload(decode=True)
>>
>> - self.assertEquals(from_addr, '<email address hidden>')
>> self.assertEquals(
>> - to_addrs, ['<email address hidden>'])
>> + str(to_addrs), "['<email address hidden>']")
>>
>> expected_subject = (
>> '[ubuntutest/breezy-autotest-security]\n\t'
>> 'dist-upgrader_20060302.0120_all.tar.gz, foocomm 1.0-2 (Accepted)')
>> self.assertEquals(msg['Subject'], expected_subject)
>>
>> - self.assertTrue(body.startswith('foocomm (1.0-2) breezy; urgency=low'))
>> -
>> + self.assertEquals(body,
>> + 'foocomm (1.0-2) breezy; urgency=low\n\n'
>> + ' * Initial version\n\n'
>> + 'Date: Thu, 16 Feb 2006 15:34:09 +0000\n'
>> + 'Changed-By: Foo Bar <email address hidden>\n'
>> + 'Maintainer: Launchpad team <email address hidden>\n'
>> + 'http://launchpad.dev/ubuntutest/breezy-autotest/+source/foocomm/1.0-2\n')
>
> This line is a bit too long. Could you split itto two lines? (OK,
> breaking the URL into two line does ot look nice either...

With your permission I'd like to leave it as it is.

>> +
>
> Please remove the whitespace in the line above.

Done.

>
>> self.layer.switchDbUser('launchpad')
>>
>> # One source and 2 binaries are pending publication. They all were
>>
>> === modified file 'lib/lp/soyuz/tests/test_publishing.py'
>> --- lib/lp/soyuz/tests/test_publishing.py 2009-09-10 08:56:15 +0000
>> +++ lib/lp/soyuz/tests/test_publishing.py 2009-10-09 09:50:27 +0000
>> @@ -385,6 +385,41 @@
>> return [BinaryPackagePublishingHistory.get(pub.id)
>> for pub in secure_pub_binaries]
>>
>> + def _findFile(self, top, name_fragment):
>
> A name like _findChangesFile would be a better, I think. The method
> does not find any file having name_fragment in its name ;)

Suggestion happily adopted :)

>
>> + """File with given name fragment in directory tree starting at top."""
>> + for root, dirs, files in os.walk(top, topdown=False):
>> + for name in files:
>> + if name.endswith('.changes') and name.find(name_fragment) > -1:
>> + return os.path.join(root, name)
>> + return None
>> +
>> + def createSource(
>> + self, archive, sourcename, version, distroseries=None,
>> + new_version=None):
>> + """Create source with meaningful '.changes' file."""
>> + top = 'lib/lp/archiveuploader/tests/data/suite'
>> + name_fragment = '%s_%s' % (sourcename, version)
>> + changesfile_path = self._findFile(top, name_fragment)
>> +
>
> Please remove the whitespace in the line above.

Done.

>
>> + source = None
>> +
>> + if changesfile_path is not None:
>> + if new_version is None:
>> + new_version = version
>> + changesfile_content = ''
>> + handle = open(changesfile_path, 'r')
>> + try:
>> + changesfile_content = handle.read()
>> + finally:
>> + handle.close()
>> +
>
> Please remove the whitespace in the line above.

Done.

>
>> + source = self.getPubSource(
>> + sourcename=sourcename, archive=archive, version=new_version,
>> + changes_file_content=changesfile_content,
>> + distroseries=distroseries)
>> +
>
> Please remove the whitespace in the line above.

Done.

>
>> + return source
>> +
>>
>> class TestNativePublishingBase(unittest.TestCase, SoyuzTestPublisher):
>> layer = LaunchpadZopelessLayer
>>
>>

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC