#launchpad-reviews 2010-02-01
<mwhudson> hm
<mwhudson> i don't suppose anyone is here?\
 * henninge is OCR but cannot change the channel subject anymore. Wonder why ...
<intellectronica> henninge: likewise :-/
<henninge> intellectronica: they may have fiddled with the settings last week during UDW.
<henninge> s/during/for/
<intellectronica> maybe
<intellectronica> henninge: sorry for not having started with reviewing yet. i'm going for a fun little visit at the dentist's soon, but will be reviewing when i'm back, around lunchtime
<henninge> intellectronica: I see. I will have to go out in the afternoon, too ... ;-)
<intellectronica> henninge: cool, so at least we're doing shifts
<bac> henninge: did you pursue the channel topic problem at all?  we need to get that resolved.  i can't change the topic in #launchpad either
<henninge> bac: no, I didn't. I don't have any clue were to start.
<bac> me neither
<henninge> s/were/where/
<bac> i guess i'll try in #freenode
<henninge> bac: I was able to change to topic on #launchapad-dev, btw.
<bac> me too
<bac> i'm seeing if our crack #is staff can help
<adeuring> intellectronica: do you know why we projoin the tables Product and SourcePackageName in BugtaskSet.search()?
<adeuring> intellectronica: soory, wrong channel...
<bac> henninge: can you have a look at https://code.launchpad.net/~bac/launchpad/bug-515171/+merge/18385 ? it is super trivial
<henninge> bac: r=me ;)
<bac> henninge: thanks
<jamalta> EdwinGrubbs: ping
<EdwinGrubbs> jamalta: hi
<jamalta> EdwinGrubbs: hi, i had noted in my calendar to bug you today about landing an MP.. do you have some time?
<jamalta> here's the link https://code.edge.launchpad.net/~jamalta/launchpad/relatedbugs-118609/+merge/18149
<EdwinGrubbs> jamalta: I'll try to land it. I'll let you know when it is in pqm.
<jamalta> EdwinGrubbs: thanks
<intellectronica> 1 2 3 test
<Ursinha> intellectronica: tested
<intellectronica> Ursinha: thanks :)
<Ursinha> intellectronica: :)
<andrea-bs> Hello everybody. I've just pushed a very small branch that's ready for review: https://code.edge.launchpad.net/~andrea-bs/launchpad/fix-411007/+merge/18389
<abentley> andrea-bs: approved.  Once it has passed the test suite, I will be happy to land it for you.
<andrea-bs> abentley: thanks!
<andrea-bs> abentley: oops! I've just noted that the branch has conflicts
<abentley> andrea-bs, I don't see any conflicts in the text.  I think the web page is bugged.
<andrea-bs> abentley: yes, there's a bug in LP. I'm going to report it
<Rinchen> well bac, in order to fix this I need to be an op here
<Rinchen> so I'll need to get help for that
<jpds> Rinchen: The channel isn't registered to begin with...
<bac> Rinchen: yeah, what jpds said
<jpds> Rinchen: Hi! too.
<Rinchen> jpds: yeah wasn't on the list when I was with LP to setup
<jpds> Rinchen: Poke someone on: /who freenode/staff/*
<bac> Rinchen: i think people just create some channels without knowing there is a procedure to register them
 * bac didn't know...
<Ursinha> Rinchen: is the channel registered?
<Rinchen> no Ursinha 
<Ursinha> Rinchen: if not, you can register and you're the op
<Ursinha> then set guard mode on and give op to others
<maxb> You can only register a channel whilst you *are* an op of it
<Ursinha> maxb: you're right, if the channel has people on it...
<Ursinha> that was easy :)
 * Rinchen has connections
<maxb> #launchpad-foundations is in need of the same treatment
<Rinchen> bac: making progress but getting an error from chanserv setting privs
<abentley> Rinchen, I am an OCR today, so I'll want to be able to change the topic, whether that requires ops or whatever.
<Rinchen> abentley, bac - try oping yourselves please
<jpds> Rinchen: /mode -t
<abentley> Rinchen, it doesn't work yet.
 * bac power!
<Rinchen> super thanks bac
<bac> Rinchen: you did the hard work
<Rinchen> I need to finish a comparison against #launchpad but it appears everyone should have the same access both places
<bac> Rinchen: did you change the mode to -t ?
<bac> intellectronica: could you try changing the topic here?
<Rinchen> I had an error in the channel flags
<Rinchen> you'll need to reop bac
<intellectronica> bac: i am told that i am not channel operator
<abentley> Same here.
<jpds> intellectronica: I think you need to identify to NickServ.
 * abentley is already identified.
<bac> intellectronica: please try now
<bac> to change topic
<intellectronica> i am still not channel operator
<Rinchen> intellectronica: the hash is set to those with @canonical or @freenode nicks
<jpds> abentley: But no c/lp cloak. :-/
<bac> oh, i see chanserv put it back to +t right after i did -t
<abentley> jpds, been on freenode years before I was with Canonical.
<bac> damn
<abentley> bac, maybe if you ask ChanServ to set the mode?
<bac> abentley: how?
<EdwinGrubbs> jamalta: I'm running the full suite of tests even though it is unlikely that anything broke with that small change. You can see the progress here: http://ec2-174-129-125-7.compute-1.amazonaws.com/
<abentley> bac, I think the flags command would do it.
<bac> abentley: even though i'm an op it says i am unauthorized
<jpds> Rinchen: /msg ChanServ set #launchpad-reviews mlock +nc-tslk
<Rinchen> jpds: already done
<Rinchen> jpds: as in, I removed it
<jpds> Now -t should be removable.
<jamalta> EdwinGrubbs: thanks for the update, will try and take a look at the link
<al-maisan> anybody up for a review?
<al-maisan> https://code.edge.launchpad.net/~al-maisan/launchpad/ejdt-n-1/+merge/18391
<bac> jtv: ping
<jtv> bac: hi
<jtv> al-maisan: your mp reports conflicts
<al-maisan> I can merge devel trunk w/o conflicts though
<al-maisan> .. and the other way around: I can merge the branch under review into a fresh devel branch w/o conflicts
<abentley> jtv, if there were actually conflicts, the affected files would be listed immediately below.
<jtv> abentley: then what does it mean for the mp to say that there are conflicts?
<al-maisan> yeah .. it's quite confusing
<abentley> jtv, it means there's a bug, and that the web page treats anything that's not None as conflicts, including ''.
<jtv> abentley: ah, known bug?
<abentley> jtv, yes.
<jtv> abentley: thanks for explaining.
<jtv> al-maisan: minor Ã¦sthetic note: in get_builder_data, ll. 50â73, consider moving the computation of totals outside the loop...  sum([count for ((processor, virtualized), count) in builder_stats.iteritems() if virtualized == True])
<jtv> Although if that's making things too complicated, nm.
<al-maisan> jtv: let me take a look
<jtv> good job catching what looked like an accidental interaction between "processor" and "virtualized" in the SQL composition code btw.
<al-maisan> jtv: thanks :)
<jtv> al-maisan: on another note, there is a way to stormify non-object queries ("SELECT count(*) FROM foo" and such)  though I don't recall the syntax off the top of my head.
<al-maisan> hmm .. hmm..
<jtv> Bad example; I meant things like "SELECT bar, count(*) FROM foo ORDER BY bar"
<al-maisan> "GROUP BY"?
<jtv> Sorry, yes.
<jtv> al-maisan: for ll. 298â299, the head_job_platform query, can't you just write "return store.execute(query).first()"?
<al-maisan> jtv: I guess so, that should be better.
<jtv> oh, dinner gong
<al-maisan> jtv-eat: enjoy your dinner :)
<al-maisan> s/first/get_one/
<al-maisan> jtv-eat: http://pastebin.ubuntu.com/366981/
 * al-maisan is being called to dinner as well
<jtv> al-maisan: in line 409, getEstimatedJobStartTime reads builder_stats[platform], where builder_stats comes out of get_builder_data.  Is it absolutely certain that that entry exists in the dict?
<al-maisan> jtv, just a second
<al-maisan> jtv: yes
<al-maisan> jtv: ..and if not a 0 will be returned
<al-maisan> since I am using a defaultdict foir builder statistics
<jtv> oic
<al-maisan> *for
<jtv> al-maisan: I wonder if maybe the normalization of head_job_platform should move into _getHeadJobPlatform.
<al-maisan> jtv: I prefer normal functions if I can .. also the normalize_virtualization() function is used in quite a few places unrelated to the head job..
<jtv> al-maisan: but maybe this particular invocation would look better inside _getHeadJobPlatform()?
<jtv> So imagine the whole "if <result of _getHeadJobPlatform()> is None" part would move into _getHeadJobPlatform.
 * al-maisan looks at the code again
<bac> abentley: would you do an experiment and see if you can change the topic here?
 * bac still wresting freenode
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> bac, yay!
<bac> abentley: cool, thanks
<al-maisan> jtv: are you suggesting to move lines 182-185 into _getHeadJobPlatform()?
<jtv> al-maisan: 181â185 even
<jtv> And then you could probably do without line 204 and rewrite line 189 slightly
<al-maisan> jtv: sorry .. that's what I meant to ask .. :P
<jtv> :)
<al-maisan> jtv: yeah .. I can make that change if desired.
<jtv> al-maisan: thanks, my impression is it'll make for a cleaner composition.
<al-maisan> no problem
<al-maisan> jtv: I agree
<jamalta> Edwin-lunch: thanks so much, looks like pqm finished :)
<jtv> al-maisan: next point...  _freeBuildersCount sounds like you're freeing a builders count!
<al-maisan> jtv: hmm .. I guess you're right .. it does sound like an action
<al-maisan> jtv: how about _countFreeBuilders() ?
<jtv> sure
<al-maisan> .. or _getFreeBuildersCount()
<jtv> fine
<al-maisan> the latter is probably more conventional
<jtv> yes, and avoids the notion that the counting itself is an action that you want the method to undertake.
<jtv> It's the result that we're interested in.
<al-maisan> hmm .. let's settle for _getFreeBuildersCount() then
<jtv> Agreed.
<jtv> al-maisan: the string on ll. 239â255 is doubly indented and the closing quotes are not indented far enough.
<al-maisan> jtv: good catch .. I'll fix that.
<jtv> al-maisan: in _getPendingJobsClauses, another way to express "buildqueue.virtualized = %(virtualized)s OR (buildqueue.virtualized IS NULL AND %(virtualized)s = TRUE" would be "COALESCE(buildqueue.virtualized, TRUE) = %(virtualized)s"
<jtv> COALESCE returns the first of its arguments that is non-null
<al-maisan> jtv: very cool
<jtv> OR clauses can make things so ugly...  :)
<al-maisan> jtv: COALESCE is definitely more elegant :)
<jtv> al-maisan: and another tip, more as background information: when composing SQL clauses like this, we often get more maintainable code by constructing a list of conditions and joining them with " AND ".join(conditions)
<jtv> In this case it wouldn't matter much though.
<al-maisan> jtv: yeah .. I agree, but in this case the clauses were all identical .. I was just looking to avoid repetitive code.
<jtv> Right ho.
<jtv> BTW the pattern of queries here suggests that you may be interested in the "DISTINCT ON" feature as well.  It lets you say things like "give me only one buildqueue/job row per (lastscore, id) value, and ignore any rows with the same values that come later in the query's ordering."
<jtv> (Again just background info, not trying to coerce you into using it here)
<al-maisan> hmm .. I see.
<jtv> al-maisan: in the docstring for getEstimatedJobStartTime, you mention "(weighted by the number of machines in the respective build pool)."  Is the "weighted by" really the right term here?  Not "divided by" or "distributed across" or somesuch?
<al-maisan> "divided by" is probably better
<jtv> thanks, that clarifies things for me
<jtv> al-maisan: unnecessary parentheses on line 440 of the diff, in test_buildqueue.py.
<al-maisan> jtv: noted, thanks!
<jtv> That's an import.  Or alternatively, the line needs to be broken up.  :-)
<al-maisan> removing parentheses is probably better
<jtv> Yes, the choice depends entirely on line length
<jtv> al-maisan: for assertEqual tests, we've standardized on passing the expected value first and the actual value second.  Not asking you to chance all the existing ones, but maybe the ones you touched?
<al-maisan> jtv: no problem -- I'll probably do all in one fell swoop
<jtv> cool!
<jtv> al-maisan: in test_buildqueue, you're disabling all native builders... is there no way to do that in a single loop without exhaustively naming architectures?
<al-maisan> jtv: yeah .. should be doable .. will fix that.
<jtv> thanks, I think i'll eliminate a distraction from the test.
<al-maisan> jtv: I will be signing off now .. if there's anything else that needs doing please mention it in the review email.
<jtv> al-maisan: sure, no worries
<jtv> I'll try to live up to my fascist reputation  :-P
<al-maisan> jtv: ah, c'mon, admit that you like that ;)
<jtv> al-maisan: oh YES I do!
<jtv> links-zwo-drei-vier...
<al-maisan> jtv: now it's on record :-)
<al-maisan> jtv: thank you very much again for reviewing my branch and good night :)
<jtv> al-maisan: well, at least nobody will accuse me of being *crypto*-fascist.  :-)
<al-maisan> oh, that's an advanced rank ;)
 * al-maisan falls of the chair
* abentley changed the topic of #launchpad-reviews to:  on-call: - || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-02
<jtv> Very strict reviewer wanted for 1-line change!  https://code.launchpad.net/~jtv/launchpad/bug-515702/+merge/18411
 * al-maisan is not very strict but looks nevertheless :)
<al-maisan> jtv: r=me
<jtv> al-maisan: thanks!  I'll get back to your review in a bit...  But a note: you moved the construction of the "default value" for _getHeadJobPlatform into the method, but you're not using the value anywhere.
 * al-maisan looks
<al-maisan> jtv: you're right, I'll revise the code to return the (currently unused) default if there is no head job
<jtv> thanks
<jpds> I have https://code.edge.launchpad.net/~jpds/launchpad/fix_515388/+merge/18413 if anyone has time. :)
<jtv> ...tense silence...
<jtv> :-)
<jtv> al-maisan: I'd feel a lot better if test_job_delay_for_unspecified_virtualization didn't rely on sample data, which it seems to do.  Wouldn't it be easier to have a test helper that clears out the existing jobs, so you can make a fresh start?
<jtv> al-maisan: or is that data set up by setUp?
<al-maisan> jtv: the latter
<jtv> oic
<al-maisan> the tests in test_buildqueue.py do not use any sample data
<jtv> right ho
<jtv> still, the test tends towards heavy data sets... for estimation in particular, is there no way you can unit-test against smaller queues and explore the tricky cases one by one?
<jtv> Because if it works for 3 queue items, chances are it's going to work for these 18 as well.
<jtv> When you put the diversity into the data set instead of in the test cases, you usually just ossify the algorithm's current results.  It seems efficient, but I find you get something that's hard to maintain not just because the test is fragile, but also because the test doesn't express why a particular value is expected for a particular item.
<al-maisan> jtv: I am on the phone at the moment, ttyl.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> gmb: care to take a look at an MP I re-submitted?
* jpds changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: || queue [jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> adeuring: Would that be the one for bug 172501?
<mup> Bug #172501: reject non-code patch attachements <patch-tracking> <story-patch-report> <Launchpad Bugs:In Progress by adeuring> <https://launchpad.net/bugs/172501>
<gmb> If so, I've just approved it :)
<adeuring> gmb: yes
<adeuring> gmb: thanks!
<gmb> np
<jtv> hi gmb!
<jtv> 3 cheers for our ocr  :)
<jtv> (No, I am _not_ softening you up for an oversized review.  Why would you think that?)
<gmb> jtv: Ohai. And thank you... But usually that kind of cheerleading precedes ... ah
<jtv> the wheels of intellect grind slowly but surely
<gmb> jtv: Well, to be fair, if you'd been bigjools or any of the other soyuz guys, I would've logged off straight away
<gmb> "No, I can't look at your mad 16,000-line abomination... by grandmother's on fire" or something
<jtv> ...and that's why I was very explicit about not needing an oversized review.
<gmb> Fair dos.
<al-maisan> ts .. ts .. as if we Soyuz guys *ever* submitted oversized branches for review
<jtv> gmb: now, I gather that the aforementioned scenario is not new to you.  Has it gotten to the point where they ask "maternal or paternal"?
<al-maisan> :)
<gmb> Not quite.
<gmb> But ever since cprov asked me to do a review in the back of a van at 40mph I've been leery of Soyuz code...
<noodles775> lol
<gmb> IIRC the conversation went "I may vomit on your laptop, Celso" ... "It's okay, it's  a thinkpad, it's vomit-proof."
<al-maisan> cprov was not the top branch submitter for no reason :)
<jtv> gmb: soyuz and rosetta are basically the place where dinosaurs still roam.  Dealing with them is a special skill, but not one that endears one to reviewers.
<al-maisan> gmb: but your nausea was not caused by his code .. or ..?
<jtv> al-maisan: probably the beating, gagging, drugging & manic driving that was involved in getting him to review the branch.
<gmb> al-maisan: I think it was actually the large steak I'd just eaten, courtesy of sg's corporate credit card... could be wrong, though
<al-maisan> gmb: ah, November Nexus ;)
<gmb> Got it in one :)
<jtv> al-maisan: on an unrelated note...  any reactions to what I said about the unit tests?  (I warn you, government bureaucracy just put me in a foul mood :)
<al-maisan> jtv: you may be theoretically right but it would a huge quantum of pain to restructure the tests
<jtv> al-maisan: I'm not asking you to restructure all the existing tests; I am an _understanding_ fascist.  But that one big test you added...
<al-maisan> jtv: also .. the longish tests allow me to exercise certain scenarios w/o having to go through the set-up over and over again
 * al-maisan looks at the added test again
<jtv> al-maisan: don't fall for that temptation!  I speak from bitter experience with the other great land-before-time codebase in LP
<al-maisan> jtv: thank you for being so understanding ;)
<al-maisan> hrmm .. hmmm
<jtv> al-maisan: the trick is to find a way to make repeating the setup easy for _you_, and then repeat it as often as necessary.  Don't spend too much time worrying about test time just now; this is too important.
<al-maisan> jtv: right .. I'll get back to you when I have something.
<jtv> al-maisan: thanks... you're becoming frighteningly experienced in dealing with my foul moods
<al-maisan> hrm .. fascist reviewers .. mumble ..
<jtv> jawohl!
<al-maisan> :)
<jtv> Code, you allied pigdog!
<al-maisan> Zu Befehl!
<jtv> o/
<jtv> hey, it works for that as well!
<al-maisan> o\
<jtv> lol
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: lunch || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: lunch || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb
<noodles775> Can I get you to look at a small update to a branch you already reviewed last week: https://code.edge.launchpad.net/~michael.nelson/launchpad/510331-syncsources-latest-pub/+merge/18070
<gmb> noodles775: Sure
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: noodles775  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Ta.
<bac> jtv: thanks for your explanation on the imports
<gmb> noodles775: I generally prefer `if not result_set.is_empty()` to `if result_set.count() > 0`, FWIW, but I'm not too fussed either way about it - just a thought.
<jtv> bac: np
<gmb> noodles775: Other than that it looks fine. I'll add another review.
<noodles775> gmb: thanks.
<jtv> gmb: you may avoid performance problems that way as well, if the data set is very large
<gmb> noodles775: jtv Speaks only in truths.
 * jtv bows orientally
<noodles775> :)
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> gmb: i am CHRing this week so i will defer my OCR unless things get really backed up
<gmb> bac: Righto.
<noodles775> gmb, jtv: http://pastebin.ubuntu.com/367524/
<noodles775> Looks like is_empty isn't included on the ISQLObjectResultSet in storm.zope.interfaces :/
<gmb> ?!
<gmb> Oh, ah.
<gmb> I see.
<gmb> That's suboptimal.
<gmb> noodles775: In that case, just go back to using .count(). You could maybe file a tech-debt bug about changing getPublishedSources() to use Storm, but I don't know how worthwhile that would be.
<noodles775> Yep, thanks.
<allenap> jml: Want someone to review your subunit-by-default branch? I've basically done it because I was interested. Hope you don't mind :)
<al-maisan> jtv: going through last night's review comments, don't quite see how
<al-maisan> "COALESCE(buildqueue.virtualized, TRUE) = %(virtualized)s"
<al-maisan> is equivalent to
<al-maisan> "buildqueue.virtualized = %(virtualized)s OR (buildqueue.virtualized IS NULL AND %(virtualized)s = TRUE"
<jtv> al-maisan: it's not entirely obvious, I'll admitâbut can you find a spot in the truth tables where they differ?
 * al-maisan did not try that yet :P
<jml> allenap, thanks.
<al-maisan> jtv: that's a clever simplification, thanks again!
<jtv> np
<gmb> Hi jpds; bac pointed out that I accidentally bumped your review from the queue earlier on. My apologies. I'll take a look at it shortly.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: jpds  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> gmb: No problem, thanks.
<gmb> jpds: If I ever do that again feel free to just ping me. I suck at managing channel topics.
<gmb> (You're not the first to suffer that fate, and you won't be the last, I'm sure...)
<allenap> jml: I reviewed it, but only just noticed that you'd marked it as work in progress. Sorry if I jumped the gun there, but I'm happy to review incrementals.
<gmb> jml: If you have time today, could you take another squizz at https://code.edge.launchpad.net/~gmb/launchpad/blob-processing-job-table-bug-513762/+merge/18218 please? If you've got a suggestion for a better table name I'm all ears.
<al-maisan> jtv: I took care of all of your review comments but the last one, the resulting changes are here: http://pastebin.ubuntu.com/367546/
<gmb> jpds: With whom di you have a pre-implementation call or chat about this branch, btw? Your cover letter is kinda sparse...
<jpds> gmb: I had a talk with sinzui about it.
<jtv> al-maisan: thanks!  otp but will get to it in a moment
<gmb> jpds: Cool.
<gmb> jpds: Normally, you should include the following in the cover letter to make the reviewer's life easier: Pre-imp discussion details, lint check (run `make lint` in the tree) and details of why you've made the changes you have.
<gmb> jpds: So, I might well have some questions for you... ;)
<jpds> gmb: That's fine, I'll remember that for next time.
<gmb> Cool.
<gmb> jpds: Looks good. r=me. Want me to run it through ec2 for you?
<gmb> s/Want me to/I will/ :)
<jpds> gmb: Yes, please.
<gmb> jpds: It's on its way now.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> jtv:  .. and last but not least, here's the big unit test split up: http://pastebin.ubuntu.com/367637/
<jtv> wow!
<jtv> al-maisan: about the get_one() on the result set...  that's the equivalent of first(), not of one(), right?  I mean, it doesn't break when you've got more than one row?
<jtv> (I think this is right, but better to ask)
<jtv> That disable-all-native-builders loop sure turned out easier than expected... so did a lot of the test splitup
<al-maisan> jtv: there will never be more than one row due to the "ORDER BY lastscore DESC, job LIMIT 1" clause
<jtv> ah ok :)
<jtv> al-maisan: assuming tests pass, r=me.  I'm off to do the paperwork.
<jtv> Thanks for bearing with me.
<al-maisan> jtv: they do pass indeed. Thanks for the review!
<jtv> np
<al-maisan> jtv: could you please "bless" this one last change (http://pastebin.ubuntu.com/367645/): "Now processors are only retrieved once and reused throughout the test code."
<jtv> al-maisan: looks like an improvement, though it does beg the question why these aren't loops.  :-)
<jtv> That said, this is clearly an improvement and I'll bless it.
<al-maisan> jtv: thank you!
<jtv> np
* adiroiban changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: -  || queue [adiroiban(bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> al-maisan: hi. Do you have time to land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-509252/+merge/18184 ? Thanks!
<al-maisan> adiroiban: later this evening .. is that agreeable?
<adiroiban> al-maisan: no hurry. I just wante to know if it's ok for you to land it or if should I ask someone else.
<adiroiban> thanks!
<al-maisan> adiroiban: no problem
* gmb changed the topic of #launchpad-reviews to: on-call: - || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Hi! ;)
<henninge> Is there a reviewer around to approve my cool improvement to jml's cool on-edge script?
* henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: -  || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454
<henninge> jtv: would you mind reviewing that great addition to the on-edge script?
<jtv> henninge: where?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454
<mrevell> Would anybody like a simple text change review? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-subscribe-help-bug-484297/+merge/18458
* jpds changed the topic of #launchpad-reviews to: on-call: - || reviewing: -  || queue [henninge,jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> mrevell: I know allenap already approved this but shouldn't full sentences end in a full stop?
<henninge> jtv: did you see my link to the MP?
<mrevell> henninge, I'm under the impression that our style guide forbids full stops in tool-tips. I'll check that
<jtv> henninge: yes, I'm just reading the original announcement for on-edge.
<henninge> jtv: cool, thanks
<henninge> mrevell: as long as we are consistent about it ... ;-)
<jtv> henninge: "res" is not a very clear variable name... in Catalan it means "nothing" IIRC whereas in Latin it means "thing."  How about "match"?
<henninge> jtv: result would be better, then.
<henninge> jtv: no, match is ok
 * henninge thought the class was called "Result" but that is in storm ...
<henninge> ;-)
<jtv> henninge: btw that script doesn't work for me because it doesn't honour LP_TRUNK_NAME.  :(
<henninge> jtv: I am sure it can take a lot of improvements ...
<jtv> henninge: done
<abentley> rockstar, https://code.edge.launchpad.net/~abentley/launchpad/empty-conflicts/+merge/18460
<kfogel> adeuring: okay, got that conditional working; about to commit.  Ratio of coding time to testing (startup/shutdown/etc) time: appx 1 to 4 :-(.
<adeuring> kfogel: that's not unusual ;)
<kfogel> adeuring: and I haven't even gotten a story test written for it yet.  I'm going to leave an XXX for that, due to Jorge's deadline.
<adeuring> kfogel: yeah, considering our time constraints, that should be ok
<henninge> jtv: thanks a lot.
<jtv> np
<rockstar> mwhudson, could you do a review for me?
<mwhudson> rockstar: sure
<rockstar> mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/default-branch-upgrade-bad/+merge/18483
<rockstar> Wow, Launchpad is being doggedly slow today.
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/i-love-sync-source/+merge/18485 pls
<mwhudson> rockstar: done
 * mwhudson looks at james_w's
<james_w> thanks mwhudson
<mwhudson> james_w: basically, can i trust you enough to be sure this is a good change?
<james_w> heh, I'd hope so
<mwhudson> james_w: also, i presume this isn't tested...
<james_w> I refer you to the quotes page for that
<mwhudson> right
<james_w> "-a" is a batch run which tries to sync eligible things, so it doesn't want to print 16000 lines
<mwhudson> james_w: do you want me to land it too?
<james_w> so it skips it unless you pass --moreverbose
<mwhudson> james_w: can you set a commit message?
<james_w> but I'm saying "sync this package please", and it's refusing to as I made a mistake, but not telling me why
<mwhudson> fair enough
<james_w> yes please and sure
<james_w> done
 * mwhudson hugs "ec2 land"
#launchpad-reviews 2010-02-03
<kfogel> Anyone here who can do UI review?
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: -  || queue [henninge,jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> henninge: I can't see a review on the queue for you, so I'll do jpds's now.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: jpds || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> allenap: oh sorry, that entry is stale. But I am currently preparing a new MP, so I'll keep my spot in the queue ... ;)
<allenap> henninge: Cool.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [jpds,henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> I have not made any progress with jpds's review yet.
<adeuring> beuno: could you have a look at kfogel's recent screenshots for the +patches view ( https://code.edge.launchpad.net/~kfogel/launchpad/506018-patch-report/+merge/18181 ) ?
<beuno> adeuring, sure, I'll do it now
<adeuring> beuno: thanks! I've added links to karl's recent screenshots in a comment
<beuno> adeuring, thank you for that
<beuno> adeuring, done
<adeuring> beuno: thanks!
<henninge> allenap: my mp is ready now but I will go to lunch. We can talk later. ;-) Thanks.
<jpds> allenap: Here's a screenshot of my changes: http://people.canonical.com/~jpds/2010-02-03-121355_1280x800_scrot.png
<allenap> jpds: I'm really sorry, I haven't looked at your branch at all; I got stuck on other things. Assuming it lp:~jpds/launchpad/fix_361650, then I think you should go back to the reviewers who've already seen it. In this case you'll need to talk to noodles775 and sinzui. I suspect sinzui1 will not be available much this week, so I can try to step in for him if you would like.
<jpds> It was ~jpds/launchpad/mirror_pages_v3
<jpds> allenap: Actually, it looks like sinzui1 has claimed the review, I'll leave it with him.
* jpds changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> jpds: Ah ha, well that's a different kettle of fish altogether :)
<allenap> jpds: It wasn't appearing on the view I use because it didn't have a review request for me or a team I'm in. I've added it, and I'll do it after lunch. Sadly, I cannot do UI reviews, but several people can, for example intellectronica and noodles775.
<jpds> allenap: fix_361650 needs, err, fixing, which I'll do later on. :)
<allenap> jpds: Hehe :) I'm going out now, but I'll be back in <2h, and other reviewers may turn up too, so one way or another your branch will be reviewed today.
 * henninge is back
<abentley> allenap, could you please review https://code.launchpad.net/~abentley/launchpad/ampoule-0.1.1/+merge/18481 ?
<allenap> abentley: Sure, sounds interesting :) I have jpds's to do first, but I don't think it will take long.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [henninge, jpds, abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> allenap, thanks.
<intellectronica> who wants a review?
<intellectronica> allenap: do you really have 3 more in the queue?
<allenap> intellectronica: Yeah, I've been stuck on trying to get the production-devel builder to work, so please talk to henninge. Thank you.
<intellectronica> are you suggesting that i talk to henninge about reviewing my branch?
<henninge> lol
<intellectronica> i'm not sure i can offer my help with reviews just now, if that's what you meant. sorry.
<henninge> intellectronica: if your's is not too big, we can trade reviews ...
 * henninge goes to look how big his is ....
<intellectronica> henninge: mine is 154 lines, and most of that boring test updates
<henninge> intellectronica: ok,  mine is 217, not quite as trivial. You chose ... ;-)
<intellectronica> henninge: i'll review your branch
* gmb changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [henninge, jpds, abentley, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> just creating an MP now
 * gmb reads scrollback.
<gmb> Ah.
<intellectronica> henninge: this is a bit urgent, so i'm assuming you can do it immediately. if you can't, please tell me and i'll find another reviewer
<gmb> allenap: I'll take a wild stab and guess that the chances of you getting through that queue this afternoon are very low
<gmb> allenap: Want me to take jpds's branch, since intellectronica's already taking care of henninge's?
<henninge> intellectronica: going right to it
<allenap> intellectronica: Oh right, I got the wrong end of the stick.
<intellectronica> henninge: wonderful. thanks a lot for offering this review. https://code.edge.launchpad.net/~intellectronica/launchpad/sort-by-patch-age/+merge/18531
<intellectronica> henninge: do you have an MP for your branch?
<henninge> intellectronica: https://code.edge.launchpad.net/~henninge/launchpad/bug-515680-status/+merge/18517
<henninge> intellectronica: thanks
<intellectronica> henninge: cool, i'm on it
<allenap> gmb: Yes please, that would be awesome. Thank you indeedy.
<gmb> allenap: I'm on it.
* gmb changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [abentley, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> oh wow, looks like MPs now generate the correct diff if you include a dependent branch
<intellectronica> i lurve launchpad code!
<intellectronica> rockstar: ^^^^^
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: abentley || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> intellectronica: I am not familiar with the patch report. Do you have an example? Is it on edge already?
<intellectronica> henninge: i'm not sure it's on edge already. you can give it a try by going to launchpad.dev/product-name/+patches
<intellectronica> but it's only interesting if there are some bugs with patches in the project
<henninge> I figured ... ;)
<intellectronica> henninge: sorry, hard as i tried i could not find even a single flaw in your branch
<intellectronica> i'm afraid i'll have to approve it
<intellectronica> maybe next time
<henninge> rats, now I have to land it!
<henninge> ;-)
<henninge> intellectronica: thanks!
<henninge> intellectronica: what's with the leading "-" on the orderby values? Is it to indicate inverse ordering?
<intellectronica> henninge: exactly
<intellectronica> it's the same format that is used on bug searches. a bit funny, i know
<henninge> intellectronica: consistency is cool
<henninge> intellectronica: but not all possible orderings are allowed, right?
<henninge> intellectronica: ok, got it now. The value is coming from a drop-down list, so the choices are limited.
<henninge> I was thinking of table headers with little arrows ... ;-)
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: abentley || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> intellectronica: now that I understood it, I had to approve it. ;-)
<intellectronica> henninge: fanks!
<EdwinGrubbs> gmb: are you looking for someone to review your process apport blob job branch?
<gmb> EdwinGrubbs: Yes.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: -, gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> EdwinGrubbs: Excellent, thank you :)
<allenap> EdwinGrubbs: I just put your name down in the topic for gmb's branch, but I'm free too and very happy to take it if you want to do something else. I've been rather a negligent reviewer today.
 * gmb notes he should *acutally read* the full "$foo has changed the topic..." notices
<EdwinGrubbs> allenap: I can take it. Thanks for offering.
<EdwinGrubbs> gmb: do you get any errors when you try to set _json_data to a str object instead of a unicode object?
<gmb> EdwinGrubbs: Yeah, that's why I do the conversion.
<gmb> I can't remember the exact error though. Hang on...
<EdwinGrubbs> gmb: since launchpad doesn't override the default encoding unicode(foo) will raise an exception if foo contains any non ascii characters. You can decode utf-8 characters correctly with unicode(foo, 'utf-8')
<gmb> EdwinGrubbs: Ah, good point.
<gmb> Thanks.
<gmb> EdwinGrubbs: Yeah, trying set _json_data to a str just gets me a TypeError.
<EdwinGrubbs>  I don't think I'll be able to keep my composure if I read job_for_blob any more.
<gmb> Hmm. A poor choice of var name, in hindsight...
<gmb> Although it was nearly called blob_job.
<gmb> Which is just too open to typos.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thekorn> hey, who wants to review a small string change in launchpadlib?  -  https://code.edge.launchpad.net/~thekorn/launchpadlib/add_tilde_to_url/+merge/18552
<leonardr> thekorn: r=me
#launchpad-reviews 2010-02-04
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> Could I get a review on https://code.edge.launchpad.net/~james-w/launchpad/get-requested-reviews/+merge/18592 please?
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> al-maisan: https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/18522
<al-maisan> stub: looking
* noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -/- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: stub/- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> stub: r=me
<stub> ta
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, BjornT || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, BjornT || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi BjornT , I'm loving your persistent test services branch!
<noodles775> I was wondering if there was a reason for not doing a property BaseLayer.persist_test_services rather than a module-level function?
<stub> Is this branch leaving things like the librarian running between test.py invocations?
 * stub wonders if it is useful for per-branch postgresql instances
<noodles775> stub: yes it is.
<BjornT> noodles775: i guess it could go to BaseLayer, even though it doesn't have any services. i'd be happy to move it to a property there; it makes it a bit cleaner
<noodles775> BjornT: ok. I've just sent the review, I only had one other thought. Thanks!
<BjornT> noodles775: cool, thanks!
<mrevell> noodles775, ping
<noodles775> Hi mrevell
<mrevell> hey
* jpds changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, BjornT || queue [noodles775,jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> Review for https://code.launchpad.net/~jpds/launchpad/fix_517020/+merge/18608 please. :)
* noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, jpds || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jpds: on it.
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: noodles775, jpds || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> noodles775: this is the one: https://code.launchpad.net/~michael.nelson/launchpad/create-source-recipe-build2/+merge/18535 ?
<noodles775> al-maisan: yep, thanks!
<al-maisan> no problem
<leonardr> al-maisan, noodles775, i have an oversize branch that needs review (about 1200 lines, complicated but has lots of tests)
<leonardr> can one of you take it?
<noodles775> pop it on the queue leonardr, if we don't get to it, the next reviewers will.
<leonardr> noodles775, ok
* leonardr changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: noodles775, jpds || queue [al-maisan,leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> leonardr: I am just starting on noodles775's branch so not sure whether I'll get to it.
<leonardr> ok, np
<al-maisan> noodles775: why did you replace super(CannotUploadToPocket, self).__init__() with Exception.__init__() in CannotUploadToPocket?
<noodles775> ...hrm, I think lint complained about not calling the super class (in one of the others that didn't even call Exception.__init__()), so I unified them.
<al-maisan> noodles775: if I understand https://dev.launchpad.net/PythonStyleGuide "Chaining method calls" correctly these __init__() calls will need to use the super() builtin
 * al-maisan counts at least 3 occurrences
<noodles775> al-maisan: I thought I had switched them to super()?
 * noodles775 looks again.
<noodles775> al-maisan: All the __init__ calls in my actual diff (the linked paste-bin) do use super(). But I'll go back and update the ones left over from the prerequisite branch too.
<al-maisan> noodles775: Oh, I see .. I just merged your branch into devel .. that was probably wrong
<noodles775> al-maisan: yeah, see the note on the MP. Thanks.
<al-maisan> yup
<al-maisan> noodles775: has the pre-requisite branch landed?
<noodles775> al-maisan: nope, it was reviewed, approved with some suggestions (you can click on the link to see them). I updated the branch with those suggestions as well as fixed the bug identified there.
<noodles775> jpds: sorry, I got sidetracked writing a script to automate some of my review setup. So in addition to the stuff mentioned on the bug,
<noodles775> you've also updated the reviewer attribute so that it's now required=True?
<noodles775> jpds: nm, I miss-read the diff.
<al-maisan> noodles775: can you please explain the lines 268-271 in http://pastebin.ubuntu.com/368292/
<al-maisan> noodles775: while you are at it, I am also mystified by lines 42-46 :P
<noodles775> al-maisan: Regarding 268-271, It's probably easier to comment those lines out and run the test, from m
<noodles775> woops,
<noodles775> So IBuild.calculated_buildstart has some asserts in it...
<noodles775> They assert that datebuilt and buildduration are actually set.
<noodles775> Calling assertProvides on a build accesses the calculated_buildstart property and the asserts are triggered.
<jtv> rockstar: still pining to hear from you re my half-finished review: https://code.edge.launchpad.net/~jtv/launchpad/bug-499405-translationtemplates-buildmanager/+merge/17811
<noodles775> (I don't personally think we should have asserts there inside a property)
<al-maisan> noodles775: hrrmm .. what is the purpose of actually setting these 2 properties on lines 268-271?
<noodles775> al-maisan: run the test with those lines commented out.
<al-maisan> OK
<noodles775> al-maisan: regarding 42-46, it might be easier to explain over the phone (although I'd like to ensure that docstring makes sense)
<al-maisan> noodles775: so assertProvides() is trying to access the properties introduced on the interfaces .. and that fails for `calculated_buildstart` unless the other 2 are set
<jpds> noodles775: No problem.
<al-maisan> noodles775: r=me
<noodles775> Thanks al-maisan
<noodles775> jpds: did you mean to make owner readonly=False? If so, it'd be great to see tests of that (for security, who can and can't set it).
<jpds> noodles775: Err, that should be readonly=False.
<noodles775> jpds: great.
<jpds> So, I'll need tests, right.
<jpds> noodles775: These should go into the stories/webservice/ right?
<noodles775> jpds: yep.
<noodles775> jpds: also, what's the reason for the change of vocabulary on reviewer?
<jpds> noodles775: Should still be ValidPersonOrTeam.
<noodles775> jpds: gar, sorry.
<noodles775> jpds: I can't currently update the MP, so here's the review: http://pastebin.ubuntu.com/368904/
<noodles775> Mainly just the tests that we chatted about earlier.
<al-maisan> noodles775: I guess you won't be able to access my merge-proposal either, so here's the diff: http://pastebin.ubuntu.com/368906/
<jpds> noodles775: Sure, am working on the tests.
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: -, jpds || queue [al-maisan,leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: jpds || queue [al-maisan,leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: al-maisan || queue [leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: it's a shame that there isn't a way to record what actually happened. It's not worth a new status FAILED_SOURCES_DELETED?
<al-maisan> noodles775: maybe
<al-maisan> noodles775: what do you think? Is it worth adding that new enum member?
<noodles775> If it means that it will be communicated to someone looking at the diff, then yeah, I'd think so. Otherwise not. Up to you.
<al-maisan> noodles775: my gut feeling is that this is something we can add if enough users ask for it.
<noodles775> OK.
<noodles775> al-maisan: is the test case setup code that you've got there *identical* to the code in test_processpendingpackagediffs.py
<al-maisan> noodles775: more or less
<noodles775> You're pushing up the line-count of LP there then ;)
* noodles775 changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> noodles775: good point .. I can refactor and share the code.
<noodles775> al-maisan: great.
<james_w> could I get a review of https://code.edge.launchpad.net/~james-w/launchpad/get-requested-reviews/+merge/18592 please?
* james_w changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [leonardr, james_w] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> it will make Daniel very happy
<intellectronica> james_w: sure, i'll review it
<thekorn> james_w, not that I have review powers, but there is a typo in xx-branchmergeproposal.txt: `Gettting`  ;)
<intellectronica> james_w: we don't use line continuation in the launchpad codebase. instead use parentheses to group different constituents of an expression appearing on different lines
<intellectronica> i can see that the same problem appears in an existing lines. no idea why the author of that line decided to format it like that. if you feel like fixing that too, i won't complain :) (but don't feel obliged)
* jelmer_ changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [leonardr, james_w, jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> james_w: the tuple on line 103 of the diff is formatted a bit funny. why not just put it on one line?
<intellectronica> james_w: also, maybe rename 'collection' in the same function to something more meaningful, like 'user_visible_branches'?
<intellectronica> james_w: using title capitalization, isn't "Gettting Merge Proposals a Person has been Asked to Review" the correct form?
<intellectronica> james_w: why the final comma on line 129?
<intellectronica> james_w: other than that it all looks good
<james_w> intellectronica: sorry, got caught in a conversation, thanks for the comments, I'll fix up now
<intellectronica> cool
<james_w> intellectronica: I'm happy with the logic etc. Do you have any concerns about things such as performance?
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> I'm guessing it's quite an efficient query, as it is based on that used to build +activereviews, but we will be using it for teams that may in the future have >100 reviews
<intellectronica> james_w:  no, i don't see anything that makes me worried. anything in particular you're concerned about?
<intellectronica> rockstar!
<rockstar> intellectronica, hi.
<intellectronica> rockstar: i owe you guys a coke or something for making MPs generate correct diffs from dependent branches. it's wonderful!
<rockstar> intellectronica, it's been doing that for a few months now actually.  :)
<rockstar> intellectronica, do you use pipes?
<intellectronica> really?!
<james_w> intellectronica: nothing in particular, but you have far more experience of this sort of thing than me
<intellectronica> rockstar: i was always under the impression that it doesn't and pasted a diff manually whenever i had a serious of branches.
<intellectronica> rockstar: and no, i don't use pipes yet
<rockstar> intellectronica, if you use pipes (and thus have dependent branches), pipes has a lp-submit command that creates a merge proposal and sets the dependent branch and everything.
<intellectronica> cool. i still have to check it out
<al-maisan> noodles775: you have mail :)
<noodles775> al-maisan: checking
<al-maisan> noodles775: thank you!
<noodles775> al-maisan: You don't need a TestProcessPendingPackageDiffsScript.setUp() anymore (it's just calling the superclass method which it will do by default).
<al-maisan> noodles775: ah, I see.
<al-maisan> let me remove it then
<al-maisan> the same holds for the other test class
<noodles775> Yep, other than that, r=me.
<noodles775> al-maisan: ^^
<al-maisan> noodles775: thank you!
<al-maisan> noodles775: http://pastebin.ubuntu.com/368974/
<noodles775> Great.
<intellectronica> BjornT: why are all the stub setup methods in the new test layer @profiled?
<BjornT> intellectronica: just to be safe. i think that if they aren't there, the parent class' setup/teardown will be run twice
<intellectronica> BjornT: oh? what does the profiling decorator have to do with that?
<intellectronica> sorry, my question was a grammatical atrocity
<BjornT> intellectronica: oh. they are there so that we don't forget to add them if we ever add some specific setup, and maybe to make the data make more sense (i.e. not omit some layers)
<intellectronica> gotcha
<intellectronica> BjornT: r=me
<BjornT> thanks!
<james_w> intellectronica: diff refreshed, could you look again please?
<intellectronica> james_w: sure. looking
<intellectronica> james_w: r=me
<james_w> thanks
<intellectronica> james_w: would you like me to land it on your behalf?
<james_w> could you submit for me please?
<james_w> yes please, thank
<james_w> s
<intellectronica> james_w: commit message?
<james_w> intellectronica: set
<intellectronica> james_w: fanks
<james_w> no, thank you
<intellectronica> are you correcting my english or being english?
<leonardr> rockstar, feel free to ask me questions about my huge branch
<rockstar> leonardr, yeah, I was getting ready to, still trying to take it all in.
* jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer,jamalta :)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
* jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jamalta :)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, does this actually give all the rest api versioning support needed to start versioning?
* jpds changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jamalta :), jpds] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> rockstar: almost all of it. i am working on support for multiversioned mutators and destructors now
<rockstar> leonardr, ah, okay.
<leonardr> in earlier branches i added support for multiversioned collections and named operations
<leonardr> this is the main branch for entry fields
<rockstar> leonardr, why do you create a VersionedDict and then push None to it?
<rockstar> Yea, I think I reviewed an earlier branch.
<leonardr> rockstar: "None" is shorthand for the earliest version (we don't know the real name of the earliest version until the second zcml stage)
<rockstar> leonardr, could you add a comment accordingly then?  That just looks funky.
<leonardr> so anything added to the VersionedDict will count as an annotation on the earliest version, until a specific version number is encountered
<leonardr> rockstar, sure
<leonardr> what file is that in?
<leonardr> nm, found it
<rockstar> leonardr, the tests here are really quite good.  They've been a huge help in understanding the patch.
<leonardr> thanks
<leonardr> i had to write them to understand how to write the patch
<rockstar> leonardr, I do see "[XXX leonardr I'll add these tests in my next branch; this branch is already way too large."  What tests are those?
<leonardr> there are a couple more paragraphs in those square brackets
<leonardr> they are describing tests i need to write
* jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jpds] || This channel is logged: http://irclogs.ubuntu.com            ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> Ah, okay.  Those paragraphs were ambiguous as to whether they were part of the documentation or if they were part of the comment.  They make more sense in the latter context.
<leonardr> that's why i did the square brackets...
<leonardr> anyway, they won't last long
<rockstar> Yea, I hadn't noticed the closing bracket.
<rockstar> Thanks for being considerate on the patch size.  I think making a note is probably okay for now.
<rockstar> I'm assuming that there won't be any releases between the time this lands and the time that those other tests land.  Am I safe in assuming that?
<leonardr> rockstar: yeah, it's pretty certain
<rockstar> leonardr, great.
<rockstar> leonardr, r=me then.
<leonardr> whew
<rockstar> leonardr, would you mind if I also ask that you request me to review the followup branch?
<leonardr> rockstar, sure
<leonardr> i discovered today it's not going to be just more tests
<leonardr> making multiversion mutators work is turning out to be very difficult
<rockstar> leonardr, yeah, I think it might help for consistency's sake if I looked at the next branch as well.
<leonardr> sure
* jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jpds, jamalta] || This channel is logged: http://irclogs.ubuntu.com             ||https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jelmer || queue [james_w, jpds, jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jelmer_, ping
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: james_w || queue [jpds, jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> james_w, ping
<james_w> hey rockstar
<rockstar> james_w, where's the branch you need reviewed?
<james_w> rockstar: oh, it's done, sorry
<rockstar> james_w, great! That makes my life easier.  :)
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jpds || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jpds, ping
<jpds> rockstar: Hi.
<jpds> rockstar: I've fixed noodles' comments on https://code.edge.launchpad.net/~jpds/launchpad/fix_517020/+merge/18608
<rockstar> jpds, oh, you'll need to follow up with noodles then.
<jpds> rockstar: Oh, right.
* jpds changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: - || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jamalta || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jamalta, you and me now.
<jamalta> rockstar: hey! that was quicker than i thought
<jamalta> did you have a chance to look at the bug?
<rockstar> jamalta, looking.
<jamalta> rockstar: alright
<jamalta> based on Francis' reply, I'm thinking adding View classes in security.py for the missing interfaces should do the trick, right?
<rockstar> jamalta, hm, I'm not sure if I'm the best person to have a pre-imp about that.
<rockstar> leonardr, ^^
<jamalta> rockstar: oh alright
<leonardr> rockstar: i don't know what you're talking about, looking at bug 517020
<jamalta> leonardr: bug #515761
<mup> Bug #515761: Anonymous API access to some collections returns nothing <Launchpad Foundations:Confirmed> <https://launchpad.net/bugs/515761>
<rockstar> leonardr, honestly, I don't know what I'm talking about.
<leonardr> jamalta: i can confirm that the problem is that lazr.restful is filtering out those items due to lack of permission
<leonardr> i don't know enough about launchpad/zope to say how to fix it, but adding View classes seems reasonable
<jamalta> leonardr: right, well adding View classes seems to work
<leonardr> ok, cool
<jamalta> so long as checkUnauthorized returns True
<jamalta> (and checkAuthorized as well, of course)
<jamalta> so are you ok with that solution?
<jamalta> also, i can't figure out how to test anonymous api access
<jamalta> all the tests dealing with the api seem to already be authenticated
<leonardr> jamalta: in a pagetest you can use anon_webservice instead of webservice
<jamalta> so that throws me off a bit
<jamalta> leonardr: oh! thanks!
<jamalta> that would be why i couldn't find it
<jamalta> i was searching for anonym
<jamalta> so are you good with this solution leonardr ?
<leonardr> jamalta: yes, with the caveat that i don't know very much about that system
<leonardr> if the security decision is 'make it visible to everyone', then what you describe should work
<jamalta> leonardr: well, i haven't talked to anyone relating to security, but this data is available through unauthenticated users in the actual site
<jamalta> is there someone i should ask about this?
<jamalta> or should this wait until the review?
<leonardr> jamalta: implement it and mention it as a topic for the review
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> leonardr: sounds good, thank you
<jamalta> rockstar: thank you as well
<abentley> rockstar, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/twisted-environ/+merge/18626
<rockstar> abentley, looking
<rockstar> abentley, r=me
<abentley> rockstar, thanks.
<EdwinGrubbs> rockstar: can you review a super simple branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-516071-setup_add_member_handler/+merge/18629
<rockstar> EdwinGrubbs, looking.
<jamalta> rockstar: ready for me again?
<rockstar> jamalta, sure.
<jamalta> rockstar: give me a min, forgot to finish my proposal
<jamalta> and run lint for that matter
<jamalta> rockstar: ok, now i'm ready.. sorry about that https://code.edge.launchpad.net/~jamalta/launchpad/515761/+merge/18638
<jamalta> wowah, not quite sure how i ended up committing wadl-testrunner.xml
<jamalta> rockstar: i guess we can hold off on that, i have a few screw ups to take care of, sorry again
<rockstar> jamalta, it's okay.
<leonardr> rockstar, i have a very short branch continuing my series if you want to review it
<rockstar> leonardr, sure.
<leonardr> ok, writing mp now
<leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-destructors/+merge/18640
<jamalta> rockstar: ok, this time i think i'm good
<rockstar> leonardr, um, this branch has the changes from the previous branch.
<leonardr> rockstar: argh, i haven't merged that yet
<leonardr> i can give you a diff
<leonardr> or give me am inute and i'll merge
<rockstar> leonardr, if you set the dependent branch for the mp, it'll generate the diff properly.
<leonardr> rockstar: i don't know how to do that. is that the same as 'merge into'?
<leonardr> no, nm...
<leonardr> no, i stand by my ignorance. how do i set that?
<rockstar> leonardr, it looks like you can't set it after it's created.  Lemme try to resubmit.
<rockstar> Oh crap, that only made your comments go away...  :/
<leonardr> rockstar: fortunately i have that page open
<rockstar> leonardr, could you delete it and re-create it, and in the "more options" section, set the dependent branch there?
<leonardr> rockstar: i've merged the other branch, so maybe it's now moot?
<rockstar> leonardr, it won't update unless you push a new revision to the target branch.
<leonardr> ok
<rockstar> Er, push a new revision to the source branch.
<leonardr> i'll just recreate it
<leonardr> rockstar: perhaps this will work better
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-destructors/+merge/18643
<rockstar> leonardr, thanks.
<rockstar> Sorry it's a pain in the butt.  We should be able to set the dependent branch after the fact.
<jamalta> rockstar: i had a few changes that bac pointed out.. because of it have to change my proposal.. should i add it as a new comment or resubmit?
<rockstar> jamalta, just add a new comment and push your changes.  That should be fine.
<jamalta> rockstar: salright
<jamalta> alright, even
<rockstar> jamalta, did you change wadl-testrunner.xml?
<rockstar> Er, did you mean to?
<rockstar> jamalta, it also says it has conflicts.  Something is not right with that mp.
<jamalta> rockstar: sorry, this mp https://code.edge.launchpad.net/~jamalta/launchpad/515761-anonymrelease/+merge/18641
<jamalta> i fixed those issues
<jamalta> i pushed to the wrong branch by accident so i just made a new MP
* rockstar changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jamalta, hm, maybe you should have a single class that inherits from AuthorizationBase that deals with checkAuthenticated and checkUnauthenticated - There's a lot of repetitive code here.
<jamalta> rockstar: i was thinking of doing that but no other class does that
<rockstar> jamalta, well, that may be for hysterical reasons.  I don't see why new code should be like that.
<jamalta> and then there's the message on launchpad-dev from henning talking about refactoring security.py altogether
<jamalta> rockstar: i could do that if you would prefer though
<rockstar> jamalta, I say do it.  Talk on a mailing list gets trumped by actual code.
<jamalta> rockstar: haha, sounds good :)
<jamalta> ViewByAnyUser a good name?
<rockstar> jamalta, AnonymousAuthorization ?
<jamalta> rockstar: even better
<jamalta> rockstar: i'm going to push this now but have to head out so will hold off the review until tomorrow
<jamalta> rockstar: thanks for the help and input :)
<rockstar> jamalta, sure.
<jamalta> rockstar: g'night
#launchpad-reviews 2010-02-05
<henninge> Any reviewers around that could review https://code.edge.launchpad.net/~henninge/launchpad/bug-516739-needs-information/+merge/18693 for me, please?
<henninge> jtv: want some fun? ^
<henninge> ;-)
<jtv> henninge: sure
<jtv> henninge: for the docstring on NEEDS_INFORMATION may I suggest, "The reviewer needs more information before this entry can be approved"?
<henninge> fbm
<henninge> = "Friday's Big Meeting" according to the GTF ... ;-)
<jtv> which is a really nice kind of online novel, highly recommend it.
<jtv> henninge: I'm puzzled by the tests... they list statuses A B D F I NR NI in that order.  But that's not the order they have in the enum.
<jtv> Should be A I D F NR B NI
<jtv> And the tests do expose that difference...
<henninge> jtv: yes, I did not look at the enum. There is a comment on it in the test.
<henninge> which difference?
<jtv> Maybe they're sorted by name somewhere in the test?
<jtv> But in that case NR and NI should be the other way around.
<henninge> jtv: right, that's what I did! ;-)
<henninge>         # Could iterate RosettaImportStatus.items but listing them here
<henninge>         # explicitely is better to read. They are sorted alphabetically.
<jtv> s/explicitely/explicitly/g
<jtv> So that comment is wrong now.
<henninge> no, my ordering. I will fix that.
<henninge> Eclipse even marked the wrong spelling on explicitly ... ;)
<jtv> nice :)
<jtv> OK, r=me, I'll make it official.
<jtv> For some reason I missed the box where I could set the review type, sorry
<henninge> jtv: you can still edit your vote
<jtv> ah yes, that lets me set it
<jtv> all done.  I've got to run to catch that train
<henninge> jtv: thanks and enjoy FOSDEM!
<jtv> thanks!
<jtv> you'll have to entertain ursula today.  :)
<henninge> I will, no worries ... ;-)
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> BjornT: https://code.edge.launchpad.net/~barry/launchpad/rnr-techdebt/+merge/18696
<jamalta> salgado: hey there, want to do a review for me? :) fibiger
<jamalta> err
<jamalta> https://code.edge.launchpad.net/~jamalta/launchpad/515761-anonymrelease/+merge/18641
<jamalta> had the wrong thing on my clipboard, sorry
<salgado> heh. no worries
<salgado> jamalta, well, it's been reviewed already?
<jamalta> salgado: yes, rockstar said it needs fixing
<jamalta> he just wanted me to create a class that defines anonymous access to inherit from, instead of repeating the same thing over and over
<jamalta> also, i'm supposed to bring up any security concerns with opening these items up for webservices
<jamalta> i haven't talked to anyone specifically regarding security but it seems in general these items should be open
<jamalta> considering, they are available to unauthorized users through the web frontend
<salgado> jamalta, cool, in that case you can just send him the incremental diff and he should approve your branch
<jamalta> salgado: rockstar?
<salgado> jamalta, yeah, but they might be public by accident in the web UI, although that's unlikely.  maybe send am email to launchpad-dev@?
<jamalta> salgado: sounds good, i will confirm that
<salgado> jamalta, yeah, he's reviewed your branch already, so he'll just have to check the changes you did since his review.
<jamalta> salgado: sounds good, sorry for bugging you then
<salgado> no worries
 * maxb has a nice tiny MP, when someone has a moment https://code.edge.launchpad.net/~maxb/launchpad/py2.6-importfascist/+merge/18646
<leonardr> rockstar, are you around for another review in the multiversion series?
<salgado> maxb, approved
<maxb> thanks. Will you land it?
<salgado> sure
<maxb> Maybe one day someone will sort out a publicly accessible PQM :-)
<salgado> does it have a commit message?
<salgado> maxb, ^
 * maxb adds one
<maxb> "Accept and propagate a 'level' parameter in importfascist. (New feature in Python 2.5, helps compatibility with 2.6)" sound ok?
<maxb> I've saved that in the MP
<leonardr> rockstar: when you come in, https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-mutators/+merge/18702
* noodles775 changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> salgado-lunch: easy one for when you get back: https://code.edge.launchpad.net/~michael.nelson/launchpad/513201-move-recipe-to-code/+merge/18703
<rockstar> leonardr, I'm on it.
<leonardr> rockstar, tx
<rockstar> jamalta, I'll get to yours next.
<jamalta> rockstar: thanks! :)
<salgado> noodles775, approved
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks salgado
* jelmer changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> salgado, hi
<salgado> hi jelmer
<jelmer> salgado: Can you perhaps review this branch: https://code.edge.launchpad.net/~jelmer/launchpad/sync-tbz2/+merge/18615
<salgado> wow, that's a big one
<salgado> jelmer, sure, I can do it
<leonardr> rockstar, taking a short subway ride, biab
<rockstar> leonardr, no sweat.
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jelmer || queue [-] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> salgado: thanks!
<rockstar> jamalta, you don't really need to post an incremental diff.  The diff on the merge proposal page gets updated, and that makes it easier for me to read the changes anyway.
<rockstar> The incremental diff was a method we had when merge proposals sucked.
<jamalta> rockstar: alright.. i'm having difficulty keeping up with how everyone likes things :(
<salgado> jelmer, so, we already support orig.tar.bz2 uploads and your branch is only fixing the script which syncs packages from debian?
<jamalta> a previous reviewer said he prefers an inc diff with the changes
<jamalta> ohh! ok
<jamalta> that makes sense then, thanks
<jelmer> salgado: yep, exactly
<salgado> cool, I got confused by the bug description
<thekorn> hi, how is this review process in this channel working? - if it is just posting the like and be patient, here it is: https://code.edge.launchpad.net/~thekorn/launchpad/fix-tagcombinator-517570/+merge/18706
<thekorn> but it is not urgent ;)
<jamalta> thekorn: there's usually an on-call reviewer (in the topic) and they keep up a queue... from what i understand you usually ping them with your MP and they'll let you know when they can get to it
<jamalta> rockstar: thanks so much! :)
<jamalta> would you be able to land it too?
<thekorn> aha, ok, salgado if you have some time, can you please have a look at my MP above
<thekorn> thank you!
<thekorn> and btw, when I'm working on a bug, is it ok to assign this but to me and mark it as "in progress" ?
<rockstar> jamalta, I can. Give me a bit.
<jamalta> rockstar: sure thing, no rush at all.. whenever you have time
<salgado> thekorn, I'm currently doing a big one for jelmer, so I might not get to yours.  if that's the case, I'm sure someone else will be able to review it either today or Monday, though
<thekorn> salgado, ok cool. again it is not urgent, just wnated to make sure I'm following the correct process here
<leonardr> rockstar: thanks for the review
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: thekorn || queue [-] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> thekorn, just approved your branch; do you want me to land it for you?
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<thekorn> salgado, yes please
<thekorn> salgado, thanks alot
<salgado> thekorn, can you set a commit message in the merge proposal?
<thekorn> sure, give me a few
<thekorn> salgado, done
<jamalta> salgado: do you have time for a quick chat?
<jamalta> salgado: after you're done with thekorn of course
<salgado> jamalta, sure, what's up?
<jamalta> salgado: well, i make silly mistakes that screw up the total in https://dev.launchpad.net/Contributions
<jamalta> so i was wondering what you thought of doing something like this http://paste.ubuntu.com/369688/ to the contributions script
<salgado> jamalta, I think that's ok
<jamalta> salgado: cool! can I push a branch and make an MP for you to review then?
<salgado> sure
<jamalta> salgado: awesome, thanks so much
<jamalta> salgado: when you have a chance https://code.edge.launchpad.net/~jamalta/launchpad/contributions-merge/+merge/18715
<jamalta> thanks again
<salgado> jamalta, the diff there has 900 lines
<jamalta> salgado: ugh realyl?
<jamalta> ...wow let me fix that
<jamalta> salgado: yeah, i'm not even sure what happened there.. my commit is only for the one script
<jamalta> i'm going to make a new branch and try again
<jamalta> salgado: really sorry about that, not sure how that happened
<thekorn_> jamalta: you have choosen lp:launchpad as merge target, but lp:launchpad/devel is the correct one
<jamalta> thekorn_: ah oops, ok that makes sense
<salgado> jamalta, just resubmit the m-p
<salgado> and pick launchpad/devel as the target
<jamalta> salgado: sorry about that
<jamalta> https://code.edge.launchpad.net/~jamalta/launchpad/contributions-merge/+merge/18716
<salgado> no worries
 * jamalta keeps an eye on the diff this time
<jamalta> that's a bit more accurate :)
<dhillon-v10> hi all, I am new here, but have been reading launchpad code for a while now (especially schemas), there are some features in soyuz that I would like to work on and the wiki said to setup a pre-implementation call, so can anyone mentor me so I can get the feature worked out :)
<dhillon-v10> they are mostly bug reports asking for features
<salgado> jamalta, would you mind fixing the pylint complaints in that file?  just shortening a couple too long lines and suppressing a warning about a bare except
<jamalta> salgado: guess it wouldn't hurt
<jamalta> i left them alone because i was unsure
<salgado> dhillon-v10, best thing would be to talk to one of the soyuz guys (bigjools, al-maisan or noodles), but none of them are around now
<dhillon-v10> salgado: hi there :) alright will do, i guess you guys all work at european times right?
<dhillon-v10> jamalta: hi :)
<jamalta> dhillon-v10: hey there! how's it going?
<dhillon-v10> jamalta: long time no see, i am good how about you
<salgado> dhillon-v10, most of the soyuz guys do, but we have plenty of devs on american timezones
<jamalta> dhillon-v10: not too bad, just fixing some stupid errors of mine :)
<jamalta> salgado: ok pushed the fixes
<dhillon-v10> salgado: alright, so do you think mailing the list could be a better idea then
<salgado> dhillon-v10, probably.  that way you might even get a reply during the weekend
<dhillon-v10> salgado: thanks a lot :) bye and take care
<salgado> dhillon-v10, you're welcome. :)
<salgado> thanks jamalta.  approved and now I'll submit it
<jamalta> salgado: thanks!
* salgado changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> salgado: by submit you mean you will land it?
<salgado> jamalta, yep
<jamalta> salgado: sweet, thanks to much
