| thumper | mwhudson: ping | 03:26 |
|---|---|---|
| mwhudson | thumper: otp | 03:26 |
| === 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 | ||
| 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:04 |
| thumper | StevenK: but it is super trivial | 04:05 |
| mwhudson | i can mentor the review i guess | 04:05 |
| 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:06 |
| 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:07 |
| thumper | ta | 04:08 |
| thumper | wallyworld__: ping | 04:35 |
| wallyworld__ | thumper: pong | 04:35 |
| thumper | wallyworld__: skype | 04:36 |
| jtv | stub: I've completed my follow-up. | 06:08 |
| jtv | Sorry, poolie. | 06:08 |
| jtv | Not stub. | 06:08 |
| poolie | jtv, any chance of yet another review? | 08:46 |
| 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:52 |
| poolie | another round of the same one | 08:53 |
| poolie | it actually got simpler, and perhaps smaller | 08:53 |
| poolie | if you can count it as OCR that would be a great use of the role | 08:55 |
| * jtv looks | 08:55 | |
| jtv | poolie: what is a "relevant" feature controller? What are the irrelevant ones? | 08:58 |
| 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> | 08:59 |
| jtv | thank you mup, you're always there when we _don't_ need your help :) | 09:00 |
| 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:01 |
| 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:02 |
| poolie | k | 09:03 |
| jtv | poolie: btw are all fields required to be nonempty? | 09:06 |
| jtv | (I'm guessing yes) | 09:06 |
| poolie | yes | 09:07 |
| jtv | That's good to know. And I guess the parser reads multiple spaces as a single separator? | 09:08 |
| poolie | yes | 09:08 |
| jtv | Is that covered by tests? | 09:09 |
| poolie | yes | 09:09 |
| poolie | test_setAllRulesFromText | 09:10 |
| jtv | Ah. It's implicit. | 09:12 |
| jtv | (And still weirdly indented) | 09:12 |
| jtv | Illustrates how quickly you can have too much data and verify too many things in one test. | 09:13 |
| 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:14 |
| 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:15 |
| 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:16 |
| 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:17 |
| jtv | All in the same ballpark. :-) | 09:18 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| 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:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:31 |
| 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:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| 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:36 |
| 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:37 |
| poolie | jtv, i pushed a few more changes, if you're not unhappy with the overall impact could you land it for me? | 09:38 |
| 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:39 |
| jtv | oic | 09:41 |
| poolie | nice unicode though :) | 09:42 |
| jtv | <compose> - > | 09:42 |
| 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:43 |
| poolie | mm i know | 09:44 |
| poolie | it's a shame there's no good ui to discover these things | 09:44 |
| jtv | Ooh yes these tests are much nicer | 09:45 |
| poolie | other than googling 'compose key' | 09:50 |
| poolie | ok, really going now | 09:50 |
| poolie | thanks very much for hte reviews | 09:50 |
| jtv | np | 09:58 |
| jtv | cya | 09:58 |
| === 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 | ||
| 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 | 10:54 |
| StevenK | noodles775: I'm so not, sorry | 11:06 |
| === 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 | ||
| noodles775 | StevenK: no worries, enjoy your evening! | 11:07 |
| === 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 | ||
| gmb | Hi jelmer; another one for your queue if that's okay with you. | 12:18 |
| === 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 | ||
| jelmer | gmb: np | 12:26 |
| === 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 | ||
| 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:42 |
| === 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 | ||
| noodles775 | jelmer: I must have used the wrong branch with bzr lp-propose-merge (although I thought I used db-devel). | 15:59 |
| 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:02 |
| noodles775 | Ah, it was the first of two. nm. | 16:03 |
| noodles775 | Thanks for the review. | 16:03 |
| EdwinGrubbs | StevenK: are you around? | 17:00 |
| === Ursinha is now known as Ursinha-lunch | ||
| === benji is now known as benji-lunch | ||
| salgado | sinzui, bdmurray, the continue/cancel links don't work if you access the page on the main vhost | 17:23 |
| === salgado is now known as salgado-lunch | ||
| bdmurray | salgado-lunch: ah! so from launchpad.dev not bugs.launchpad.dev? | 17:27 |
| bdmurray | salgado-lunch: okay, I've recreated it now and will fix it. thanks | 17:28 |
| === deryck is now known as deryck[lunch] | ||
| 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 | 17:43 |
| === 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 | ||
| 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:00 |
| === 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 | ||
| bac | sinzui: yes | 19:03 |
| === 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 | ||
| bac | sinzui: i need to work on a [testfix] so your code review will have to wait | 19:30 |
| === 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 | ||
| bac | sinzui: do you think the example.zcml had no purpose? | 20:36 |
| 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:38 | |
| bac | sinzui: seems like it could have utility, unless it requires a lot of upkeep | 20:39 |
| sinzui | bac, where could I move the file? We have ignored it for 5 years | 20:58 |
| bac | sinzui: ok, delete it. | 20:58 |
| === salgado is now known as salgado-brb | ||
| abentley | rockstar: I can has review? https://code.edge.launchpad.net/~abentley/launchpad/test-feature-flags/+merge/37061 | 21:40 |
| rockstar | abentley, ack. | 21:40 |
| rockstar | abentley, r=me | 21:42 |
| abentley | rockstar: thanks. | 21:44 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!