Code review comment for lp:~cody-somerville/launchpad/ppa-deletion

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Cody, thanks for all these changes it's looking great! There's just a couple of minor things to fix:

In lib/lp/archivepublisher/tests/test_publisher.py:

101 + open(os.path.join(
102 + publisher._config.archiveroot, 'test_file'), 'w').close()

Please only tab in 4 chars, not 8.

In lib/lp/soyuz/scripts/publishdistro.py:

132 + archives = [archive for archive in archives if archive.publish
133 + or archive.status == ArchiveStatus.DELETING]

Our coding guidelines say to have a newline after the opening "["

So format it like this, which I think is more readable:

    archives = [
        archive for archive in archives
        if archive.publish or archive.status == ArchiveStatus.DELETING]

There's also a bug as I discussed on IRC:

158 + try_and_commit("deleting archive", publisher.deleteArchive,
159 + options.careful or options.careful_publishing)

Should not have the extra args for deleteArchive().

Finally:

160 + else:
161 + # Other types of archives do not currently support deletion.
162 + pass

Please log a warning here.

After these changes it's good to land, thanks for the great job!

J

review: Approve

« Back to merge proposal