[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad

2018-02-26 Thread noreply
The proposal to merge lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad has 
been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/ptuj-repr/+merge/338559
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp


Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad

2018-02-26 Thread Maximiliano Bertacchini
Makes sense, thanks.
-- 
https://code.launchpad.net/~cjwatson/launchpad/ptuj-repr/+merge/338559
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp


Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad

2018-02-26 Thread Colin Watson
That's because we're now using those properties in the parent class, 
specifically in the new PackageTranslationsUploadJobDerived.__repr__ method.

The whole *Derived thing for jobs is pretty confusing, but basically the job 
runner calls iterReady on an IJobSource to get the list of jobs it should run, 
and the returned instances (FooJobDerived) typically delegate to self.context 
(which is a FooJob) for the business logic of the job proper.  Now, I'm not a 
big fan of this layout because it's usually unnecessarily complicated, and in 
this case it's an artificial distinction because self.context == self; however, 
strictly speaking the job runner's logging is done using the FooJobDerived 
instance, so __repr__ belongs there and I therefore moved the properties it 
uses as well.
-- 
https://code.launchpad.net/~cjwatson/launchpad/ptuj-repr/+merge/338559
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp


Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad

2018-02-26 Thread Maximiliano Bertacchini
Review: Approve

Looks good to me. One question though: why move those @properties from 
`PackageTranslationsUploadJob` to its parent class 
`PackageTranslationsUploadJobDerived`?
-- 
https://code.launchpad.net/~cjwatson/launchpad/ptuj-repr/+merge/338559
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad.

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp


[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad

2018-02-22 Thread Colin Watson
Colin Watson has proposed merging lp:~cjwatson/launchpad/ptuj-repr into 
lp:launchpad.

Commit message:
Give PackageTranslationsUploadJob a more useful __repr__.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/ptuj-repr/+merge/338559

 is not the most useful __repr__ in the world.  
Replace this with something that tells us what source package name and series 
are involved.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~cjwatson/launchpad/ptuj-repr into lp:launchpad.
=== modified file 'lib/lp/soyuz/model/packagetranslationsuploadjob.py'
--- lib/lp/soyuz/model/packagetranslationsuploadjob.py	2015-07-09 20:06:17 +
+++ lib/lp/soyuz/model/packagetranslationsuploadjob.py	2018-02-22 14:55:33 +
@@ -1,4 +1,4 @@
-# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -8,16 +8,17 @@
 'PackageTranslationsUploadJob',
 ]
 
+import json
+
 from lazr.delegates import delegate_to
-import simplejson
 from zope.component import getUtility
 from zope.interface import (
 implementer,
 provider,
 )
 
+from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
-from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.services.config import config
 from lp.services.database.interfaces import IStore
 from lp.services.job.interfaces.job import JobType
@@ -82,13 +83,20 @@
 self.job = job
 self.context = self
 
+def __repr__(self):
+return "<%(job_class)s for %(source)s in %(series)s>" % {
+"job_class": self.__class__.__name__,
+"source": self.sourcepackagename.name,
+"series": self.distroseries,
+}
+
 @classmethod
 def create(cls, distroseries, libraryfilealias, sourcepackagename,
requester):
 job = Job(
 base_job_type=JobType.UPLOAD_PACKAGE_TRANSLATIONS,
 requester=requester,
-base_json_data=simplejson.dumps(
+base_json_data=json.dumps(
 {'distroseries': distroseries.id,
  'libraryfilealias': libraryfilealias.id,
  'sourcepackagename': sourcepackagename.id,
@@ -109,22 +117,17 @@
 return [format_address_for_person(self.requester)]
 return []
 
-
-@implementer(IPackageTranslationsUploadJob)
-@provider(IPackageTranslationsUploadJobSource)
-class PackageTranslationsUploadJob(PackageTranslationsUploadJobDerived):
-
 @property
 def distroseries_id(self):
-return simplejson.loads(self.base_json_data)['distroseries']
+return json.loads(self.base_json_data)['distroseries']
 
 @property
 def libraryfilealias_id(self):
-return simplejson.loads(self.base_json_data)['libraryfilealias']
+return json.loads(self.base_json_data)['libraryfilealias']
 
 @property
 def sourcepackagename_id(self):
-return simplejson.loads(self.base_json_data)['sourcepackagename']
+return json.loads(self.base_json_data)['sourcepackagename']
 
 @property
 def distroseries(self):
@@ -138,6 +141,11 @@
 def sourcepackagename(self):
 return getUtility(ISourcePackageNameSet).get(self.sourcepackagename_id)
 
+
+@implementer(IPackageTranslationsUploadJob)
+@provider(IPackageTranslationsUploadJobSource)
+class PackageTranslationsUploadJob(PackageTranslationsUploadJobDerived):
+
 def attachTranslationFiles(self, by_maintainer):
 distroseries = self.distroseries
 sourcepackagename = self.sourcepackagename

=== modified file 'lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py'
--- lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py	2018-02-02 03:14:35 +
+++ lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py	2018-02-22 14:55:33 +
@@ -90,6 +90,13 @@
 self.assertTrue(verifyObject(IPackageTranslationsUploadJobSource,
  job_source))
 
+def test___repr__(self):
+_, sp, job = self.makeJob()
+self.assertEqual(
+"" % (
+sp.sourcepackagename.name, sp.distroseries),
+repr(job))
+
 def test_iterReady(self):
 _, _, job1 = self.makeJob()
 removeSecurityProxy(job1).job._status = JobStatus.COMPLETED

___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp