[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 everyone and welcome to this week's ameu reviewer's meeting.  who's here today?
[15:00] <salgado> me
[15:00] <flacoste> me
[15:01] <BjornT> me
[15:01] <abentley> me
[15:02] <bac> me
[15:02] <barry> danilos, EdwinGrubb, sinzui ping
[15:02] <sinzui> me
[15:02] <danilos> me
[15:02] <EdwinGrubb> me
[15:03] <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:04] <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:05] <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:06] <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:07] <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:08] <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:09] <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:10] <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:11] <flacoste> and the trick is really that as soon as you stop and wonder...
[15:11] <flacoste> write down a question
[15:12] <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:13] <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:14] <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:15] <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:16] <barry> bigjools: +1.  we want to reduce stall
[15:17] <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:18] <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:19] <flacoste> or will suggest an improvement that will make the dev learn a new pattern or technique
[15:19] <barry> flacoste, gmb exactly
[15:20] <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:21] <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:22] <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:23] <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:24] <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:25] <barry> abentley: it's not the reviewer's responsibility to uncover design flaws, imo, that's also way too late in the process
[15:26] <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:27] <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:28] <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:29] <barry> it makes them stop and think, and maybe identify holes in their own design or code
[15:30] <barry> so anyway, i encourage you all to respond to the ml thread.  be critical and (yes you guessed it) ask question!
[15:31] <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:32] <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:33] <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:34] <barry> that's it.  any comments?
[15:35] <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:36] <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:37] <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:38] <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:39] <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:40] <sinzui> barry: right. Only one implementation example ever worked by pasting it into the code as I recall.
[15:41] <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:42] <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:43] <barry> any comments on the new format?  i'm trying to get away from the old boring scripted meeting
[15:44] <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:45] <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.