launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Thanks for the fixes. Very nearly there!

44	+ def process(self, packageupload, libraryfilealias):
45	+
46	+ if packageupload.package_name is None:

That blank line shouldn't be there.

67	+ only_templates = spr.sourcepackage.has_sharing_translation_templates

I'd calculate this in the job, rather than passing it in. You can calculate it there by doing distroseries.getSourcePackage(sourcepackagename).has_sharing_translation_templates.

91	+ @property
92	+ def package_name(self):
93	+ if hasattr(self, "_package_name"):
94	+ return self._package_name
95	+ return None

Couldn't this just be "package_name = None", then adjust the bits that set self._package_name to just set self.package_name?

121	+ def _findSourcePublication(self, packageupload):
122	+ """Find destination source publishing record of the packageupload."""
123	+ return packageupload.archive.getPublishedSources(
124	+ name=self.package_name, exact_match=True,

This probably wants to crash if self.package_name is None, otherwise it'll find the latest SPPH for *any* package. That case should never happen, but we don't want to do the wrong thing if it somehow does.
-- 
https://code.launchpad.net/~ursinha/launchpad/qa-bad-bug-1201485-rebased/+merge/191077
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.