=== mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell === salgado-afk is now known as salgado [14:57] hey barry [14:57] jtv: hi [15:00] #startmeeting [15:00] hello everyone and welcome to this week's ameu reviewer's meeting. who's here today? [15:00] me [15:00] Reviewers meeting? I came here for the Revue Girls Meeting... [15:00] me [15:01] me [15:01] jtv: it immediately follows this meeting. [15:01] sinzui: ta [15:01] me [15:01] abentley: "me" :) [15:01] me me me [15:01] me [15:02] me [15:02] me [15:02] me [15:02] danilos: flacoste ping [15:03] me [15:03] salgado: ping [15:03] barry: danilos was going to be here [15:03] me! [15:03] intellectronica sends apologies [15:03] jtv: thanks [15:03] i think that's everyone... [15:04] == Agenda == [15:04] * Roll call [15:04] * Next meeting [15:04] * Action items [15:04] * Queue status [15:04] * Mentoring update [15:04] * Review process [15:04] * watch for dead end links ([[https://bugs.edge.launchpad.net/launchpad/+bug/254436|bug 254436]] - flacoste) [15:04] Launchpad bug 254436 in launchpad-foundations "Launchpad contains lots of dead-end links" [Undecided,Incomplete] [15:04] Launchpad bug 254436 in launchpad-foundations "Launchpad contains lots of dead-end links" [Undecided,Incomplete] https://launchpad.net/bugs/254436 [15:04] * extended demo explanation in cover letter (salgado, celso) [15:04] [TOPIC] next meeting [15:04] week +=1 ? [15:04] if you can't make it, please update ReviewerMeetingAgenda with your apologies [15:05] [TOPIC] * Action items [15:06] * sinzui to put XXX report into Makefile [15:06] Done. [15:06] sinzui: awesome, thanks [15:06] * barry to propose new bug tag for tech debt paydown [15:06] done [15:06] * barry remembers that abentley already updated UsingMergeProposals [15:07] [TOPIC] * Queue status [15:07] don't forget to cleanup your PR branches when they land (he says as an offender) [15:07] only 4 pink branches [15:08] danilos: any word on stub/launchpad/trivial? [15:08] barry: haven't heard from stub, it's needs-reply [15:08] danilos: ah, the PR status just hasn't been updated then? [15:08] barry: that's right [15:09] danilos: cool, thanks for the update [15:09] the PR queue doesn't look too bad. does anybody have comments on the review queue? [15:10] it looked bad yesterday, thanks to bac for doing most of the work on it [15:10] * barry applauds bac! [15:10] actually it wasn't *that* bad [15:10] apologies for slacking on monday due to u.s. holiday [15:11] [TOPIC] * Mentoring update [15:11] abentley: i think you're the only mentat right now. anything you want to mention? [15:12] No, haven't done anything since last week. [15:12] abentley: cool [15:13] let me just mention that i think we have slots available, if anybody has ideas for new recruits, please let me know [15:14] [TOPIC] * Review process [15:14] * watch for dead end links ([[https://bugs.edge.launchpad.net/launchpad/+bug/254436|bug 254436]] - flacoste) [15:14] Launchpad bug 254436 in launchpad-foundations "Launchpad contains lots of dead-end links" [Undecided,Incomplete] https://launchpad.net/bugs/254436 [15:14] Launchpad bug 254436 in launchpad-foundations "Launchpad contains lots of dead-end links" [Undecided,Incomplete] [15:14] flacoste: the floor is yours [15:15] flacoste: ^^ [15:15] that bug describe an issue where we have alot of dead end links [15:15] link to empty pages [15:15] Pages with structure and nothing else? [15:16] so that's something we need to look for in reviews [15:16] UI review ideally [15:16] flacoste: can you reiterate here what you mentioned in our standup meeting, re: ui reviews? [15:17] sure [15:18] actually, it's more about ui pre-implementation [15:18] so I discussed with beuno last week how he wanted to interact with the team for UI considerations [15:19] i suggested that when we work on a feature involving UI it would be great if he could join the pre-implentation call [15:19] to discuss the UI aspect [15:19] he agreed that it would be a good idea [15:19] so it's something to try out [15:20] so if somebody asks you for a pre-impl and it involves UI [15:20] try to get beuno involved [15:21] flacoste: +1 ...done? [15:21] yes [15:21] flacoste: thanks, i'm really glad beuno will be more involved in ui preimps [15:22] * extended demo explanation in cover letter (salgado, celso) [15:22] salgado, cprov-afk the floor is yours [15:22] celso? [15:22] I'm not sure what this is about === cprov-afk is now known as cprov [15:22] okay, I think I know [15:22] right, I know [15:23] * Salgado and Celso will propose an extended version of the current DemoURL section in our review cover-letter. Most of the times features and/or bugfixes are not properly expressed by a single URL, they involve a sequence of steps (a story) and should be described like that. This way, we expect reviewers (and readers in general) to observe the proposed change in a more holistic aspect, possibly catching details that will result in a [15:23] more consistent revision. [15:23] * sinzui often needs to state several URLs and which user to use. [15:23] * barry too [15:24] * sinzui also needs to remark that there is no URL, check the oops reports or database. [15:24] exactly, most of the times a single URL is not enough to represent an UI change. [15:24] I think this is different than a demo URL [15:24] I see the demo URL as something to show a new UI you added [15:24] me [15:24] Maybe a "how to reproduce/demonstrate" section? [15:24] shouldn't this be captured in a story doctest in the branch? [15:25] barry, in general it is [15:25] e.g. "to demonstrate, read .../story/frobnicate.txt" [15:25] But it's nice to have a quick guide to doing it interactively [15:25] This issue relates to how we verify this works on staging. [15:26] jtv: maybe story doctests should have a summary section at the top? [15:26] barry: they should do that already [15:26] jtv: +1 [15:26] barry: but make it too long and it becomes a drag on development [15:27] jtv: if you've got to write it anyway, put it in the test and keep it short and sweet [15:27] jtv: followed by detailed user story [15:27] I think the demo section should be transformed into a statement about how to verify the feature/fix in development, staging, production. [15:27] Maybe the problem here is that our doctests and pagetest stories are too long to get a good overview. [15:27] jtv: yes, definitely a problem already identified [15:28] A bug fix is does not necessarily change a user story [15:28] sinzui: that's different. that's like a q/a plan [15:28] I don't think this id different [15:28] sinzui: I like this idea, a guide to reproduce the problem in production and check the fix in development then in staging/edge when it gets committed [15:29] sinzui: different in the sense: you probably wouldn't put that in our tree [15:29] What is the purpose of providing demo steps if it is not to verify that the code works as expected [15:29] sinzui: you'd put that in our q/a artifacts [15:29] barry: that is exactly what I am saying [15:30] sinzui: then we agree! :) [15:30] a bug probably does not have a use story because we captured the story when created the feature [15:30] sinzui: this is a good idea because we've already said we want two people to do q/a for each issue [15:30] sinzui: For UI, say, it gives the reviewer an immediate handle on the end result, then they can review the changes in that light. [15:30] barry: i agree it's q/a, the idea is to introduce those artifacts earlier in the development workflow. [15:31] cprov: +1. so where/how do we capture these? wiki? pastebin? tree? [15:32] barry: i'd say it must be in the review cover-letter [15:32] review-proposal summary, to be precise. [15:32] cprov: it's probably not a bad thing to begin to hash out during preimp either [15:33] iow, during pre-imp ask how you would q/a the change [15:33] i like these ideas [15:33] discussing qa during pre-impl [15:33] barry: agreed, it should be considered as part of 'understanding/reproducing the problem' [15:33] and putting the QA validation steps in the cover letter [15:34] +1 [15:34] +1 [15:34] * abentley is a bit worried about how much we keep saying "pre-impl" [15:34] In my experience that doesn't happen much. [15:34] abentley: make it happen [15:34] * sinzui does [15:34] * gmb does, too [15:34] * abentley lacks a time machine. [15:35] * sinzui uses good habits to compensate for poor skilz [15:35] salgado, cprov i'd like to capture this in our documentation. can i give one of you the following action: email the list and add it to the wiki? [15:35] barry: sure [15:35] sinzui: do you suggest, as a reviewer, outright rejection of non preimp'd (and q/a planned) branches? [15:35] cprov: thanks [15:35] thanks for taking it, cprov [15:35] To be clear, I'm speaking as a reviewer, not a submitter. All *my* stuff has a pre-impl. [15:36] [ACTION] cprov will email the list and update the wiki, re: review q/a plan during preimp,and include that in the cover letter [15:36] salgado: np, I reckon you already have enough in your plate ;) [15:36] barry: I think Rinchen might want that. QA signoff after landing is in our process [15:36] abentley: It's not unheard of for a reviewer to say "this is bong; go and have a call with someone about it..." [15:37] Though with OCR that seems to happen less, AFAICT [15:37] Also, I think that open-sourcing Launchpad will probably reduce the likelihood of pre-impls. [15:37] gmb: that's been my observation as well, but as a reviewer, i've let it slide (probably too much) [15:38] abentley: tasty can of worms there [15:38] Om nom nom [15:38] so, should we hard-assed about requiring pre-imps? [15:39] and q/a plans? [15:39] I think, re: pre-imps, it really depends on branch size. [15:39] gmb: +1 [15:39] barry: I was a guest in the bzr stand-up, and it's clear they think pre-impls won't happen in a real open-source project. [15:39] i think the only thing we should be hard-assed about is rejecting bad branches [15:39] For less than, say, 200 lines of diff, a pre-imp might not be necessary (particularly if it's mopstly removals) [15:39] one thing i'm worried about is possible effect on increasing overhead (and thus discouraging) cleanup branches [15:39] I hate to be a Nazi on this issue, but this issue comes up every month. We need to take some form of action to improve the quality of our branches. [15:40] if pre-impl avoids bad branches, then people will do it [15:40] Right. [15:40] because otherwise they'll be rejected in review [15:40] Even a mid-imp is better than nothing, because at least someone has said "yes, your approach is sane" [15:40] Which isn't the reviewer's first job [15:40] Since sanity can be domain specific. [15:41] hmm [15:41] good point [15:41] * abentley cannot judge sanity of 75% of things submitted to him for review. [15:41] (So, Soyuz makes my brain bleed, but when bigjools says he's discussed a change with cprov I can accept there's some sanity to it) [15:41] flacoste, sinzui maybe reviewers should be more willing to punt on a branch early if it's had no preimp, lacks a q/a plan, looks like crack [15:41] i think abentley and gmb have a valid points here [15:41] barry: I have done that as a sanity check [15:41] gmb: and 'vice-versa' ;) [15:42] cprov: No, I trust *you* implicitly ;) [15:42] abentley: i agree. in mailman, we occasionally have email discussions about approaches, but it's mostly rare [15:42] * gmb waits for bigjools' boot to descend [15:42] gmb: historically, I've done the most insane changes, you know that :) [15:42] flacoste: i agree, all good points [15:42] * bigjools -> winds up [15:42] I expect someone on my team to accept my tests and implementation plan. I also expect that person to agree with how many branches it will take to complete. [15:43] sinzui, abentley so preimps are probably best done with team mates, rather than the general lpdev population? [15:44] then reviewers with less domain knowledge will have more confidence that the approach is sound, and will have a q/a plan in the cover letter to guide them [15:44] barry: Yes. And reviews submitted to team mates are less likely to need pre-impl. [15:44] barry: definitely, unless you're unsure of your footing in some other important area of your branch. [15:44] barry: often, but I have used BjornT for example when he was person with the most experience with the issue. [15:44] abentley: i still don't want to give up on reviewers can review anything [15:45] jtv: yes, you might need multiple pre-imps (i've done that) [15:45] * sinzui tends to bounce around domains. [15:45] sinzui: good point [15:45] barry: Reviewers *can* review anything. But, for example, if allenap reviews one of my branches, he doesn't necessarily need me to have had a pre-impl because he can call me on its crack-ness pretty quickly. [15:45] barry: I'm not suggesting giving up on that, just noting a corollary. [15:45] gmb: good point [15:45] barry: In which case pre/mid-imp and review become one. [15:46] gmb: well [15:46] (Although that's not an excuse to not have had one, I'll grant you) [15:46] the point of the pre-implentation is not only to bounce crack branch at review time [15:46] * barry apologies for going over 45 minutes, but wants to let this good discussion continue for a bit [15:46] but to make sure that you don't work on crack branch [15:46] it's about building quality in [15:46] if pre-impl ensures quality in, we should do it more [15:46] flacoste: You're right, of course. [15:47] if not, we should drop it as waste [15:47] flacoste: and also to not waste time [15:47] reduce waste [15:47] flacoste: +1 [15:47] I think it's definately a positive that we have pre-imps. BjornT has saved me from myself many times during a pre-imp ;) [15:48] amen [15:48] so we've run over. i'd like to carry this forward on the main lp list, so that we can come up with action items next week [15:48] [ACTION] barry will move the preimp discussion to the ml [15:49] thank you everyone for some very good points [15:49] apologies for going long [15:49] #endmeeting [15:49] yay! [15:49] Thanks barry [15:49] thanks barry === salgado is now known as salgado-lunch === ursula_ is now known as Ursinha === salgado-lunch is now known as salgado === cprov is now known as cprov-afk === thumper_laptop is now known as thumper === salgado is now known as salgado-afk