launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Review: Approve code

Hi Colin,

What an interesting branch!  Well, not the branch per se but the whole endeavor.  I knew this work was coming but hadn't paid attention to the details so it was a good read.

<supertrivial>
In lib/lp/archivepublisher/uefi.py the items in __all__ should be alphabetized.
</supertrivial>

The comment in uefi.py:

The filename should be something like:
        <TYPE>_<VERSION>_<ARCH>.tar.gz

seems a bit breezy to me.  When I first read it I thought the naming convention was optional.  Should the description really read "The filename must be of the form:" ?

In fact, if the filename does not follow that pattern there could be problems in setTargetDirectory.  Perhaps there needs to be checks in there and a test to exercise it.

On a related note, it looks like setTargetDirectory and getSeriesKey both embed the same knowledge about the structure of the filename.  A common method could be used by both so that the filename parsing (simple as it is) isn't repeated.

I know it was not your doing, but I also know you share an aversion to poor spelling, so could you fix the occurrences of 'wheter' in lib/lp/soyuz/interfaces/queue.py (and perhaps the one other place it appears).

Thanks Colin.


-- 
https://code.launchpad.net/~cjwatson/launchpad/custom-uefi/+merge/111626
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.