Code review comment for lp:~cjwatson/launchpad/copy-ddtp

On Mon, Jun 25, 2012 at 06:25:23PM -0000, Benji York wrote:
> 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.

Yes, indeed. Much of this is changing anyway as a result of feedback on
https://code.launchpad.net/~cjwatson/launchpad/custom-uefi/+merge/111626;
as a result I think I'm going to put this branch on temporary hold until
that one's ready to land, rather than doing much the same work in two
branches. (For instance recent work on that branch means that
getSeriesKey has to be a classmethod, although a helper method it calls
can probably become a staticmethod.)

> Using the ValueError as an way of determining that the destructuring
> assignment failed seems a bit indirect as far as input validation goes.
[...]
> @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]

I don't much like the look-before-you-leap pattern here, TBH; it seems
easy for it to get out of sync as code mutates over time, especially
since "fn.count('_') != 2" and "fn.split('_')[1]" are spelled just
differently enough to make it easy to forget to change one of them.