launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

The proposal to merge lp:~cjwatson/launchpad/custom-upload-parsing into lp:launchpad has been updated.

Description changed to:

== Summary ==

In bug 827973, Jeroen noted that the custom uploads copier has code that is rather similar to the filename parsing in custom upload implementations, and that these should be unified.

== Proposed fix ==

Redesign the custom upload interface to support both its previous uses and the custom upload copier.

Note that this should make a subsequent fix for bug 827941 trivial.

== Implementation details ==

The custom upload constructor now takes no parameters, with these being passed in later.  This makes the new filename parsing methods implemented by each custom upload type (setTargetDirectory and getSeriesKey) make somewhat more sense, and avoids inelegant passing of None in tests.

I discarded the business where extractNameFields uses a default architecture of "all".  Firstly, this should now be handled by custom upload implementations if at all.  But secondly and more importantly, I know of no real-world case where it matters.  The copier only handles debian-installer and dist-upgrader uploads right now.  debian-installer uploads always have an architecture (e.g. https://launchpadlibrarian.net/106301545/debian-installer_20101020ubuntu144_i386.changes), and dist-upgrader uploads always have "all" (e.g. https://launchpadlibrarian.net/103130380/update-manager_0.156.14.1_i386.changes).  When we add support for ddtp-tarball uploads, they will simply return a different key (the component) rather than worrying about a default architecture.

I pushed the check for conflicts in the filesystem down into CustomUpload.  The ddtp-tarball implementation overrides this, since it doesn't care.

== LOC Rationale ==

+25 (my first attempt was +119, but its interface was the wrong shape).  However, this is part of an arc of work building on r15312, which was -199, so I'd like to claim credit against that.

== Tests ==

bin/test -vvct test_customupload -t test_debian_installer -t test_dist_upgrader -t test_ddtp_tarball -t test_distroseriesqueue -t test_custom_uploads_copier

== Demo and Q/A ==

None needed; this is refactoring.

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/custom-upload-parsing/+merge/107656
-- 
https://code.launchpad.net/~cjwatson/launchpad/custom-upload-parsing/+merge/107656
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/custom-upload-parsing into lp:launchpad.