[15:00] <barry> #startmeeting
[15:00] <MootBot> Meeting started at 09:00. The chair is barry.
[15:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[15:00] <barry> hello and welcome to this week's ameu reviewer's meeting.  who's here today?
[15:00] <sinzui> me
[15:00] <henninge> me
[15:00] <deryck_> me
[15:00] <noodles775> me
[15:00] <intellectronica> me
[15:01] <salgado> me
[15:02] <gmb> me
[15:02] <bac> me
[15:02] <barry> flacoste: adeuring gary_poster leonardr bigjools ping
[15:02] <abentley> me
[15:03] <flacoste> me
[15:03] <leonardr> me
[15:03] <bigjools> me
[15:03] <adeuring> me
[15:03] <gary_poster> me
[15:03] <barry> cprov: danilos mars allenap ping
[15:03] <danilos> me
[15:03] <barry> [TOPIC] agenda
[15:03] <MootBot> New Topic:  agenda
[15:03] <danilos> barry: thanks
[15:04] <barry> np!  it's a full agenda today...
[15:04] <barry>  * Roll call
[15:04] <barry>  * Action items
[15:04] <barry>  * UI review call update
[15:04] <allenap> me
[15:04] <barry>  * On-call review responsiveness, [intellectronica]
[15:04] <barry>  * cls.links should be a tuple, never a list (menu.py), [bigjools, barry]
[15:04] <barry>  * `__iter__()` in model objects? [cprov, barry]
[15:04] <barry>  * mkstemp()/open() combination leaks file-descriptors [cprov]
[15:04] <barry>  * Guidelines changes for tests checking systems pre-requisites (specific umasks, app configurations) [maxb, cprov]
[15:04] <barry>  * Peanut gallery (anything not on the agenda)
[15:04] <barry>  
[15:04] <barry> [TOPIC] action items
[15:04] <MootBot> New Topic:  action items
[15:04] <barry>  * beuno to add a 'ui' specialty to ReviewerSchedule
[15:04] <barry> he did this, so if you have any questions about who can do a ui review for you, check the ReviewerSchedule page
[15:05] <barry>  * gary_poster and barry will transfer review guidelines from the old wiki and old old wiki to the new wiki
[15:05] <barry> not done :(
[15:05] <gary_poster> Let's set a time.
[15:05] <gary_poster> Or at least together make a list of what needs to be done
[15:05] <barry> gary_poster: +1 we can coordinate off-channel
[15:05] <gary_poster> so we can divvy it up
[15:05] <gary_poster> cool
[15:05] <barry>  * UI review call update
[15:06] <barry> beuno's not here.  intellectronica do you or anyone else want to give an update on that c/c?
[15:06] <intellectronica> barry: i wasn't on the last one, so not me
[15:06] <barry> ah right
[15:07] <barry> 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] <flacoste> that's the most important issue i think
[15:07] <barry> ping me with any questions or problems in that area.
[15:07] <barry> your pages will /not/ currently look right until i fix certain things
[15:08] <flacoste> we clarified the rules for that
[15:08] <barry> and then you may have to go back and adjust your pages (e.g. remove heading slots, etc)
[15:08] <flacoste> did you update the wiki?
[15:08] <intellectronica> flacoste: is that written somewhere?
[15:08] <barry> flacoste: i did.  i have maybe one small round of corrections, but it's darn close
[15:08] <intellectronica> barry: url?
[15:08] <barry> i sent an email to lp-dev with details, but here's the url:
[15:09] <barry> https://dev.launchpad.net/VersionThreeDotO/UI/Conversion#Heading%20rules
[15:09] <intellectronica> thanks
[15:09] <barry> let's move on.  please read the email & wiki and respond to that thread with any feedback
[15:10] <barry> [TOPIC]  * On-call review responsiveness, [intellectronica]
[15:10] <MootBot> New Topic:   * On-call review responsiveness, [intellectronica]
[15:10] <intellectronica> on call reviews are wonderful, but i feel that sometimes people don't make the best use of them
[15:11] <intellectronica> at best, on call reviews are an approximation of pair programming, not a queuing mechanism
[15:11] <intellectronica> it's hard to set any clear rules, because reviews are subject to variation in patch size and complexity, personal style, etc
[15:12] <intellectronica> but i would just like to remind reviewers that there is a lot of value in having a very interactive reviews
[15:12] <intellectronica> at the very least, it's important to communicate what the status of a review is
[15:12] <danilos> 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] <intellectronica> 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] <abentley> intellectronica: There may be tension between wanting to capture everything in Code Review and doing it on IRC.
[15:13] <intellectronica> danilos: in very busy times, that can be a problem, but in many cases, you can literally work on the changes together
[15:13] <intellectronica> danilos: a really great way to ensure that is to do the review on the phone
[15:13] <barry> 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] <bigjools> there is another problem - the OCR queue overrides the +active-reviews queue
[15:14] <intellectronica> 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] <danilos> bigjools: I think the idea is that people should do on-call reviews, so +active-reviews is a very last resort
[15:14] <bigjools> leave a branch in the latter and it can get left behind until much later
[15:15] <abentley> 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] <intellectronica> yeah, i think of +activereviews as offline reviews, a different beast altogether
[15:15] <danilos> 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] <barry> bigjools: it should always be possible to prod a reviewer into taking one of yours on +activereviews
[15:15] <bigjools> my point is that often a branch will languish unless you get an OCR to look at it
[15:15] <danilos> bigjools: you seem to be saying that you don't like to use OCRs :P
[15:16] <bigjools> hence the problem that intellectronica points out
[15:16] <intellectronica> bigjools: you could coordinate an offline review with a reviewer who isn't on call
[15:16] <bigjools> sometimes an interactive review is totally inappropriate
[15:16] <bigjools> intellectronica: that's what I do
[15:17] <intellectronica> bigjools: absolutely, but i'm not talking about these cases
[15:17] <bigjools> I am just pointing out a possible reason for the behaviour you see
[15:17] <barry> 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] <barry> intellectronica: do you have any thoughts on improving the responsiveness, effectiveness, or throughput of reviews?
[15:19] <intellectronica> barry: one thing that i take from this discussion, is that we should make the preference for an online review explicit
[15:19] <abentley> 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] <barry> intellectronica: instead of just taking a branch off the queue and disappearing for an hour or so?
[15:19] <gary_poster> I agree with abentley, and have been so guilty
[15:19] <barry> abentley: agreed.  let's make that explicit too
[15:20] <intellectronica> 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] <abentley> gary_poster: I have also been so guilty.
[15:21] <barry> agreed.  i'll take an action item to communicate these two ideas to the team
[15:21] <barry> [ACTION] barry to communicate about +activereviews and doing on-line reviews
[15:21] <MootBot> ACTION received:  barry to communicate about +activereviews and doing on-line reviews
[15:22] <danilos> 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] <bigjools> you finish it
[15:22] <danilos> 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] <barry> danilos: last resort.  i.e. when you have nothing else on the queue, there's an implicit off-line queue in +activereviews
[15:23] <danilos> 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] <intellectronica> danilos: don't take an 800 lines branch for review from an author who is asleep if you're on call reviewer
[15:23] <bigjools> danilos: citation needed ;)
[15:23] <barry> 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] <barry> intellectronica: that too
[15:24] <bigjools> remember kiko's guide to reviewing ...
[15:24] <barry> bigjools: +1
[15:24] <danilos> 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] <bigjools> I don't get nitpicky if it's a big branch
[15:25] <barry> danilos: as on ocr customer, you should get preference over off-line reviews
[15:25] <danilos> 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] <danilos> s/offline branch/offline review/
[15:26] <flacoste> i don't agree with that
[15:26] <flacoste> if you started a review, finish it
[15:26] <flacoste> don't interrupt for online review
[15:26] <flacoste> never interrupt work
[15:26] <flacoste> it leaves work-in-progress
[15:26] <flacoste> and that's bad
[15:26] <danilos> flacoste: I agree with that, but my point is, I want OCR to be OCR
[15:26] <flacoste> is it a problem
[15:27] <barry> flacoste: that's why i say, ask yourself WWKD
[15:27] <danilos> 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] <barry> 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] <bac> WWKD about this discussion?
[15:27] <barry> :)
[15:27] <flacoste> didn't we have multiple OCR coverage
[15:27] <gary_poster> heh
[15:27] <flacoste> they can coordinate between them
[15:27] <danilos> barry: I do reviews, and all I do I do OCR
[15:27] <gary_poster> do be do be do?
[15:27] <flacoste> to make sure that one is available for interactive review
[15:28] <noodles775> On that note, thurs/euro is quite thin if anyone else is available.
[15:28] <danilos> flacoste: again, that's another requirement people have not been mentioning
[15:28] <flacoste> i also think it's fine to say, i'll have time for an interactive review in x time
[15:28] <flacoste> i need to finish this review
[15:28] <barry> danilos: that is great. it really helps!
[15:28] <abentley> communication improves most problems.
[15:29] <barry> noodles775: is not yourself and cprov on that slot?
[15:29] <noodles775> barry: cprov is no longer in Euro...
[15:29]  * barry can't keep it all straight
[15:29] <noodles775> So he's around from about 2pm my time.
[15:29] <danilos> 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] <barry> i guess we need to move cprov to thurs/west?
[15:30] <flacoste> danilos: i'm not sure we were successfull about the pre-impl with OCR
[15:30] <danilos> anyway, I'd say this: if there's sufficient OCR coverage for people asking for interactive reviews, please go for +activereviews
[15:30] <abentley> danilos: It depends on the situation, but I usually want someone who knows the domain intimately for a pre-imp.
[15:30] <flacoste> and i've come to think that random pre-impl aren't that effective
[15:30] <bigjools> flacoste: I agree
[15:31] <flacoste> better to have a pre-impl with somebody with some knowledge in the area
[15:31] <danilos> flacoste, abentley: I am only talking about rough "pre-imp"... sometimes you'd like to discuss a few things mid-implementation
[15:31] <flacoste> i think it's fine not to put that responsability on OCR
[15:31] <barry> 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] <flacoste> well, pre-impl works if you do them
[15:32] <danilos> 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] <flacoste> but pre-impl is a separate topic
[15:32] <flacoste> danilos: sure, that is right
[15:32] <intellectronica> 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] <barry> 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] <barry> flacoste: but yeah, this is off-topic and we're running out of time ;)
[15:33] <danilos> sorry everyone for making this longer than everybody expected :)
[15:33] <barry> danilos: yes, please do!
[15:34] <barry> thanks everybody for this excellent topic.  i think we can continue to discuss it but i'd like to move on now
[15:34] <barry> [TOPIC]  * cls.links should be a tuple, never a list (menu.py), [bigjools, barry]
[15:34] <MootBot> New Topic:   * cls.links should be a tuple, never a list (menu.py), [bigjools, barry]
[15:35] <barry> 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] <barry> class attributes should not be mutable
[15:35] <barry> but we've lots of established code that uses lists
[15:35] <sinzui> barry: some do mutate. I was stung by that
[15:35] <barry> sinzui: right.  if you need an instance to override the class attribute, make a copy
[15:35] <sinzui> barry: I advised tuples in documentation, but there are some subclasses trying to mutate I think
[15:36] <danilos> if some do, I'd go for the consistency over the minimal performance gains
[15:36] <danilos> or, if we can work them around like barry suggests, that's fine too
[15:36] <barry> danilos: it's not about performance, it's about not getting bit by mutation
[15:36] <barry> so, i would suggest, always default to a tuple, and rs=me for any such changes in existing code
[15:36] <barry> if you /have/ to mutate in a subclass, make a copy
[15:37] <barry> and make the copy nonmutable
[15:37] <barry> if those rules don't work for a specific case, ping me and we'll try to figure something out
[15:37] <barry> any objections?
[15:37] <sinzui> 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] <barry> sinzui: indeed.  but that's a whole 'nuther tasty can of worms :)
[15:38] <bac> and after the migration will we change menu.py to enforce it, since it already makes a check for list or tuple?
[15:38] <barry> bac: yes, we should do that (and maybe add a deprecation warning for using lists)
[15:38] <barry> [ACTION] barry to update coding guidelines and look into deprecation warning in menu.py for using links = []
[15:38] <MootBot> ACTION received:  barry to update coding guidelines and look into deprecation warning in menu.py for using links = []
[15:39] <bigjools> +1 for good error text if you use the wrong thing
[15:39] <barry> cprov: are you here?
[15:39] <bigjools> he's not around
[15:39] <barry> [TOPIC] cprov: are
[15:39] <MootBot> New Topic:  cprov: are
[15:39] <barry> er
[15:40] <barry> bigjools: okay, the next three items are proposed by cprov, so i think we'll just carry those forward until next week
[15:40] <bigjools> sounds good
[15:40] <barry> that leaves 5 minutes for...
[15:40] <barry> [TOPIC] peanut gallery
[15:40] <MootBot> New Topic:  peanut gallery
[15:41] <bigjools> mmm peanuts
[15:41] <barry> anything on your mind?  or if not, we can take up any previous discussion from today?
[15:41] <henninge> just saw that you were using "cls" for the class variable
[15:41] <henninge> taht is good
[15:41] <barry> henninge: yep.  pep 8
[15:41] <henninge> I have seen places where klass is still used
[15:41] <henninge> just mentioning
[15:41] <barry> (though i had misremembered it as class_ ;)
[15:41] <abentley> 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] <bigjools> were they discriminating against KDE users? :)
[15:42] <barry> abentley: has that been a problem in practice?
[15:42] <henninge> bigjools: no, against Germans
[15:42] <henninge> ;)
[15:42] <abentley> barry: Now and then.
[15:42]  * sinzui incorporates pep8.py into his lint checker
[15:43]  * sinzui removed pylint
[15:43] <barry> abentley: i think the ocr really has to make arrangements with another ocr if that's the case
[15:43] <intellectronica> abentley: my advice is, be shameless. if you get bad service, complain. this shouldn't be a problem
[15:43] <barry> intellectronica: +1
[15:43] <abentley> barry, intellectronica: Okay.
[15:44] <barry> 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] <barry> #endmeeting
[15:44] <MootBot> Meeting finished at 09:44.
[15:45] <abentley> barry: Thanks.