launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

In ddtp_tarball.py ( line 23 of the diff) there is this line:

    _, component, _ = tarfile_path.split("_")

Since you are interested in underscores in the file name and not
the entire path, os.path.basename(tarfile_path).split("_") would be
better.

It would also be good to have some tests with underscores in the path
that show that the method handles them correctly.

Using the ValueError as an way of determining that the destructuring
assignment failed seems a bit indirect as far as input validation goes.

Why is getSeriesKey a classmethod when "cls" isn't used?  Maybe a
staticmethod is more appropriate.

How about this for getSeriesKey?

@staticmethod
def getSeriesKey(tarfile_path):
    fn = os.path.basename(tarfile_path)
    if fn.count('_') != 2:
        # The file name does not match the pattern, so no series key
        # can be extracted.
        return None
    return fn.split('_')[1]

-- 
https://code.launchpad.net/~cjwatson/launchpad/copy-ddtp/+merge/111688
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.