=== cprov is now known as cprov-zzz === thumper is now known as thumper-afk === cprov-zzz is now known as cprov === thumper-afk is now known as thumper === salgado-afk is now known as salgado [15:00] #startmeeting [15:00] Meeting started at 09:00. The chair is barry. [15:00] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:00] hello and welcome to this week's ameu reviewer's meeting. who's here today? [15:00] me [15:00] me [15:00] me [15:00] me [15:00] me [15:01] me [15:02] me [15:02] me [15:02] flacoste: adeuring gary_poster leonardr bigjools ping [15:02] me [15:03] me [15:03] me [15:03] me [15:03] me [15:03] me [15:03] cprov: danilos mars allenap ping [15:03] me [15:03] [TOPIC] agenda [15:03] New Topic: agenda [15:03] barry: thanks [15:04] np! it's a full agenda today... [15:04] * Roll call [15:04] * Action items [15:04] * UI review call update [15:04] me [15:04] * On-call review responsiveness, [intellectronica] [15:04] * cls.links should be a tuple, never a list (menu.py), [bigjools, barry] [15:04] * `__iter__()` in model objects? [cprov, barry] [15:04] * mkstemp()/open() combination leaks file-descriptors [cprov] [15:04] * Guidelines changes for tests checking systems pre-requisites (specific umasks, app configurations) [maxb, cprov] [15:04] * Peanut gallery (anything not on the agenda) [15:04] [15:04] [TOPIC] action items [15:04] New Topic: action items [15:04] * beuno to add a 'ui' specialty to ReviewerSchedule [15:04] he did this, so if you have any questions about who can do a ui review for you, check the ReviewerSchedule page [15:05] * gary_poster and barry will transfer review guidelines from the old wiki and old old wiki to the new wiki [15:05] not done :( [15:05] Let's set a time. [15:05] Or at least together make a list of what needs to be done [15:05] gary_poster: +1 we can coordinate off-channel [15:05] so we can divvy it up [15:05] cool [15:05] * UI review call update [15:06] beuno's not here. intellectronica do you or anyone else want to give an update on that c/c? [15:06] barry: i wasn't on the last one, so not me [15:06] ah right [15:07] i don't remember everything we talked about, but i will say that i am currently trying to fix the global issues with page titles, h1, h2 etc. headings [15:07] that's the most important issue i think [15:07] ping me with any questions or problems in that area. [15:07] your pages will /not/ currently look right until i fix certain things [15:08] we clarified the rules for that [15:08] and then you may have to go back and adjust your pages (e.g. remove heading slots, etc) [15:08] did you update the wiki? [15:08] flacoste: is that written somewhere? [15:08] flacoste: i did. i have maybe one small round of corrections, but it's darn close [15:08] barry: url? [15:08] i sent an email to lp-dev with details, but here's the url: [15:09] https://dev.launchpad.net/VersionThreeDotO/UI/Conversion#Heading%20rules [15:09] thanks [15:09] let's move on. please read the email & wiki and respond to that thread with any feedback [15:10] [TOPIC] * On-call review responsiveness, [intellectronica] [15:10] New Topic: * On-call review responsiveness, [intellectronica] [15:10] on call reviews are wonderful, but i feel that sometimes people don't make the best use of them [15:11] at best, on call reviews are an approximation of pair programming, not a queuing mechanism [15:11] it's hard to set any clear rules, because reviews are subject to variation in patch size and complexity, personal style, etc [15:12] but i would just like to remind reviewers that there is a lot of value in having a very interactive reviews [15:12] at the very least, it's important to communicate what the status of a review is [15:12] intellectronica: I think the main problem is that we are using them as queuing system as well; I can't come talk to an OCR person about something I just want to do since when I come back with the implementation, someone will be in the queue having their branch reviewed [15:12] occassionally ive had reviews where i didn't get a reply until after a few hours, or even a day later, and i think that's a shame [15:13] intellectronica: There may be tension between wanting to capture everything in Code Review and doing it on IRC. [15:13] danilos: in very busy times, that can be a problem, but in many cases, you can literally work on the changes together [15:13] danilos: a really great way to ensure that is to do the review on the phone [15:13] intellectronica: i agree that reviewers should let the dev know what's happening with their branch. i try to follow up in irc with a "hey intellectronica, r=me" or whatever when i'm done, but maybe that's not enough [15:14] there is another problem - the OCR queue overrides the +active-reviews queue [15:14] abentley: indeed, and for some patches a very thorough offline review is better than a very dynamic but potentially not exhaustive online review [15:14] bigjools: I think the idea is that people should do on-call reviews, so +active-reviews is a very last resort [15:14] leave a branch in the latter and it can get left behind until much later [15:15] bigjools: Why is it a problem? OCR is for interactive reviews. A branch from a sleeping Australian shouldn't take precedence over someone who asked for a review in IRC. [15:15] yeah, i think of +activereviews as offline reviews, a different beast altogether [15:15] bigjools: well, if you don't want to be around for a review, then it will happen; otoh, you can go with an OCR through the branch together and it won't be left behind [15:15] bigjools: it should always be possible to prod a reviewer into taking one of yours on +activereviews [15:15] my point is that often a branch will languish unless you get an OCR to look at it [15:15] bigjools: you seem to be saying that you don't like to use OCRs :P [15:16] hence the problem that intellectronica points out [15:16] bigjools: you could coordinate an offline review with a reviewer who isn't on call [15:16] sometimes an interactive review is totally inappropriate [15:16] intellectronica: that's what I do [15:17] bigjools: absolutely, but i'm not talking about these cases [15:17] I am just pointing out a possible reason for the behaviour you see [15:17] btw, asking questions and resolving issues on irc is a great way to do it. very interactive and often, cuts down on needsfixing round trips. just try to capture the conversation (in summary) on the review. e.g. "you and i talked about your frobnification of baz, and you agreed on irc to frobnicate foo too" [15:18] intellectronica: do you have any thoughts on improving the responsiveness, effectiveness, or throughput of reviews? [15:19] barry: one thing that i take from this discussion, is that we should make the preference for an online review explicit [15:19] bigjools: I think that if there's a problem, it's that the OCR doesn't do +activereviews when the OCR queue is empty. [15:19] intellectronica: instead of just taking a branch off the queue and disappearing for an hour or so? [15:19] I agree with abentley, and have been so guilty [15:19] abentley: agreed. let's make that explicit too [15:20] barry: yes. i would go as far as say that the OCR's main responsibility is to do online reviews. if he has extra time, taking stuff off +activereviews is great, but these branches can also be scheduled for reviewing in other ways [15:21] gary_poster: I have also been so guilty. [15:21] agreed. i'll take an action item to communicate these two ideas to the team [15:21] [ACTION] barry to communicate about +activereviews and doing on-line reviews [15:21] ACTION received: barry to communicate about +activereviews and doing on-line reviews [15:22] I disagree with the concept that +activereviews should be the next resort, what happens when someone pops in and you are in the middle of the review? [15:22] you finish it [15:22] i.e. it's fine only if you should stop working on offline review, because people wanting to use OCR don't get the full benefit then [15:22] danilos: last resort. i.e. when you have nothing else on the queue, there's an implicit off-line queue in +activereviews [15:23] barry: yeah, but offline reviews are usually longer (I am not doing OCR anymore, but I remember spending over 2h on a complicated branch), and that means that there's no OCR for that time [15:23] danilos: don't take an 800 lines branch for review from an author who is asleep if you're on call reviewer [15:23] danilos: citation needed ;) [15:23] danilos: channel your inner kiko and send a partial review. do your ocr, then if you have time, follow up with the online review [15:23] intellectronica: that too [15:24] remember kiko's guide to reviewing ... [15:24] bigjools: +1 [15:24] barry: I am not worried as an OCR person (as I remember, I am not doing it anyway), but as an OCR customer [15:24] I don't get nitpicky if it's a big branch [15:25] danilos: as on ocr customer, you should get preference over off-line reviews [15:25] barry: that's exactly what I am saying, and that's exactly what I want communicated in the announcement: i.e. if someone pops up asking for OC review, drop work you've been doing on offline branch if at all possible (i.e. you are not halfway through or something) [15:26] s/offline branch/offline review/ [15:26] i don't agree with that [15:26] if you started a review, finish it [15:26] don't interrupt for online review [15:26] never interrupt work [15:26] it leaves work-in-progress [15:26] and that's bad [15:26] flacoste: I agree with that, but my point is, I want OCR to be OCR [15:26] is it a problem [15:27] flacoste: that's why i say, ask yourself WWKD [15:27] flacoste: if the only way to get that is to make people suffer before they understand why OCR is valuable, I'd be happy to do that [15:27] danilos: also, as deryck_ and i were discussing today, team leads are not official ocr's but they should be available to do reviews when the load gets too big for the ocr. at least, i know sinzui and flacoste are almost always willing to jump in and help. [15:27] WWKD about this discussion? [15:27] :) [15:27] didn't we have multiple OCR coverage [15:27] heh [15:27] they can coordinate between them [15:27] barry: I do reviews, and all I do I do OCR [15:27] do be do be do? [15:27] to make sure that one is available for interactive review [15:28] On that note, thurs/euro is quite thin if anyone else is available. [15:28] flacoste: again, that's another requirement people have not been mentioning [15:28] i also think it's fine to say, i'll have time for an interactive review in x time [15:28] i need to finish this review [15:28] danilos: that is great. it really helps! [15:28] communication improves most problems. [15:29] noodles775: is not yourself and cprov on that slot? [15:29] barry: cprov is no longer in Euro... [15:29] * barry can't keep it all straight [15:29] So he's around from about 2pm my time. [15:29] flacoste: I think our original idea behind OCRs was to have people you can sort of do pre-imp calls with as well... if developers have to break their workflow because there's noone to talk to, it's as bad as reviewers stopping to answer something else [15:29] i guess we need to move cprov to thurs/west? [15:30] danilos: i'm not sure we were successfull about the pre-impl with OCR [15:30] anyway, I'd say this: if there's sufficient OCR coverage for people asking for interactive reviews, please go for +activereviews [15:30] danilos: It depends on the situation, but I usually want someone who knows the domain intimately for a pre-imp. [15:30] and i've come to think that random pre-impl aren't that effective [15:30] flacoste: I agree [15:31] better to have a pre-impl with somebody with some knowledge in the area [15:31] flacoste, abentley: I am only talking about rough "pre-imp"... sometimes you'd like to discuss a few things mid-implementation [15:31] i think it's fine not to put that responsability on OCR [15:31] flacoste: agreed. we've tried it enough that we should really re-think pre-imps and not keep trying what doesn't work [15:32] well, pre-impl works if you do them [15:32] anyway, I am happy to try this out, but if I suddenly start seeing a lot more delay in getting reviews because people are working on branches from developers who weren't willing to spend their time to go through a branch together, I'd come back complaining :) [15:32] but pre-impl is a separate topic [15:32] danilos: sure, that is right [15:32] flacoste: i guess that's a different subject, of how to optimize for collaborative work. it doesn't really have much to do with reviews per-se [15:33] flacoste: right, but lots of branches don't pre-imp, or do mid-imps (which is okay with me). so if people are often jumping into branches w/o pre-imps we should ask why that is and what problem we're trying to solve with a pre-imp [15:33] flacoste: but yeah, this is off-topic and we're running out of time ;) [15:33] sorry everyone for making this longer than everybody expected :) [15:33] danilos: yes, please do! [15:34] thanks everybody for this excellent topic. i think we can continue to discuss it but i'd like to move on now [15:34] [TOPIC] * cls.links should be a tuple, never a list (menu.py), [bigjools, barry] [15:34] New Topic: * cls.links should be a tuple, never a list (menu.py), [bigjools, barry] [15:35] seems so minor compared to the previous, but in a branch i reviewed for bigjools, i suggested that links = (...) is a better way to go than links = [...] [15:35] class attributes should not be mutable [15:35] but we've lots of established code that uses lists [15:35] barry: some do mutate. I was stung by that [15:35] sinzui: right. if you need an instance to override the class attribute, make a copy [15:35] barry: I advised tuples in documentation, but there are some subclasses trying to mutate I think [15:36] if some do, I'd go for the consistency over the minimal performance gains [15:36] or, if we can work them around like barry suggests, that's fine too [15:36] danilos: it's not about performance, it's about not getting bit by mutation [15:36] so, i would suggest, always default to a tuple, and rs=me for any such changes in existing code [15:36] if you /have/ to mutate in a subclass, make a copy [15:37] and make the copy nonmutable [15:37] if those rules don't work for a specific case, ping me and we'll try to figure something out [15:37] any objections? [15:37] Menus have a lot of overlap. We are doing a lot of typing to make them work. I think we need a one-true-menu-design [15:37] sinzui: indeed. but that's a whole 'nuther tasty can of worms :) [15:38] and after the migration will we change menu.py to enforce it, since it already makes a check for list or tuple? [15:38] bac: yes, we should do that (and maybe add a deprecation warning for using lists) [15:38] [ACTION] barry to update coding guidelines and look into deprecation warning in menu.py for using links = [] [15:38] ACTION received: barry to update coding guidelines and look into deprecation warning in menu.py for using links = [] [15:39] +1 for good error text if you use the wrong thing [15:39] cprov: are you here? [15:39] he's not around [15:39] [TOPIC] cprov: are [15:39] New Topic: cprov: are [15:39] er [15:40] bigjools: okay, the next three items are proposed by cprov, so i think we'll just carry those forward until next week [15:40] sounds good [15:40] that leaves 5 minutes for... [15:40] [TOPIC] peanut gallery [15:40] New Topic: peanut gallery [15:41] mmm peanuts [15:41] anything on your mind? or if not, we can take up any previous discussion from today? [15:41] just saw that you were using "cls" for the class variable [15:41] taht is good [15:41] henninge: yep. pep 8 [15:41] I have seen places where klass is still used [15:41] just mentioning [15:41] (though i had misremembered it as class_ ;) [15:41] barry: I actually thought intellectronica's topic was going to be about OCRs not giving priority to that activity and doing their own work instead. [15:41] were they discriminating against KDE users? :) [15:42] abentley: has that been a problem in practice? [15:42] bigjools: no, against Germans [15:42] ;) [15:42] barry: Now and then. [15:42] * sinzui incorporates pep8.py into his lint checker [15:43] * sinzui removed pylint [15:43] abentley: i think the ocr really has to make arrangements with another ocr if that's the case [15:43] abentley: my advice is, be shameless. if you get bad service, complain. this shouldn't be a problem [15:43] intellectronica: +1 [15:43] barry, intellectronica: Okay. [15:44] cool. i think with 1 minute left, let's call it a day. great discussions everyone. and please feel free to bring issues up on the mlist or to me [15:44] #endmeeting [15:44] Meeting finished at 09:44. [15:45] barry: Thanks. === salgado is now known as salgado-lunch === salgado-lunch is now known as salgado === salgado is now known as salgado-afk