[03:26] mwhudson: ping [03:26] thumper: otp === StevenK changed the topic of #launchpad-reviews to: On call: StevenK || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [04:04] https://code.edge.launchpad.net/~thumper/launchpad/branch-distro-avoid-scan/+merge/36945 [04:04] mwhudson: trivial 3 line review [04:04] Argh, I can't review that :-) [04:04] StevenK: well you could [04:05] StevenK: but it is super trivial [04:05] i can mentor the review i guess [04:06] thumper: oh haha, so the branches on disk were ok but the db was wrong? [04:06] mwhudson: yep [04:06] oops [04:06] wasn't too bad [04:06] just a tweak to avoid the scan [04:07] mwhudson: I'm looking at writing a script to fix up the stacking of the maverick branches that are stacked on lucid [04:07] thumper: do we have any idea what happened there? [04:07] not yet [04:07] I was thinking I should talk to james_w to ask [04:07] but was too busy this morning [04:07] thumper: branch reviewed [04:08] ta [04:35] wallyworld__: ping [04:35] thumper: pong [04:36] wallyworld__: skype [06:08] stub: I've completed my follow-up. [06:08] Sorry, poolie. [06:08] Not stub. [08:46] jtv, any chance of yet another review? [08:52] poolie: arrh! Well, I _am_ supposed to be OCR today (though being lazy because of health issues) [08:52] poolie: or do you mean, another round of the same one? [08:53] another round of the same one [08:53] it actually got simpler, and perhaps smaller [08:55] if you can count it as OCR that would be a great use of the role [08:55] * jtv looks [08:58] poolie: what is a "relevant" feature controller? What are the irrelevant ones? [08:59] I guess the "workaround for bug 631884" comment explains it—for as long as we keep that comment. [08:59] <_mup_> Bug #631884: multiple variables pointing to FeatureController get out of sync easily

[09:00] thank you mup, you're always there when we _don't_ need your help :) [09:01] :) [09:01] "the one you should look at" [09:01] i could change it to "for request" or "for thread" [09:01] but i wasn't sure i wanted to commit to that [09:02] perhaps if you pass nothing it must be per thread in some way [09:02] poolie: given that it's complex, I can understand your reluctance. But it shows that there's often more good material for a docstring than you'd expect! Then, in line 74 of the diff (the __all__ in lib/lp/services/features/browser/edit.py), there should be a comma. [09:03] k [09:06] poolie: btw are all fields required to be nonempty? [09:06] (I'm guessing yes) [09:07] yes [09:08] That's good to know. And I guess the parser reads multiple spaces as a single separator? [09:08] yes [09:09] Is that covered by tests? [09:09] yes [09:10] test_setAllRulesFromText [09:12] Ah. It's implicit. [09:12] (And still weirdly indented) [09:13] Illustrates how quickly you can have too much data and verify too many things in one test. [09:14] If you feel inclined to point out to me that I'm a fascist reviewer, rest assured that I've been told this before. [09:14] ah, i am feeling a bit impatient with this at the moment [09:14] in bzr, i feel that it's more important to build up the quality of the contributors than the quality of their early patches [09:15] i don't mean to sound snarky [09:15] what i mean is that it's useful to describe the rules so people will use them automatically next time [09:15] more so than fixing up stuff that's unidiomatic [09:16] it's interesting nobody picked up on my camelCase functions in the earlier landings in this theme [09:16] poolie: ah! My apologies. I've known you as a team member so long that it never really occurred to me that you've been working more in a different codebase. [09:16] * lifeless doesn't care particularly about camel-or-not [09:16] this is about my 6th lp patch ever [09:16] it seems inconsequential because we work with heterogenous stuff [09:17] certainly the biggest so far [09:17] And I've been assuming that you've heard all this before. [09:17] My mistake. [09:17] np [09:17] i appreciate being told [09:17] so enforcing it in LP simply makes the inclination to do stuff that should be out of LP in LP even stronger [09:17] maybe my 10th patch; certainly less than the 20th [09:18] All in the same ballpark. :-) [09:19] lifeless: about the camel (or dromedary, I guess, what with the one missing hump), you intended at one point to take the difference with PEP8 to the list. [09:19] lifeless: did you ever get around to that? [09:19] jtv: I did, I'm frying one fish at a time. [09:20] the current one is review size limits [09:20] lifeless: you want to push them down or dispense with them? [09:20] lifeless: I must have missed it. :( Anyway, my reaction to what you said was to accept either dromedary or lower-with-underscores for methods; but afaik dromedary for stand-alone functions is wrong either way, right? [09:20] i care more about the elapsed time than the number of lines [09:21] poolie: I want to remove them from the scope of reviews. [09:21] poolie: I think its a valueless overhead to worry about [09:21] surely it is in the submitter's best interests to do small patches anyhow [09:21] lifeless: I do see our lives being appreciably better now that we have the limit though. [09:21] if they're enlightened enough to realize it [09:21] well [09:21] though there are overhead factors that push people away from doing small landings, istm [09:22] The limit is more a reminder than a limit, in practice. [09:22] line count in the diff is a poor surrogate [09:22] for anything. [09:22] right [09:22] there are two questions, in my mind. [09:22] a) do we need a 'limit or negotiate' clause at all [09:22] b) if we do, whats the right metric. [09:22] I have points on both a) and b) [09:23] for a), assuming we have a perfect metric that isn't a poor surrogate [09:23] also, and more urgently [09:23] there's a wide gap between "perfect metric" and "not a poor surrogate" [09:23] any other feedback, jtv? [09:23] poolie: yes, I was letting lifeless finish, but now that you ask: [09:23] jtv: sure, assuming we have a non-poor surrogate [09:24] not all reviewers are equally fluent across the code base [09:24] poolie: you use check_permission to see if the poster is permitted to make the change. I'm not very confident on this one, but ISTM you can just leave that to Zope permissions checks. [09:24] and across the domains [09:25] jtv i wondered about that too [09:25] it does seem unnecessary [09:25] bit of belt-and-braces there [09:25] so the metric will need to be specific to the reviewer, which seems pretty awkward [09:25] i could just remove it [09:25] lifeless: to me, 800 lines works pretty well—both to justify effective time management for the OCR and to remind the developer that it's time to cut off. [09:25] jtv: well, that talks to b) [09:26] poolie: yes, I'd test (if it's not tested already; can't recall just now) and then remove the check [09:26] jtv: staying with a), I think the primary thing is that patches be large enough to be confident in the change and small enough to be confident in it. [09:26] why would anyone want to do a large patch? [09:27] poolie: say they were rewriting the http stack for lp [09:27] Sometimes there's significant overhead to breaking things up in smaller self-contained chunks. [09:27] there may be reasons, but it would be interesting to know what they are [09:27] it will be very hard to be sure that the direction is right until you've migrated everything across [09:27] right [09:27] but there is lot of risk in changing that at all, so doing it twice is undesirable [09:28] which is one of the forms of overhead jtv refers to [09:28] assuming arguendo that's true, having done it, you might as well land it [09:28] Or consider what you've done a proof-of-concept. [09:28] right, or perhaps it is useful to see if you can improve just one area [09:28] but it would depend on the case [09:28] Conversely it can also happen that things get clearer and better just from the act of re-doing it in chunks. [09:29] so, I'm saying that the pragmatic reality is: [09:29] - patches smaller than $metric sometimes get asked to be rearranged [09:29] jtv, my thing does have tests for access control so i'll remove the check [09:29] hm, well, there was one thing there [09:29] poolie: great, then the tests will tell you whether we're making a mistake. :-) [09:29] it was about trying to give read access more broadly than write access [09:29] - patches larger are routinely accepted when from trusted individuals or by particularly confident reviewers [09:30] however it seems like ~registry has launchpad.Edit on ILaunchpadRoot anyhow [09:30] having a policy that doesn't match reality adds to the learning curve for new contributors [09:30] i need to go and cook soon [09:30] so we should make it match reality, make reality match it, or just nuke it. [09:31] for b), I don't think there is a good nonsurrogate for 'how complex and risky a change is'. Plenty of one lines will pass tests but utterly break LP. [09:31] jtv, so that's the point of it [09:31] plenty of 1000liner patches will just be pure and wonderful [09:31] poolie: as long as you test { anonymous, unprivileged, edit, admin} × { view, modify} it should be safe to trust zope with the whole thing [09:32] e.g. don't trust zope at all [09:32] this is a bug in our security framework that such tests are needed at all. [09:33] do people actually bounce patches for being too big? [09:33] Shouldn't normally happen, but if the reviewer doesn't have much time or isn't very confident, it gives them a reason. [09:34] well, i added them because i didn't trust my understanding of it [09:34] I've had a 200 line one deferred to another reviewer [09:34] Also, we're not supposed to chuck oversized branches on the queue without first discussing it with the OCR. [09:34] but most of my branches are > 800, and reviewers generally take about 5 minutes to review them. [09:35] This is one of the reasons I think the metric is poor; its clearly possible to write textually long changes that are easily understandable [09:35] But that's rarely been an issue. Plenty of branches get submitted as "this branch is over the limit, _but_ it's mostly deletions" (or sampledata updates, etc.) [09:36] jtv ok, so [09:36] i've cleaned up the formatting and added a comment [09:36] anyhow, its late, I have a 4am start to get to sydney tomorrow [09:36] i think the permission check is at most harmless [09:36] * lifeless waves [09:36] i know! [09:36] go to bed, see you tomorrow [09:36] \o lifeless [09:36] we can have lunch with steph if that suits your schedule [09:37] poolie: to avoid confusion; you're staying at home, we'll get to st leonards on our own steam. [09:37] right [09:37] poolie: I'll sms you when I'm in st leonards. [09:37] great [09:37] fly well [09:37] thanks [09:37] lunch could be fun [09:37] I'm with you / at the urban all thurs [09:38] jtv, i pushed a few more changes, if you're not unhappy with the overall impact could you land it for me? [09:39] i don't have pqm access atm [09:39] poolie: OK; got a few more small things such as "x.get('foo') or ''" → "x.get('foo', '')" [09:39] that's not the same thing [09:39] But that shouldn't stop it from landing. [09:39] in the case of {a=None} [09:41] oic [09:42] nice unicode though :) [09:42] - > [09:43] Waited for years for that one to be supported… [09:43] Still no Dutch "ij" ligature though. :( It's a letter. It's capitalized as one: IJsselmeer. [09:43] To type Šegan, use c S [09:44] mm i know [09:44] it's a shame there's no good ui to discover these things [09:45] Ooh yes these tests are much nicer [09:50] other than googling 'compose key' [09:50] ok, really going now [09:50] thanks very much for hte reviews [09:58] np [09:58] cya === noodles775 changed the topic of #launchpad-reviews to: On call: StevenK || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:54] StevenK: hi! I'm guessing you're not really still reviewing, but if you're keen for one: https://code.edge.launchpad.net/~michael.nelson/launchpad/649599-ajax-comment-on-dsdiff/+merge/36966 [11:06] noodles775: I'm so not, sorry === StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:07] StevenK: no worries, enjoy your evening! === allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775, allenap, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: noodles775 || queue: [allenap, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:18] Hi jelmer; another one for your queue if that's okay with you. === gmb changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: noodles775 || queue: [allenap, StevenK, gmb(http://bit.ly/gmb-bw-branch)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:26] gmb: np === mrevell is now known as mrevell-lunch === matsubara-afk is now known as matsubara === mrevell-lunch is now known as mrevell === Ursinha-afk is now known as Ursinha === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jcsackett changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch), jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:42] jelmer, EdwinGrubbs: when you have time to get to mine in the queue, MP is here: https://code.edge.launchpad.net/~jcsackett/launchpad/registry-errors-649836/+merge/36994 === sinzui changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch), jcsackett, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:59] jelmer: I must have used the wrong branch with bzr lp-propose-merge (although I thought I used db-devel). [16:02] bzr lp-propose-merge lp:~launchpad-pqm/launchpad/db-devel <-- was what I used, not sure why it ended up targeted to devel. [16:03] Ah, it was the first of two. nm. [16:03] Thanks for the review. [17:00] StevenK: are you around? === Ursinha is now known as Ursinha-lunch === benji is now known as benji-lunch [17:23] sinzui, bdmurray, the continue/cancel links don't work if you access the page on the main vhost === salgado is now known as salgado-lunch [17:27] salgado-lunch: ah! so from launchpad.dev not bugs.launchpad.dev? [17:28] salgado-lunch: okay, I've recreated it now and will fix it. thanks === deryck is now known as deryck[lunch] [17:43] jelmer, edwingrubbs, i'm putting a simple branch into the queue [17:43] https://code.edge.launchpad.net/~leonardr/lazr.restful/474522-type-error/+merge/37018 === leonardr changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap || queue: [StevenK, gmb(http://bit.ly/gmb-bw-branch), jcsackett, sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === Ursinha-lunch is now known as Ursinha === matsubara is now known as matsubara-lunch === benji-lunch is now known as benji === salgado-lunch is now known as salgado === deryck[lunch] is now known as deryck === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap, jcsacket || queue: [StevenK, sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:00] bac: would do you have time to read an apocalyptic branch https://code.launchpad.net/~sinzui/launchpad/apocalypse-interface-imports-3/+merge/37032 === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap, sinzui || queue: [StevenK, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:03] sinzui: yes === matsubara-lunch is now known as matsubara === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, EdwinGrubbs || Reviewing: allenap, leonardr || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:30] sinzui: i need to work on a [testfix] so your code review will have to wait === jelmer changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: leonardr || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [20:36] sinzui: do you think the example.zcml had no purpose? [20:38] I never used it. Did you know it existed? [20:38] sinzui: yeah, i'd seen it a time or two [20:38] * sinzui find children [20:39] sinzui: seems like it could have utility, unless it requires a lot of upkeep [20:58] bac, where could I move the file? We have ignored it for 5 years [20:58] sinzui: ok, delete it. === salgado is now known as salgado-brb [21:40] rockstar: I can has review? https://code.edge.launchpad.net/~abentley/launchpad/test-feature-flags/+merge/37061 [21:40] abentley, ack. [21:42] abentley, r=me [21:44] rockstar: thanks. === salgado-brb is now known as salgado === Ursinha is now known as Ursinha-bbl === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk