launchpad-reviewers team mailing list archive

[Ursula Junque <ursinha@xxxxxxxxxxx>]

> 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.

Removed.

> 
> 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_tr
> anslation_templates.

Done. Not caring if distroseries is None though. Is that even possible?

> 
> 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?

Silly mistake, removed that.

> 
> 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.

Done! Raises an AssertionError now.
-- 
https://code.launchpad.net/~ursinha/launchpad/qa-bad-bug-1201485-rebased/+merge/191077
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.