#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
{'channel': 'launchpad-reviews', 'month': 2}