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