[03:26] <thumper> mwhudson: ping
[03:26] <mwhudson> thumper: otp
[04:04] <thumper> https://code.edge.launchpad.net/~thumper/launchpad/branch-distro-avoid-scan/+merge/36945
[04:04] <thumper> mwhudson: trivial 3 line review
[04:04] <StevenK> Argh, I can't review that :-)
[04:04] <thumper> StevenK: well you could
[04:05] <thumper> StevenK: but it is super trivial
[04:05] <mwhudson> i can mentor the review i guess
[04:06] <mwhudson> thumper: oh haha, so the branches on disk were ok but the db was wrong?
[04:06] <thumper> mwhudson: yep
[04:06] <mwhudson> oops
[04:06] <thumper> wasn't too bad
[04:06] <thumper> just a tweak to avoid the scan
[04:07] <thumper> mwhudson: I'm looking at writing a script to fix up the stacking of the maverick branches that are stacked on lucid
[04:07] <mwhudson> thumper: do we have any idea what happened there?
[04:07] <thumper> not yet
[04:07] <thumper> I was thinking I should talk to james_w to ask
[04:07] <thumper> but was too busy this morning
[04:07] <mwhudson> thumper: branch reviewed
[04:08] <thumper> ta
[04:35] <thumper> wallyworld__: ping
[04:35] <wallyworld__> thumper: pong
[04:36] <thumper> wallyworld__: skype
[06:08] <jtv> stub: I've completed my follow-up.
[06:08] <jtv> Sorry, poolie.
[06:08] <jtv> Not stub.
[08:46] <poolie> jtv, any chance of yet another review?
[08:52] <jtv> poolie: arrh!  Well, I _am_ supposed to be OCR today (though being lazy because of health issues)
[08:52] <jtv> poolie: or do you mean, another round of the same one?
[08:53] <poolie> another round of the same one
[08:53] <poolie> it actually got simpler, and perhaps smaller
[08:55] <poolie> if you can count it as OCR that would be a great use of the role
[08:55]  * jtv looks
[08:58] <jtv> poolie: what is a "relevant" feature controller?  What are the irrelevant ones?
[08:59] <jtv> 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 <feature-flags> <qa-untestable> <Launchpad Foundations:New for lifeless> <https://launchpad.net/bugs/631884>
[09:00] <jtv> thank you mup, you're always there when we _don't_ need your help :)
[09:01] <poolie> :)
[09:01] <poolie> "the one you should look at"
[09:01] <poolie> i could change it to "for request" or "for thread"
[09:01] <poolie> but i wasn't sure i wanted to commit to that
[09:02] <poolie> perhaps if you pass nothing it must be per thread in some way
[09:02] <jtv> 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] <poolie> k
[09:06] <jtv> poolie: btw are all fields required to be nonempty?
[09:06] <jtv> (I'm guessing yes)
[09:07] <poolie> yes
[09:08] <jtv> That's good to know.  And I guess the parser reads multiple spaces as a single separator?
[09:08] <poolie> yes
[09:09] <jtv> Is that covered by tests?
[09:09] <poolie> yes
[09:10] <poolie> test_setAllRulesFromText
[09:12] <jtv> Ah.  It's implicit.
[09:12] <jtv> (And still weirdly indented)
[09:13] <jtv> Illustrates how quickly you can have too much data and verify too many things in one test.
[09:14] <jtv> 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] <poolie> ah, i am feeling a bit impatient with this at the moment
[09:14] <poolie> 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] <poolie> i don't mean to sound snarky
[09:15] <poolie> what i mean is that it's useful to describe the rules so people will use them automatically next time
[09:15] <poolie> more so than fixing up stuff that's unidiomatic
[09:16] <poolie> it's interesting nobody picked up on my camelCase functions in the earlier landings in this theme
[09:16] <jtv> 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] <poolie> this is about my 6th lp patch ever
[09:16] <lifeless> it seems inconsequential because we work with heterogenous stuff
[09:17] <poolie> certainly the biggest so far
[09:17] <jtv> And I've been assuming that you've heard all this before.
[09:17] <jtv> My mistake.
[09:17] <poolie> np
[09:17] <poolie> i appreciate being told
[09:17] <lifeless> so enforcing it in LP simply makes the inclination to do stuff that should be out of LP in LP even stronger
[09:17] <poolie> maybe my 10th patch; certainly less than the 20th
[09:18] <jtv> All in the same ballpark.  :-)
[09:19] <jtv> 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] <jtv> lifeless: did you ever get around to that?
[09:19] <lifeless> jtv: I did, I'm frying one fish at a time.
[09:20] <lifeless> the current one is review size limits
[09:20] <poolie> lifeless: you want to push them down or dispense with them?
[09:20] <jtv> 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] <poolie> i care more about the elapsed time than the number of lines
[09:21] <lifeless> poolie: I want to remove them from the scope of reviews.
[09:21] <lifeless> poolie: I think its a valueless overhead to worry about
[09:21] <poolie> surely it is in the submitter's best interests to do small patches anyhow
[09:21] <jtv> lifeless: I do see our lives being appreciably better now that we have the limit though.
[09:21] <poolie> if they're enlightened enough to realize it
[09:21] <lifeless> well
[09:21] <poolie> though there are overhead factors that push people away from doing small landings, istm
[09:22] <jtv> The limit is more a reminder than a limit, in practice.
[09:22] <lifeless> line count in the diff is a poor surrogate
[09:22] <lifeless> for anything.
[09:22] <poolie> right
[09:22] <lifeless> there are two questions, in my mind.
[09:22] <lifeless> a) do we need a 'limit or negotiate' clause at all
[09:22] <lifeless> b) if we do, whats the right metric.
[09:22] <lifeless> I have points on both a) and b)
[09:23] <lifeless> for a), assuming we have a perfect metric that isn't a poor surrogate
[09:23] <poolie> also, and more urgently
[09:23] <jtv> there's a wide gap between "perfect metric" and "not a poor surrogate"
[09:23] <poolie> any other feedback, jtv?
[09:23] <jtv> poolie: yes, I was letting lifeless finish, but now that you ask:
[09:23] <lifeless> jtv: sure, assuming we have a non-poor surrogate
[09:24] <lifeless> not all reviewers are equally fluent across the code base
[09:24] <jtv> 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] <lifeless> and across the domains
[09:25] <poolie> jtv i wondered about that too
[09:25] <poolie> it does seem unnecessary
[09:25] <poolie> bit of belt-and-braces there
[09:25] <lifeless> so the metric will need to be specific to the reviewer, which seems pretty awkward
[09:25] <poolie> i could just remove it
[09:25] <jtv> 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] <lifeless> jtv: well, that talks to b)
[09:26] <jtv> poolie: yes, I'd test (if it's not tested already; can't recall just now) and then remove the check
[09:26] <lifeless> 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] <poolie> why would anyone want to do a large patch?
[09:27] <lifeless> poolie: say they were rewriting the http stack for lp
[09:27] <jtv> Sometimes there's significant overhead to breaking things up in smaller self-contained chunks.
[09:27] <poolie> there may be reasons, but it would be interesting to know what they are
[09:27] <lifeless> it will be very hard to be sure that the direction is right until you've migrated everything across
[09:27] <poolie> right
[09:27] <lifeless> but there is lot of risk in changing that at all, so doing it twice is undesirable
[09:28] <lifeless> which is one of the forms of overhead jtv refers to
[09:28] <poolie> assuming arguendo that's true, having done it, you might as well land it
[09:28] <jtv> Or consider what you've done a proof-of-concept.
[09:28] <poolie> right, or perhaps it is useful to see if you can improve just one area
[09:28] <poolie> but it would depend on the case
[09:28] <jtv> Conversely it can also happen that things get clearer and better just from the act of re-doing it in chunks.
[09:29] <lifeless> so, I'm saying that the pragmatic reality is:
[09:29] <lifeless>  - patches smaller than $metric sometimes get asked to be rearranged
[09:29] <poolie> jtv, my thing does have tests for access control so i'll remove the check
[09:29] <poolie> hm, well, there was one thing there
[09:29] <jtv> poolie: great, then the tests will tell you whether we're making a mistake.  :-)
[09:29] <poolie> it was about trying to give read access more broadly than write access
[09:29] <lifeless>  - patches larger are routinely accepted when from trusted individuals or by particularly confident reviewers
[09:30] <poolie> however it seems like ~registry has launchpad.Edit on ILaunchpadRoot anyhow
[09:30] <lifeless> having a policy that doesn't match reality adds to the learning curve for new contributors
[09:30] <poolie> i need to go and cook soon
[09:30] <lifeless> so we should make it match reality, make reality match it, or just nuke it.
[09:31] <lifeless> 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] <poolie> jtv, so that's the point of it
[09:31] <lifeless> plenty of 1000liner patches will just be pure and wonderful
[09:31] <jtv> 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] <lifeless> e.g. don't trust zope at all
[09:32] <lifeless> this is a bug in our security framework that such tests are needed at all.
[09:33] <poolie> do people actually bounce patches for being too big?
[09:33] <jtv> 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] <poolie> well, i added them because i didn't trust my understanding of it
[09:34] <lifeless> I've had a 200 line one deferred to another reviewer
[09:34] <jtv> Also, we're not supposed to chuck oversized branches on the queue without first discussing it with the OCR.
[09:34] <lifeless> but most of my branches are > 800, and reviewers generally take about 5 minutes to review them.
[09:35] <lifeless> 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] <jtv> 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] <poolie> jtv ok, so
[09:36] <poolie> i've cleaned up the formatting and added a comment
[09:36] <lifeless> anyhow, its late, I have a 4am start to get to sydney tomorrow
[09:36] <poolie> i think the permission check is at most harmless
[09:36]  * lifeless waves
[09:36] <poolie> i know!
[09:36] <poolie> go to bed, see you tomorrow
[09:36] <jtv> \o lifeless
[09:36] <poolie> we can have lunch with steph if that suits your schedule
[09:37] <lifeless> poolie: to avoid confusion; you're staying at home, we'll get to st leonards on our own steam.
[09:37] <poolie> right
[09:37] <lifeless> poolie: I'll sms you when I'm in st leonards.
[09:37] <poolie> great
[09:37] <poolie> fly well
[09:37] <lifeless> thanks
[09:37] <lifeless> lunch could be fun
[09:37] <lifeless> I'm with you  / at the urban all thurs
[09:38] <poolie> jtv, i pushed a few more changes, if you're not unhappy with the overall impact could you land it for me?
[09:39] <poolie> i don't have pqm access atm
[09:39] <jtv> poolie: OK; got a few more small things such as "x.get('foo') or ''" → "x.get('foo', '')"
[09:39] <poolie> that's not the same thing
[09:39] <jtv> But that shouldn't stop it from landing.
[09:39] <poolie> in the case of {a=None}
[09:41] <jtv> oic
[09:42] <poolie> nice unicode though :)
 - >
[09:43] <jtv> Waited for years for that one to be supported…
[09:43] <jtv> Still no Dutch "ij" ligature though.  :(  It's a letter.  It's capitalized as one: IJsselmeer.
[09:43] <jtv> To type Šegan, use <compose> c S
[09:44] <poolie> mm i know
[09:44] <poolie> it's a shame there's no good ui to discover these things
[09:45] <jtv> Ooh yes these tests are much nicer
[09:50] <poolie> other than googling 'compose key'
[09:50] <poolie> ok, really going now
[09:50] <poolie> thanks very much for hte reviews
[09:58] <jtv> np
[09:58] <jtv> cya
[10:54] <noodles775> 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] <StevenK> noodles775: I'm so not, sorry
[11:07] <noodles775> StevenK: no worries, enjoy your evening!
[12:18] <gmb> Hi jelmer; another one for your queue if that's okay with you.
[12:26] <jelmer> gmb: np
[15:42] <jcsackett> 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
[15:59] <noodles775> jelmer: I must have used the wrong branch with bzr lp-propose-merge (although I thought I used db-devel).
[16:02] <noodles775> 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] <noodles775> Ah, it was the first of two. nm.
[16:03] <noodles775> Thanks for the review.
[17:00] <EdwinGrubbs> StevenK: are you around?
[17:23] <salgado> sinzui, bdmurray, the continue/cancel links don't work if you access the page on the main vhost
[17:27] <bdmurray> salgado-lunch: ah! so from launchpad.dev not bugs.launchpad.dev?
[17:28] <bdmurray> salgado-lunch: okay, I've recreated it now and will fix it.  thanks
[17:43] <leonardr> jelmer, edwingrubbs, i'm putting a simple branch into the queue
[17:43] <leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/474522-type-error/+merge/37018
[19:00] <sinzui> bac: would do you have time to read an apocalyptic branch https://code.launchpad.net/~sinzui/launchpad/apocalypse-interface-imports-3/+merge/37032
[19:03] <bac> sinzui: yes
[19:30] <bac> sinzui: i need to work on a [testfix] so your code review will have to wait
[20:36] <bac> sinzui: do you think the example.zcml had no purpose?
[20:38] <sinzui> I never used it. Did you know it existed?
[20:38] <bac> sinzui: yeah, i'd seen it a time or two
[20:38]  * sinzui find children
[20:39] <bac> sinzui: seems like it could have utility, unless it requires a lot of upkeep
[20:58] <sinzui> bac, where could I move the file? We have ignored it for 5 years
[20:58] <bac> sinzui: ok, delete it.
[21:40] <abentley> rockstar: I can has review? https://code.edge.launchpad.net/~abentley/launchpad/test-feature-flags/+merge/37061
[21:40] <rockstar> abentley, ack.
[21:42] <rockstar> abentley, r=me
[21:44] <abentley> rockstar: thanks.