=== bigjools-out is now known as bigjools === salgado-afk is now known as salgado === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [15:01] me [15:02] (psst, flacoste, we haven't started yet) [15:02] as evidenced by barry's absence [15:06] sorry for the delay folks! [15:06] #startmeeting [15:06] Meeting started at 09:06. The chair is barry. [15:06] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:06] welcome to today's ameu reviewer's meeting, who's here today? [15:06] me [15:06] me [15:06] me [15:06] me [15:06] me [15:06] me [15:06] me [15:07] barry: abel apologises, he's in transit [15:07] intellectronica: thanks [15:07] me [15:07] me [15:08] bigjools, cprov, EdwinGrubbs ping [15:08] * bigjools offers apologies as he is sprinting [15:08] as is cprov [15:08] gmb:, rockstar, salgado, sinzui ping [15:08] oops [15:08] bigjools, cprov no worries! enjoy the sprint [15:08] barry, me'ed already. :) [15:09] we're redesigning the world, it's very enjoyable [15:09] salgado: oops, sorry ;} [15:09] me [15:09] [TOPIC] agenda [15:09] New Topic: agenda [15:09] * Roll call [15:09] * encourage appropriate RENormalizer use in doctests (it is nicer than ellipses in many cases), gary [15:09] * Action items [15:09] * Mentoring update [15:09] * Peanut gallery (anything not on the agenda) [15:09] i have a peanut gallery item [15:10] bigjools: remember to include beer waterfalls. [15:10] me (sprinting, so distracted) [15:10] abentley: ++ [15:10] flacoste: cool, i'll make sure we get to it [15:10] [TOPIC] * encourage appropriate RENormalizer use in doctests (it is nicer than ellipses in many cases), gary [15:10] New Topic: * encourage appropriate RENormalizer use in doctests (it is nicer than ellipses in many cases), gary [15:10] gary_poster: the floor is yours [15:11] :-) k. So, I wanted to make sure everyone knew about the RENormalizing in zope.testing [15:11] And I wanted to see if we could encourage folks to use it more when appropriate [15:11] is anyone not familiar with what it does? [15:11] gary_poster: can you explain what that is for those not familiar with it? [15:11] gary_poster: me [15:11] me [15:11] gary_poster: not a clue, tbh [15:11] k [15:11] * BjornT knows about it, and doesn't like it so much :) [15:12] so, you use it when an ellipsis is too generic, and when an example output might be more helpful [15:12] so for instance, in your test, you might not want to specify what a host name is [15:12] so you can write in your test that http://launchpad.net/ should be normalized. [15:12] It's not good for big long strings IMO [15:13] gary_poster: do you have an example? [15:13] but good for short bits in which having real text would help [15:13] my main gripes for it is that it's hard to setup using our current infrastructure [15:13] gary_poster: What does normalized mean in this context? [15:13] gary_poster: is there an example we can look at? [15:13] me [15:13] yeah I was thinking that I should have gotten that. One sec, we have one in the tree (my fault ;-) ) [15:13] is this a feature already in doctest? [15:14] no [15:14] it's an addition on top of doctest [15:14] you have to install it as an outputchecker [15:14] iirc [15:14] right. you hook it up in tests.py [15:14] that's the setup part that isn't convenient [15:14] since we usually don't have a separate setup for doctests [15:14] but it's probablys omething we should change [15:14] setuptools extension points FTW [15:15] me [15:15] gary_poster: fwiw, i would like RENormalizing more, if it was possible to use it from within the doctest, instead of doing everything in the harness [15:15] right. sorry my grep was too promiscuous [15:15] flacoste: well, we kind of do, but it's tricky to add stuff globally [15:15] * sinzui keeps typing into the wrong window [15:15] BjornT: I can see that. [15:15] BjornT: you mean something like a + option? [15:15] like Bjorn says, having to modify the harness everytime you want to use it isn't convenient [15:16] oh, /that/ kind of setup - not activating the tool, but setting up the harness. I get it. [15:16] gary_poster, flacoste: i'm not worried about the difficulty of setting up, but rather making it clear to the reader that something special is going on [15:16] barry: +1 [15:16] barry: yes, something like [15:16] flacoste, BjornT that makes a lot of sense (maybe: i still don't quite know what renormalizing looks like :) [15:16] gary_poster: perhaps it would be better to write about this to the list, so that we can discuss after we've all familiarized ourselves with it? [15:16] ok, so in the set up: [15:16] lib/canonical/launchpad/mail/ftests/test_stub.py [15:16] barry: think of it as a regex that is applied before comparing the output [15:17] flacoste: that's kind of cool [15:17] barry: it is! [15:17] not just that convenient to use [15:17] especially if you need a bunch of different normalizing rules for different examples in the same test [15:18] being able to control it from within the doctest would be a huge gain [15:18] oh, now i understand. i agree with BjornT, having this transformation done behind the scene without any indication in the document itself sounds very confusing to me [15:18] gary_poster: nice example, i get it [15:18] * barry agrees [15:18] well [15:19] i think the confusion springs for us to be accustomed to seeing ellipses everywhere [15:19] so the premise is that it is supposed to make it easier to get the gist; and that it makes the test a-bit-to-a-lot more precise than an ellipsis [15:19] if the policy was never to use ellipses [15:19] it isn't confusing [15:19] because you know that normalisation is happening [15:19] always [15:19] or can expect it to be [15:20] i've never looked: how hard is it to add new +OPTIONS? [15:20] i think it is very hard [15:20] not pluggable at all [15:20] so anyway, my proposal is that we encourage the use of this thing. [15:20] Right [15:20] dang [15:20] barry, doctest.register_optionflag(name) [15:20] You may be able to do it in zope.testing [15:20] ok, i'm on crack [15:20] because that is already essentially a fork of dictes [15:20] heh [15:20] doctest [15:21] since it is not pluggable [15:21] well, mars seems to indicate that it is [15:21] http://docs.python.org/library/doctest.html#doctest.register_optionflag [15:21] LINK received: http://docs.python.org/library/doctest.html#doctest.register_optionflag [15:21] bzr development frequently uses assertContainsRe in its unittests, and it's quite useful. [15:21] heh MootBot, you're not on strike today! [15:21] um, okay, that registers the flag, how do you tell it what the flag should do? [15:21] (It asserts that a string contains a given regex) [15:21] Well, I know that customizing doctest has been a problem [15:21] and right [15:22] this is not just a flag [15:22] This seems similarly useful. [15:22] Though it does kinda get away from the idea that the tests are documentation. [15:22] gary_poster: are you interested in investigating whether renormalization can be hooked into a +flag? if so, then i think we could start using it in a limited/experimental way [15:22] barry: i don't think we should start there [15:23] i think we can try using it today [15:23] with the current limitations [15:23] abentley: do you feel that the example I gave took it away from documentation? [15:23] which will give us more exposure to the setup problems [15:23] the idea there was that it helped, since an example revision # is more informative than an ellipsis [15:23] gary_poster: No. The example you gave didn't seem to need it either, though. [15:23] flacoste: that sounds good to me [15:24] abentley: you mean, an ellipsis would have been as good or better for you? [15:24] Regexes tend to look like line noise. [15:24] gary_poster: Yes. [15:24] abentley: hm. diff'rent strokes and all I guess. [15:24] abentley: but you don't generally see the regexp in the doctest, and also ellipses can often be distracting [15:25] ok so anyway, barry, that's my spiel. :-) [15:25] as an aside, maybe we ought to investigate manuel or other more flexible doctest mechanisms at some point, but that's a much bigger deal [15:25] gary_poster: thanks. can you send an email to the ml explaining this and letting them know we can use it experimentally? [15:25] gary_poster: Okay, I skimmed too quickly. [15:25] gary_poster: yeah, and yeah :) [15:26] gary_poster: It's not as bad as I thought. [15:26] abentley: cool [15:26] barry: will do. [15:26] [ACTION] gary_poster to send an email to ml explaining RENormalization [15:26] ACTION received: gary_poster to send an email to ml explaining RENormalization [15:26] thanks [15:26] my item? [15:26] flacoste: sure [15:26] [TOPIC] flacoste peanut gallery item [15:26] it's about dead zone reviews [15:26] New Topic: flacoste peanut gallery item [15:26] TOPC: dead zone reviews [15:27] flacoste: you mean reviews outside my cell phone range? [15:27] how to proceed with branches that are ready when nobody is on call [15:27] for example, stuart put out a branch last week for review [15:27] that hasn't been picked up by anybody yet [15:28] simply because he didn't try finding a reviewer [15:28] rockstar: when are we getting mp queues? :) [15:28] nobody was on call when the branch was reviewed [15:28] barry, dunno. thumper talked about it. [15:28] flacoste: didn't try or couldn't find? [15:29] i think the former, but nobody was on call [15:29] and previously [15:29] barry, but there is http://edge.launchpad.net/launchpad/+requestedreviews [15:29] the general queue was assigned automatically [15:29] but this kind of died with the advent of OCR [15:29] flacoste: well, not so much automatically, but i get what you're saying [15:29] flacoste, I still think it's the reviewee's responsibility to find a reviewer, even if there are tools for the reviewer to find outstanding reviews. [15:29] right, you put a branch on pending reviews and somebody would get to it [15:29] flacoste: as a fallback, you can always ping someone on #lp-code. i've done reviews that way several times [15:30] rockstar: Do you mean +merges? That link breaks. [15:30] rockstar: yep [15:30] allenap, no, +requestedreviews [15:30] flacoste: or, set up a mp and pick some reviewer at random to review it [15:30] well [15:30] that would be annoying for everybody i think [15:31] flacoste: yeah, but it shouldn't happen often, right? [15:31] rockstar: Ah yeah, I see it. [15:31] allenap, sorry, address is https://edge.launchpad.net/~launchpad/+requestedreviews [15:31] flacoste: can't we just make it policy that the OCR checks the queue at the start of their shift for any un-owned branches? [15:31] that's more what I was getting into [15:31] back in the days of PendingReviews [15:31] allocating reviews was part of the OCR shift [15:31] and this was never replaced when we switched to MPs [15:31] flacoste: Also [15:32] A simple option would be to put your nick and a link to the mp in the queue in #lp-r [15:32] Regardless of whether someone is on call. [15:32] That way the next OCR knows what needs dealing with. [15:32] IRC is a lossy medium [15:32] I re-he-ally don't like the idea of just throwing a review on a pile and saying "get to this" [15:32] so i wouldn't count on it [15:33] rockstar: What, any more than we do already? The only difference now is that hte reviewer can say "sorry, branch X took longer than I expected, no can do sunny jim" [15:33] gmb, but there's still an personal interaction there. [15:34] True [15:34] For instance, I think there's real value in someone ASKING me if I can do a review even if I'm OCR. [15:34] ok, so we are changing the policy here [15:34] if we agreees on this change [15:34] Code reviews should not be mechanical. [15:34] Maybe we're changing the policy to match our current flow. [15:34] well [15:34] rockstar: i tend to agree with you [15:34] what about when we get drive-by contributors? [15:35] we'll have to have some form of queue there [15:35] My understanding has always been that it's reviewee's responsibility to make sure the branch gets reviewed. [15:35] rockstar: it is [15:35] but [15:35] there were mechanism to make that easy [15:35] now, it's much more "personal" [15:35] Yes, it's called "hey flacoste, can you review my branch" ? [15:35] rockstar: the problem is that it's not always easy to find someone online. if there's a policy that if you submitted the merge request yesterday, you can jump the queue, then it might work. otherwise we'll have branches that take a long time to get reviews, just because you'r not at irc at the right time [15:35] exactly [15:36] especially in dead zones like thailand [15:36] flacoste: the main thing i want to ensure tho, is that it's foolproof that the queue is managed correctly. meaning specifically it cannot be possible that someone forgot to pop an item off the queue and an ocr wastes time reviewing something already reviewed because they failed to look in the magical right place [15:36] BjornT, you can email. I've done reviews for jtv that way before. [15:36] which increase the turn-around time even more [15:37] since there is no queue management right now [15:37] If you're requesting a review from someone on the other side of the world, you're not worried about turnaround time. [15:37] email increases turn-around time? not if stub/whoever looks at the schedule and picks the next OCR to come on duty [15:37] i think it's ok for the policy to be, you have to find someone to agree to review your branch [15:37] given the current number of reviewers it shouldn't be a problem [15:37] but we need to make that official policy [15:38] it means that once you have a merge proposal out there [15:38] there should be an indivual assigned to it [15:38] not just the LP team [15:38] otherwise, your branch isn't going to get reviewed [15:38] that's how review allocation is done through Launchpad [15:38] flacoste, well, it can be the LP team, just in case that person doesn't get to it in time (like in gmb's case). [15:38] that doesn't work [15:39] In that case, you ask someone else and they pick it up instead. [15:39] flacoste, rockstar, would adding a "Pending review by" column to +activereviews help? [15:39] and you assign it to them [15:39] if it's assigned to the team [15:39] flacoste, or anyone: do we know of other people having problems or is it primarily stub? if the latter, we can add special cases for his situation [15:39] it's assigned to no one [15:39] barry: foolproof that queue is managed correctly: sometimes you get a branch that has not been assigned specifically for you to review. Maybe in that case you make a "i'm reviewing it" review initially as a marker for others? [15:39] mars, I don't know if it would. It wouldn't help me. [15:39] rockstar, ok [15:40] dammit! [15:40] It would be helpful if you could un-request someone to review it. [15:40] barry: i only know of stub, but that's mainly because he's used to the old scheme [15:40] what did i miss? ;) [15:40] rockstar: that would be useful. then stub could pick the next schedule ocr, which would work 90% of the time [15:41] Because ideally, once every reviewer says "Approve" then the status of the BMP should be "approved" [15:41] flacoste: as an ocr, i'd be happy if there were a stub branch or two that were waiting for me when i started [15:41] gary_poster: Would it make sense for "claim review" to be separate from "perform review"? [15:41] yes! [15:41] there is no way to claim a review [15:41] abentley: I think that would be cool [15:41] except by doing it [15:42] flacoste: i'd just pick them off first. the question is, how best to seed the queue? [15:42] or marking your vote as abstain [15:42] which sounds weird [15:42] flacoste: what about this as a trial: stub can pick the next scheduled ocr, assign the mp to him, and leave a note in the channel topic [15:43] flacoste: that sounds simple and i think it would work for me. if that fails then we can try something else [15:43] barry: i don't think making an exception for stub is useful at this point [15:43] we just have to state that you have to find a reviewer for your branch [15:43] so if there is no indivual reviewer assigned to it [15:44] your branch is not going to be reviewed [15:44] and run with it for a while [15:44] flacoste: stub was just an example. i think most devs don't fall into dead zones [15:44] it's possible that it's not a problem in practice for anybody to be able to find somebody to commit to a review [15:44] reviewers will need to get in the habit of checking their +requestedreviews ... i know i've never looked at it before [15:44] the problem here is that he was expecting somebody to pick it up, like it used to work [15:44] bac, it's a relatively new view. [15:44] the problem is that there was a subtle change in workflow without any formal announcement [15:45] bac: well, if you commit to something, you commited to something [15:45] flacoste: i think i get what you're saying [15:45] flacoste: well, i didn't commit to it if someone just assigns the review to me [15:45] you only need to look at +requestedreviews if you have trouble remembering your commitments :-) [15:45] bac: nobody would [15:45] they would ask you permission to assign you to it [15:46] that's not what barry suggested ^^, which seems reasonable to me [15:46] flacoste: it's always been dev's responsibility to get reviewed, but you're right that it's relatively new that you had to actively round up a reviewer [15:46] hey bac, can you review this branch for me (because you are OCR, or simply because you are cool) [15:46] sure! [15:46] then I assign the review to you [15:46] flacoste: that's great if it is the flow we decide on. [15:47] flacoste: also would work: an email saying "hey barry, you're on call tomorrow so i left you a present" [15:47] well, it's the one that we seem to be using and that is possible given the current tool [15:47] (since we don't want to manage a queue somewhere else) [15:47] ah, nm. being assigned a review will generate email addressed directly to me, so i'd know about it. [15:48] bac: yeah [15:48] * bac still like the new +requestedreviews view... [15:48] bac, yeah, I do too, but I think it's important to have that acknowledgement/handshake in the review process. [15:49] flacoste: we're out of time today. can you bring this up on the ml just to solidify the policy? or email me to hash it out first [15:49] the problem with this workflow is that now you suddenly rely on a single person to have time to review your branch, instead of relying that any of the OCRs have time [15:49] Q: i see ~launchpad/+requestedreviews has very few listed, which means ~launchpad is not automatically being added to some new MPs. why is that? [15:49] BjornT: only if you're not around for the OCR. [15:50] bac: Most likely because reviews created by email don't have that set. [15:50] bac: or the branch is around too late, so that you have to wait the next OCRs to start their shift [15:50] barry: i'll start a thread [15:50] bac: there are also some cases where mp's won't auto-assign a team [15:50] abentley: i created one this morning using lpsend [15:51] flacoste: thanks [15:51] BjornT: that's what I usually do, if i'm too late, i'll just ask somebody the next day [15:51] bac: and it automatically requested ~launchpad to review? [15:51] [ACTION] flacoste to take dead zone reviews discussion to ml [15:51] ACTION received: flacoste to take dead zone reviews discussion to ml [15:51] abentley: it did not. i expected it would. [15:51] abentley: wrong assumption? [15:51] flacoste: right. it would still be nicer to ask a group of people, instead of a single person. [15:51] bac: Wrong assumption. [15:52] bac: Not that it shouldn't, but it doesn't. [15:52] abentley: ok. [15:52] BjornT: yeah, but my experience is that this doesn't really work in practice [15:52] at least, that's my experience with the advent of OCR [15:52] sure, the OCR might swap it between themselve [15:52] but only because the first one agreed to it [15:53] flacoste: that's why i said "would" :) it doesn't work atm, but it would be nice if it would [15:53] i agree [15:53] okay, sorry to do this and my apologies for starting the meeting late, but we're way over, so let's end things here and take it to the ml or #lp-code [15:53] #endmeeting [15:53] Meeting finished at 09:53. [15:53] thanks everybody === mrevell is now known as mrevell-dinner === salgado is now known as salgado-afk