Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Jeroen T. Vermeulen
+def runGerminate(self, override_file, series_name, arch, flavours, + structures): Rightly or wrongly, the “run” in that name led me to expect that this method would fire up a separate process. Maybe a very short docstring to dispel that notion? +germinator =

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Jeroen T. Vermeulen
Getting a better feel for the structure of runGerminate now. Which makes me wonder: +# Generate apt-ftparchive extra overrides for Build-Essential +# fields. +if ('build-essential' in structure.names and +flavour == flavours[0]): +

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Colin Watson
I think you're right that the file shouldn't be put in place if the context exits with an exception. If I were writing this without context managers, then rather than: with AtomicFile('foo') as handle: print handle, 'bar' ... I'd probably write: handle = open('foo.new', 'w')

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Jeroen T. Vermeulen
Ah, I see now why you can't do that: writeOverrides is a closure. It picks up “structure” and “arch” from the surrounding method at its point of definition. There's another easy thing to break by accident! The structure should again be structures[flavours[0]], I guess. Might it be

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Colin Watson
On Wed, Dec 07, 2011 at 07:20:29AM -, Jeroen T. Vermeulen wrote: The part from “series = None” onward seems to be an isolated unit of work. I think it looks for the first distroseries in development state, or failing that, the first in frozen state. But the structure of the code makes

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Jeroen T. Vermeulen
Some very nice tests there! To continue: === added file 'lib/lp/archivepublisher/tests/test_generate_extra_overrides.py' +def file_contents(path): +Return the contents of the file at path. +with open(path) as handle: +return handle.read() Since you're only reading the file,

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad

2011-12-07 Thread Colin Watson
[I've not responded to your entire review point-by-point, but I believe I've addressed all points where I haven't given an explicit response.] On Wed, Dec 07, 2011 at 09:22:24AM -, Jeroen T. Vermeulen wrote: +def runGerminate(self, override_file, series_name, arch, flavours, +

[Launchpad-reviewers] [Merge] lp:~cjwatson/meta-lp-deps/python-germinate into lp:meta-lp-deps

2011-12-07 Thread Colin Watson
Colin Watson has proposed merging lp:~cjwatson/meta-lp-deps/python-germinate into lp:meta-lp-deps. Requested reviews: Launchpad code reviewers (launchpad-reviewers) Related bugs: Bug #899972 in Launchpad itself: cron.germinate is very slow https://bugs.launchpad.net/launchpad/+bug/899972

[Launchpad-reviewers] [Merge] lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad

2011-12-07 Thread Aaron Bentley
Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-reset-to-default into lp:launchpad. Requested reviews: Launchpad code reviewers (launchpad-reviewers) Related bugs: Bug #892211 in Launchpad itself: reset to default in bug listing visibility widget doesn't work

[Launchpad-reviewers] [Merge] lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad

2011-12-07 Thread Curtis Hovey
Curtis Hovey has proposed merging lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad. Requested reviews: Launchpad code reviewers (launchpad-reviewers) Related bugs: Bug #901332 in Launchpad itself: AttributeError: 'SourcePackage' object has no attribute

[Launchpad-reviewers] [Merge] lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad

2011-12-07 Thread Curtis Hovey
The proposal to merge lp:~sinzui/launchpad/nomminate-driver-permissions-1 into lp:launchpad has been updated. Description changed to: Allow driver permission checks on source packages. Launchpad bug: https://bugs.launchpad.net/bugs/901332 Pre-implementation: No one SourcePackage does

[Launchpad-reviewers] [Merge] lp:~mbp/launchpad/featurefixture-from-request into lp:launchpad

2011-12-07 Thread Martin Pool
Martin Pool has proposed merging lp:~mbp/launchpad/featurefixture-from-request into lp:launchpad. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~mbp/launchpad/featurefixture-from-request/+merge/84887 At the moment the test

[Launchpad-reviewers] [Merge] lp:~wgrant/launchpad/drop-unused-views into lp:launchpad

2011-12-07 Thread William Grant
William Grant has proposed merging lp:~wgrant/launchpad/drop-unused-views into lp:launchpad. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~wgrant/launchpad/drop-unused-views/+merge/84888 There are lots of unused views and

[Launchpad-reviewers] [Merge] lp:~wgrant/launchpad/drop-unused-views into lp:launchpad

2011-12-07 Thread William Grant
The proposal to merge lp:~wgrant/launchpad/drop-unused-views into lp:launchpad has been updated. Commit Message changed to: Delete various unused views and templates. For more details, see: https://code.launchpad.net/~wgrant/launchpad/drop-unused-views/+merge/84888 --

[Launchpad-reviewers] [Merge] lp:~lifeless/python-oops-tools/prune into lp:python-oops-tools

2011-12-07 Thread Robert Collins
Robert Collins has proposed merging lp:~lifeless/python-oops-tools/prune into lp:python-oops-tools. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~lifeless/python-oops-tools/prune/+merge/84892 Pruning too many oopses at

[Launchpad-reviewers] [Merge] lp:~mbp/launchpad/show-timeline into lp:launchpad

2011-12-07 Thread Martin Pool
The proposal to merge lp:~mbp/launchpad/show-timeline into lp:launchpad has been updated. Status: Needs review = Approved For more details, see: https://code.launchpad.net/~mbp/launchpad/show-timeline/+merge/80166 -- https://code.launchpad.net/~mbp/launchpad/show-timeline/+merge/80166 Your