| === mwhudson_ is now known as mwhudson | ||
| === mrevell is now known as mrevell-lunch | ||
| === salgado-afk is now known as salgado | ||
| === mrevell-lunch is now known as mrevell | ||
| 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 everyone and welcome to this week's ameu reviewer's meeting. who's here today? | 15:00 |
| salgado | me | 15:00 |
| flacoste | me | 15:00 |
| BjornT | me | 15:01 |
| abentley | me | 15:01 |
| bac | me | 15:02 |
| barry | danilos, EdwinGrubb, sinzui ping | 15:02 |
| sinzui | me | 15:02 |
| danilos | me | 15:02 |
| EdwinGrubb | me | 15:02 |
| === EdwinGrubb is now known as EdwinGrubbs | ||
| barry | gmb: ping? | 15:03 |
| gmb | me | 15:03 |
| * sinzui sips lovely coffee | 15:03 | |
| gmb | barry: Oops, sorry. | 15:03 |
| bigjools | me | 15:03 |
| barry | cprov-afk: ping | 15:03 |
| barry | anybody else missing? | 15:03 |
| cprov-afk | me | 15:04 |
| barry | [TOPIC] agenda | 15:04 |
| MootBot | New Topic: agenda | 15:04 |
| barry | * Roll call | 15:04 |
| barry | * The Kiko way to quick and easy reviews (barry, kiko) | 15:04 |
| barry | * Pre-implementation calls | 15:04 |
| barry | * If there's time, the old boring script | 15:04 |
| barry | * Next meeting | 15:04 |
| barry | * Action items | 15:04 |
| barry | * Queue status | 15:04 |
| barry | * Mentoring update | 15:04 |
| barry | i want to try something different today | 15:04 |
| barry | i want to talk about the fun stuff first, and then do the boring scripted stuff last if there's time. any objections? | 15:05 |
| barry | great! | 15:05 |
| barry | :) | 15:05 |
| barry | [TOPIC] quick and easy reviews | 15:05 |
| MootBot | New Topic: quick and easy reviews | 15:05 |
| barry | did anybody /not/ get a chance to read the message i sent around this morning on this topic? | 15:05 |
| bac | me | 15:06 |
| abentley | me | 15:06 |
| barry | (it's not a problem of course :) | 15:06 |
| flacoste | better than that, i tried it out! | 15:06 |
| barry | flacoste: excellent! i'll ask you about your feedback in a bit | 15:06 |
| barry | so let me try to summarize... | 15:06 |
| barry | we want to make reviewing quick, easy, and fun | 15:07 |
| barry | kiko, flacoste and i have been talking about this a bit so here are some key insights: | 15:07 |
| barry | - reviews are a mediocre qa tool, but an excellent learning tool | 15:07 |
| barry | - don't research, ask questions | 15:08 |
| barry | - ask more questions | 15:08 |
| barry | - improve your code scanning abilities; identify changes, ask a question and move on | 15:08 |
| barry | - concentrate on stuff that looks weird. if you see something that looks weird... ask a question! | 15:08 |
| barry | there's more in the email but the goal is this: | 15:08 |
| barry | medium sized branches should take around 15 minutes to review. large branches no longer than 30 minutes | 15:09 |
| barry | first let me ask flacoste to give his impressions, having been our trail blazer, then let's open this to some discussion... | 15:09 |
| barry | flacoste: ? | 15:09 |
| flacoste | i agree, it was fun and fast | 15:10 |
| flacoste | well | 15:10 |
| flacoste | it was a 2000 lines branch | 15:10 |
| flacoste | so it still took 1.5h | 15:10 |
| flacoste | but still | 15:10 |
| barry | ! | 15:10 |
| flacoste | i'm looking forward to seeing stub's reply | 15:10 |
| flacoste | but there was a lot of questions in there | 15:10 |
| * barry spends that much time on an 800 line branch | 15:10 | |
| flacoste | and the trick is really that as soon as you stop and wonder... | 15:11 |
| flacoste | write down a question | 15:11 |
| barry | flacoste: great point: trust your gut. i've been trying to train myself that if the question pops in my mind, don't suppress it | 15:12 |
| barry | ask it | 15:12 |
| barry | flacoste: anything else? | 15:12 |
| flacoste | not really, it's that simple | 15:13 |
| flacoste | but like i said this means that all initial reviews are needs-reply | 15:13 |
| flacoste | which is fine, but i'll see how it unfolds | 15:13 |
| barry | flacoste: thanks. okay. i'll open the floor for discussion | 15:14 |
| abentley | I've found that people are slow to answer questions when doing on-call reviews. The process just drags. | 15:14 |
| abentley | Also, if the main purpose of reviews is QA, and they're a mediocre tool, are they worth it? | 15:15 |
| sinzui | I have found developers more than happy to explain how I'm wrong during on-call reviews. | 15:15 |
| bigjools | then go to the next review, that's the dev's problem not yours. OCRs are meant for quick reviews. | 15:15 |
| barry | bigjools: +1. we want to reduce stall | 15:16 |
| barry | and remember, it's the dev's responsibility to shepherd their branch, and to respond to questions | 15:17 |
| bigjools | exactly | 15:17 |
| barry | abentley: review is never going to make a branch perfect anyway | 15:17 |
| flacoste | the main purpose of a review isn't QA | 15:18 |
| gmb | We can't catch the majority of annoying bugs using reviews. We *can* prevent a lot of stupid bugs by doing them. | 15:18 |
| abentley | barry: Yeah, but a sloppy review isn't even going to prevent the quality of launchpad's codebase from deteriorating. | 15:18 |
| barry | abentley: kiko's insight is that reviews are for learning and questioning first. it's by the process of questioning that the dev begins to re-examine some of the assumptions they've made | 15:18 |
| flacoste | but to ensure some uniformity in code | 15:18 |
| flacoste | and to improve the quality of the code | 15:18 |
| flacoste | and also to spread knowledge around | 15:18 |
| flacoste | that last point is probably the most important | 15:18 |
| flacoste | a lot of ideas will cross-pollinate through reviews | 15:18 |
| flacoste | where the reviewer will either learn a new technique by looking at the code | 15:18 |
| flacoste | or will suggest an improvement that will make the dev learn a new pattern or technique | 15:19 |
| barry | flacoste, gmb exactly | 15:19 |
| gmb | Actually, I'd *really* like us to emphasise that reviews are not a bug prevention tool. | 15:20 |
| bigjools | something that we do in Soyuz is a "pre-review" with a domain expert, it's just a quick look at the code to make sure it's doing what we all expect it to do before a real review | 15:20 |
| barry | abentley: i think more emphasis on pre/mid-impls and continuous qa will do more to improve quality | 15:20 |
| gmb | Because doing a review and trying to look for bugs makes life really, really hard for the reviewer, and hurts my brain. | 15:20 |
| barry | bigjools: that's very interesting | 15:21 |
| gmb | The checks for a review should be: | 15:21 |
| bigjools | so maybe pre-review to look for bugs, main review for the big picture? | 15:21 |
| gmb | * Does the code do what it says it does (needs a domain expert sometimes) | 15:21 |
| gmb | * Is the code covered in tests | 15:21 |
| gmb | * If there's something weird, why is it weird? | 15:21 |
| gmb | * Does to code conform to our standards. | 15:21 |
| abentley | gmb: bugs are hard to catch in review. Lack of test coverage is easier. Design flaws easier still. | 15:21 |
| gmb | abentley: Right. Design flaws should be caught in the pre-imp. | 15:22 |
| sinzui | gmb: I also look for: does the code reinvent what we already have? | 15:22 |
| gmb | sinzui: That's not always easy if you don't know much about domain context though. | 15:22 |
| abentley | gmb: IME, pre-imp focuses on "what", not "how". | 15:22 |
| gmb | abentley: I always try to come to a pre-imp ready-to-code, which means I have tests, which means I have a design. | 15:23 |
| sinzui | gmb: Reviewers can can see outside the domain, where common problems are solved. | 15:23 |
| barry | gmb: though you /can/ sniff out refactoring potential even if you don't know specifically. if in doubt... ask! | 15:23 |
| gmb | True enough. | 15:23 |
| gmb | And I confess I like weilding the YAGNI bat every once in a while. | 15:23 |
| abentley | gmb: I don't. And I don't agree that having tests means you have enough design to be sure there aren't design flaws. | 15:24 |
| barry | abentley: it's not the reviewer's responsibility to uncover design flaws, imo, that's also way too late in the process | 15:25 |
| kiko | abentley, sure, but what impact could that have on review? the most you can say "this code looks like crap. how can we improve it?" | 15:26 |
| barry | kiko: and if so, ask that question and kick it out quickly | 15:26 |
| bigjools | barry: but it's a useful backtop | 15:26 |
| abentley | kiko: You can say "doing it that way will scale badly and fail in production. Have you considered doing it this way?" | 15:26 |
| kiko | abentley, sure, and that's insightful, but if it was me I would not even state, and just ask "how well will this scale in production, considering we have 1500 neutrinos being frobbed every run of the proton accelerator?" | 15:27 |
| kiko | I always ask | 15:27 |
| kiko | let the coder figure out the answer | 15:27 |
| barry | bigjools: yes, but only as a sanity check. iow, if it smells bad, it's the dev's burden of proof | 15:28 |
| kiko | and TBH I don't care too much if it works or not -- I just care that I get a reasonable answer from the coder | 15:28 |
| bigjools | barry: agreed | 15:28 |
| barry | it makes them stop and think, and maybe identify holes in their own design or code | 15:29 |
| barry | so anyway, i encourage you all to respond to the ml thread. be critical and (yes you guessed it) ask question! | 15:30 |
| barry | cprov-afk: has to leave early and sends his apologies | 15:31 |
| barry | [TOPIC] pre-impls | 15:31 |
| MootBot | New Topic: pre-impls | 15:31 |
| barry | i have just a few thoughts, which i'm going to put into an email after this meetingggg | 15:31 |
| barry | so you're getting the raw brain dump :) | 15:32 |
| barry | - we want to include beuno in pre-impls concerning u/i. do a 3-way call with him to sanity check th eui | 15:32 |
| barry | - is it okay to bounce branches that have no pre-impl? | 15:32 |
| barry | - btw, a pre-impl can also be a mid-impl | 15:32 |
| barry | - include the q/a plan in the cover letter (cprov's email which i havent' read yet) | 15:33 |
| barry | - no pre-imp, no on-call review? | 15:33 |
| barry | - one exception: cleanup branches. do 'em fast, get 'em reviewed fast, land it... JFDI | 15:33 |
| barry | that's it. any comments? | 15:34 |
| === kiko is now known as kiko-phone | ||
| sinzui | When we do UI changes, do we include a sketch? | 15:35 |
| BjornT | i think it's ok to bounce branches that have no pre-impl call, if they have design flaws. if not, no reason to bounce. | 15:35 |
| barry | sinzui: are you a good artist? :) | 15:36 |
| barry | BjornT: good point. a lot depends (for me) on the cover letter. if it's detailed enough that i understand the tradeoffs and design decisions, i might let it pass | 15:36 |
| barry | if not, bounce and move on | 15:36 |
| barry | sinzui: i'm not sure how beuno wants to work re: ui review prep. we'll have to ask. or do you mean in the cover letter? | 15:37 |
| sinzui | barry: I write tests and implementation before I do a preimp. I image I would also be using ascii art to illustrate behaviour. | 15:38 |
| sinzui | s/implementation/implementation examples/ | 15:39 |
| barry | sinzui: ah. is that because you're trying to figure out how to attack the issue? | 15:39 |
| barry | i ask because i try not to write anything before having the pre-imp, except notes & cover | 15:39 |
| sinzui | barry: right. Only one implementation example ever worked by pasting it into the code as I recall. | 15:40 |
| sinzui | barry: I use tests to set scope, judge the number of branches I need, and to establish ne APIs. | 15:41 |
| barry | sinzui: i agree that all those thing make for a solidly researched branch | 15:41 |
| sinzui | I suppose I am really doing a mid-imp since I may have the branch complete before I talk to someone. | 15:41 |
| barry | sinzui: which is fine :) | 15:42 |
| barry | okay, we have just a couple of minutes left. as i said, i'll take this to the ml too | 15:42 |
| barry | any comments on the new format? i'm trying to get away from the old boring scripted meeting | 15:43 |
| flacoste | works from now | 15:44 |
| flacoste | but it's not like the old scripted one | 15:44 |
| flacoste | was taking most of the meeting | 15:44 |
| barry | flacoste: yeah, but i like the meat up front :) | 15:44 |
| flacoste | it's just that the first common 10 minutes are at the end instead of the beginning | 15:45 |
| barry | anyway, we're out of time, so thanks everybody! | 15:45 |
| barry | #endmeeting | 15:45 |
| MootBot | Meeting finished at 09:45. | 15:45 |
| === kiko-phone is now known as kiko | ||
| === salgado is now known as salgado-lunch | ||
| === kiko is now known as kiko-fud | ||
| === salgado-lunch is now known as salgado | ||
| === kiko-fud is now known as kiko | ||
| === salgado is now known as salgado-afk | ||
| === kiko is now known as kiko-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!