| === 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 | ||
| 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:00 |
| salgado | me | 15:01 |
| gmb | me | 15:02 |
| bac | me | 15:02 |
| barry | flacoste: adeuring gary_poster leonardr bigjools ping | 15:02 |
| abentley | me | 15:02 |
| 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:03 |
| 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:04 |
| 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:05 |
| 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:06 |
| 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:07 |
| 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:08 |
| 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:09 |
| 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:10 |
| 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:11 |
| 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:12 |
| 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:13 |
| 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:14 |
| 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:15 |
| 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:16 |
| 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:17 |
| barry | intellectronica: do you have any thoughts on improving the responsiveness, effectiveness, or throughput of reviews? | 15:18 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| 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:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:31 |
| 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:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| 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:36 |
| 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:37 |
| 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:38 |
| 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:39 |
| 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:40 |
| 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:41 |
| 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:42 | |
| * 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:43 |
| 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:44 |
| abentley | barry: Thanks. | 15:45 |
| === salgado is now known as salgado-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!