launchpad-dev team mailing list archive

[Barry Warsaw <barry@xxxxxxxxxxxxx>]

= ReviewerMeeting20090902 =

== summary ==

 * online reviews should be more interactive; use irc and voice liberally
 * +activereviews is a fallback for when you have no other work to do
 * if an ocr request comes in while you're doing a +activereview, channel your inner kiko, but finish the +activereview
 * ocr's should not "disappear for an hour or so".  be interactive!  give feedback and capture that in your review.  at least give regular status reports to your dev
 * `YouClass.links` should be a tuple.  if you think you need a list, make a copy and make sure the copy is a tuple.  rs=barry for any such change to existing code.  eventually we'll change menu.py to enforce this (may do a deprecation warning in the meantime)
 * Use `cls` as the first argument to `@classmethods` (not `klass` or `class_`)

== logs ==

=== ameu ===

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

=== asiapac ===

None, I suck.

Attachment: signature.asc
Description: PGP signature