| mrevell | ning! | 09:39 |
|---|---|---|
| bigjools | moo | 15:00 |
| barry | #startmeeting | 15:00 |
| barry | tbot | 15:00 |
| barry | hello everyone and welcome to this week's ameu reviewers meeting. who's here today? | 15:00 |
| BjornT | me | 15:00 |
| bigjools | me | 15:00 |
| EdwinGrubbs | me | 15:01 |
| sinzu1 | me | 15:01 |
| bac | me | 15:01 |
| === sinzu1 is now known as sinzui | ||
| sinzui | me | 15:01 |
| allenap | me | 15:01 |
| flacoste | me | 15:01 |
| gmb | me | 15:01 |
| schwuk | me | 15:01 |
| flacoste | salgado is on leave | 15:01 |
| BjornT | intellectronica is also on leave | 15:02 |
| barry | danilo_: ping | 15:02 |
| barry | * (Julian) Since I seem to be finding it hard to get my Soyuz comrades to follow our own informal coding standards, when reviewing Soyuz code please make sure you don't let code of the form: ` if archive.purpose == ArchivePurpose.PPA:` land, instead it should be the simpler: `if archive.is_ppa:` which not only encapsulates the decision in IArchive, it should remove an import of the DBEnum. | 15:02 |
| barry | erg | 15:02 |
| bigjools | ahem | 15:02 |
| barry | == Agenda == | 15:03 |
| barry | * Roll call | 15:03 |
| barry | * Next meeting | 15:03 |
| barry | * Action items | 15:03 |
| barry | * Queue status | 15:03 |
| barry | * Mentoring update | 15:03 |
| barry | * Review process | 15:03 |
| barry | * Help people learn how big branches can be split up (BjornT) | 15:03 |
| barry | * (Julian) Since I seem to be finding it hard to get my Soyuz comrades to follow our own informal coding standards... | 15:03 |
| * barry sighs | 15:03 | |
| barry | * Next meeting | 15:03 |
| barry | everyone okay with week += 1 | 15:03 |
| bigjools | yep | 15:03 |
| barry | anybody know you'll be sprinting or away? | 15:04 |
| barry | * Action items | 15:04 |
| * barry had an action item to communicate our multiline guidelines | 15:04 | |
| barry | i've updated PythonStyleGuide and will send an email to the list after this meeting | 15:05 |
| * schwuk reads the updated wiki page | 15:05 | |
| barry | * barry to solicit ideas to better handle review scheduling and workload | 15:05 |
| barry | FAIL | 15:05 |
| statik | me (sorry I'm late) | 15:05 |
| barry | schwuk: i welcome yours (and everyone's) feedback! | 15:05 |
| barry | * flacoste to add `safe_hasattr()` to `lazr.utils` | 15:05 |
| barry | flacoste: ? | 15:06 |
| flacoste | i still suck, please keep it | 15:06 |
| barry | k | 15:06 |
| barry | * bjorn to add recommendation to test style guide saying don't use asserts in doctests | 15:06 |
| barry | BjornT: ^^ | 15:07 |
| BjornT | oops, not done | 15:07 |
| barry | BjornT: np, we'll keep it on the agenda | 15:07 |
| barry | * Queue status | 15:08 |
| barry | it's week 4 so a light week for branches i think | 15:08 |
| barry | EdwinGrubbs: how's that adeuring branch going? | 15:08 |
| * bigjools has got a corker Soyuz branch coming up today | 15:09 | |
| EdwinGrubbs | barry: I was waiting for the dependent branch to be finished being reviewed. | 15:09 |
| schwuk | EdwinGrubbs: is that the hwdb one? | 15:09 |
| EdwinGrubbs | yes | 15:09 |
| EdwinGrubbs | is that done? | 15:09 |
| schwuk | If so, -1 has just gone to merge-conditional* and it's waiting on sinzui's attention. | 15:09 |
| schwuk | However I'm sure we'll revisit this branch in BjornT's agenda item... | 15:10 |
| barry | EdwinGrubbs, schwuk thanks | 15:11 |
| barry | we have 3 other ping branches: reviewers = flacoste, jtv, allenap | 15:11 |
| flacoste | barry: mine is done | 15:11 |
| barry | s/ping/pink/ | 15:11 |
| barry | flacoste: excellent, thanks | 15:11 |
| flacoste | need to update the status | 15:11 |
| allenap | doing that one today, sorry gmb for being slow. | 15:11 |
| barry | allenap: thanks | 15:12 |
| barry | anything else on the queue status? | 15:12 |
| gmb | allenap: I hadn't noticed through the Ubuflu fog. | 15:12 |
| allenap | gmb: Get well soon, but not before I've done your review. | 15:12 |
| gmb | Thanks, I think. | 15:12 |
| barry | * Mentoring update | 15:13 |
| sinzui | schwuk: I'll follow up your review today while I lobby for an RC. | 15:13 |
| barry | since it's week 4, if you have any recommendations for graduations or recruits, send them my way | 15:13 |
| bigjools | I did one for jml at UDS but otherwise it's been lean for me this cycle because of UDS and holidays | 15:14 |
| barry | other than that, any comments from mentors or mentorees? | 15:14 |
| barry | bigjools: that's my impression too | 15:14 |
| bigjools | barry: I'll get some done next Monday | 15:15 |
| schwuk | Not really, although week 3 was kind of lean for me (apart from the hwdb branch) as people tend to prefer non-mentored reviews. Especially on Friday :) | 15:15 |
| barry | bigjools: cool. sorry i wasn't ocr this monday; us holiday | 15:15 |
| barry | schwuk: of week 3, eh? :) | 15:15 |
| bigjools | I seek out mentored reviews if it's not urgent - I get the benefit of two pairs of eyes | 15:15 |
| barry | schwuk: but really, we need to make sure mentorees have enough branches to review | 15:16 |
| bigjools | barry: no problem re: ocr, I was at UDS anyway | 15:16 |
| barry | cool | 15:16 |
| barry | any other mentoring updates? | 15:17 |
| barry | * Review process | 15:17 |
| barry | * Help people learn how big branches can be split up (BjornT) | 15:18 |
| barry | BjornT: i know we talked about this a few weeks ago, but i don't think we came up with anything specific. any new thoughts on this? | 15:18 |
| barry | guess not :) | 15:19 |
| BjornT | no, no new thoughts from me. my proposal is still to require people submitting too large branches, to go through them with a reviewer, to see how it could have been split up. | 15:19 |
| schwuk | Pre-implementation calls are the killer here - case-in-point abel's current branch which a) didn't have one and b) is huge. | 15:19 |
| bigjools | schwuk: my thoughts also | 15:19 |
| schwuk | It also doesn't lend itself well to being chunked for review. | 15:19 |
| bigjools | is he even aware of the size restriction? | 15:19 |
| BjornT | except that pre-implementation calls don't always happen... | 15:20 |
| bac | schwuk: but abel did split his work up. his problem was submitting all of the parts for parallel review. | 15:20 |
| bac | schwuk: albeit he split them after the fact, to your point | 15:20 |
| schwuk | bac: yes | 15:20 |
| schwuk | and I'm -1 on that sort of splitting as context can be broken | 15:20 |
| schwuk | e.g. he had comments referring to a function that wasn't implemented in the branch I was reviewing. | 15:21 |
| gmb | schwuk: What if one reviewer reviewed the chunks in series? | 15:21 |
| bac | schwuk: ok, that's nasty | 15:21 |
| bigjools | oy | 15:21 |
| gmb | schwuk: So the big branch is split into chunks a, b and c, which you agree ahead of time to review. | 15:22 |
| schwuk | However in his defence, it's very difficult to fit a whole new feature into 800 lines, but he should have had a pre-imp call to chunk the work more. | 15:22 |
| * BjornT thinks it would be best to limit this discussion to how we can get people to try to split up the work, not how to actually do the splitting | 15:22 | |
| sinzui | Bjorn +1 | 15:22 |
| bigjools | insist on pre-imps | 15:23 |
| BjornT | i also don't think pre-imp calls is the answer here. they are good, but we've tried to require people to have pre-imp calls before, and it didn't work. | 15:23 |
| schwuk | gmb: no agreement was made (by me). From my perspective he made the branch, chunked it and submitted it concurrently | 15:23 |
| BjornT | bigjools: how? | 15:23 |
| barry | BjornT: it still doesn't work. pre-imps are rare ime | 15:23 |
| bigjools | BjornT: well some time ago I thought they were made mandatory by Steve | 15:23 |
| gmb | bigjools: I don't see how we can enforce them. | 15:23 |
| BjornT | bigjools: yes. and that didn't work. | 15:23 |
| bigjools | jeez | 15:24 |
| gmb | I nearly *always* forget to have a pre imp (though I often have mid-imp sanity checks). | 15:24 |
| BjornT | bigjools: that's why "people should have a pre-imp call" isn't a good answer. | 15:24 |
| sinzui | I think a small plan is required before coding can start. Another developer must agree to the plan | 15:24 |
| bigjools | so are we to wipe everyone's bottoms for them too? | 15:24 |
| flacoste | BjornT: i think it's our responsibility as team leads to ensure that people do pre-impl | 15:24 |
| BjornT | if we can find a way to really get people to have pre-imp calls, than that's fine. | 15:24 |
| gmb | sinzui: But again that's an enforcement issue, isn't it? | 15:24 |
| flacoste | we can check that up in the daily standup | 15:24 |
| flacoste | did you have your pre-impl call? | 15:24 |
| BjornT | flacoste: sure, that's a good point. | 15:25 |
| BjornT | i still think that it's really useful to go through a real diff, though. | 15:25 |
| flacoste | so if somebody didn't have a pre-implc call, blame the team lead | 15:25 |
| sinzui | gmb: If part of the cover letter is written during the plan, it might be doable. That is to say. the pre-imp has a deliverable that we can track | 15:25 |
| flacoste | sinzui does this | 15:25 |
| flacoste | and it works well | 15:25 |
| BjornT | it's easier to learn going through a diff, than to just talk about the problem. | 15:26 |
| gmb | sinzui: But you've still got the "whoops, I forgot, sorry, review-it-anyway please, kthxbye" factor. | 15:26 |
| sinzui | gmb: That is true, there is also the critical branch that even I skip the implementation plan | 15:26 |
| gmb | Right. | 15:26 |
| barry | i also think reviewers need more latitude to kick a branch (or several branches) out of the review queue if they are not organized well and/or didn't have a preimpl | 15:26 |
| sinzui | gmb: We want best practices, not dogma | 15:27 |
| flacoste | agreed | 15:27 |
| gmb | Also true. | 15:27 |
| sinzui | We need better planning, but we don't need to require planning to the point of unprodictivity | 15:27 |
| barry | sinzui: true | 15:28 |
| bigjools | I think this should be brought up at the team meeting | 15:28 |
| bac | barry: you mean remove not just reject? i did that yesterday but didn't feel like there was much precedent. | 15:28 |
| schwuk | barry: You're right. There's (unseen) pressure to review a branch even if it exceeds the line limit or hasn't been pre-imp'd. | 15:28 |
| bigjools | then it will have more weight | 15:28 |
| schwuk | Maybe we do need to kick more branches out. | 15:28 |
| barry | bac: yes | 15:28 |
| bac | barry: +1 | 15:28 |
| barry | reject if you can't get to it, but remove (with an email) if there are fundamental problems with the branch | 15:29 |
| barry | if there are questions, contact me and we can decide together | 15:29 |
| bac | perhaps, for a bit, we can set all branches with no pre-imp to 'needs-reply' where the developer has to explain why they didnt' do a pre-imp. it might then sink in. | 15:30 |
| barry | our time as reviewers is limited and valuable and we should not be spending time on branches that really weren't ready to code | 15:30 |
| flacoste | bac: i think that would only introduce more dealys | 15:31 |
| flacoste | in the process | 15:31 |
| barry | since that reduces our productivity and thus the productivity of the entire team | 15:31 |
| schwuk | How about peer review? If a pre-imp call doesn't happen (for whatever reason) then the code should be reviewed by a peer (i.e. same team) before submitted for permission-to-merge. | 15:31 |
| flacoste | lack of pre-impl is only a problem if it is a problem | 15:31 |
| flacoste | if the code is easy to review | 15:31 |
| bac | flacoste: yes it would. but you're trying to change culture and that may require some temporary pain | 15:31 |
| flacoste | who cares that it didn't have a pre-imp | 15:31 |
| BjornT | bac: i don't think that will help. people already have to "explain" why the branch is large. and the answer is usually, "sorry, it couldn't be split up". | 15:31 |
| bigjools | schwuk: particularly with junior/new team members | 15:31 |
| bac | BjornT: :) ok, so you got me there | 15:32 |
| barry | flacoste: i agree, plus a "pre-impl" call can be fairly liberal for easy branches or fairly indepth for complicated branches | 15:32 |
| BjornT | a pre-imp call can also slow you down sometimes. that's why i don't what to require it. | 15:32 |
| barry | maybe we need a buddy system for new devs or devs having persistent problems with this? | 15:32 |
| flacoste | that's a nice idea | 15:32 |
| sinzui | buddy-buddy live? | 15:33 |
| barry | e.g. i'm buddying abentley when thumper isn't around. we don't talk often, but he knows he can always ping me with questions | 15:33 |
| barry | flacoste: do you think buddying by default with your team lead is a good idea, or should it be a non-superior peer? | 15:34 |
| flacoste | barry: i don't know, i think team leads are fine, but i may be biased here :-) | 15:34 |
| barry | :) | 15:35 |
| bigjools | In the Soyuz world, we've buddied Muharem and it's worked well | 15:35 |
| barry | bigjools: cool. can you provide some details? e.g. was he buddied by one person, the whole team? | 15:36 |
| bigjools | well there's only three of us, so Celso and I took it in turns | 15:36 |
| bigjools | because Soyuz is so complicated it's kinda necessary anyway, we still check his landings to make sure nothing awry is going on | 15:36 |
| schwuk | BjornT: I don't think pre-imp calls should be mandatory for the same reasons as you, but I do think that a large branch without a pre-imp call should be flagged. | 15:37 |
| bigjools | schwuk: instadeath to large branches with no pre-imp | 15:37 |
| BjornT | yes, so let's back up a bit | 15:37 |
| barry | bigjools: +1 to that | 15:37 |
| BjornT | let's say that someone submits a branch that is too large | 15:38 |
| BjornT | what should happen? | 15:38 |
| bigjools | firstly, reject. secondly, get someone to explain how it could be split | 15:38 |
| sinzui | I think sending a summary of what you intend to do for sign off will suffice in many cases. If there are concerns, a call is needed. | 15:38 |
| barry | reject. send email to dev and their team lead | 15:39 |
| flacoste | yeah, that's good | 15:39 |
| BjornT | sinzui: what do you mean by "what you intend to do"? | 15:39 |
| flacoste | i think he's talking about the cover letter | 15:39 |
| flacoste | as a pre-impl artefact | 15:39 |
| BjornT | right. that's what happens today, isn't it? | 15:40 |
| flacoste | reading the email, you can tell, yes that's fine, no call needed | 15:40 |
| flacoste | BjornT: only in sinzui's case | 15:40 |
| sinzui | This is what I started yesterday for a plan:https://pastebin.canonical.com/5582/ | 15:40 |
| flacoste | he's talking about writing the cover letter before doing the implementation | 15:40 |
| flacoste | and have that letter be sanity-checked by someone else as part of the pre-impl phase | 15:41 |
| sinzui | I need to know how to test and where to code change points are still. | 15:41 |
| BjornT | flacoste: ok. how do you do that, after you've submitted the branch for review? ;) | 15:41 |
| flacoste | right, that's a separate process :-) | 15:41 |
| BjornT | flacoste: yes. i'd like us to limit the scope of the discussion a bit :) | 15:42 |
| BjornT | 17:38 < BjornT> let's say that someone submits a branch that is too large | 15:42 |
| BjornT | 17:38 < BjornT> what should happen? | 15:42 |
| flacoste | i think bigjools and barry suggestions were very good | 15:42 |
| BjornT | +1 to bigjools, since that was basically my original proposal | 15:42 |
| schwuk | < barry> reject. send email to dev and their team lead | 15:42 |
| bigjools | yup | 15:43 |
| schwuk | BjornT: who is the someone though? | 15:43 |
| schwuk | The reviewer, or the team lead? | 15:43 |
| bigjools | team lead I think | 15:43 |
| bigjools | reviewer is already busy | 15:43 |
| schwuk | bigjools: that was my thought as well | 15:43 |
| flacoste | ? | 15:44 |
| flacoste | someone is the developer in that sentence | 15:44 |
| barry | flacoste: that's what i thought too | 15:44 |
| flacoste | a developer submits a branch that is too large | 15:44 |
| BjornT | well, all team leads are reviewers, so there should be a greater chance of finding a reviewer that is free | 15:44 |
| flacoste | action: reviewer rejects the review emailing the dev and his team-leads | 15:44 |
| bigjools | flacoste, barry: we're talking about who to ask to discuss the large branch with the dev | 15:44 |
| flacoste | ok, i get it | 15:44 |
| schwuk | too many someones... | 15:45 |
| flacoste | yeah, i'm sure the team lead can help the dev or find somebody to help him out | 15:45 |
| BjornT | i think it doesn't matter who you discuss the branch with, as long as an e-mail is sent to the list, with the result. then other people can chime in as well. | 15:45 |
| bigjools | in fact, yes, anyone else in the team, not just the lead | 15:45 |
| flacoste | that's unless the dev happened to fall on a friendly reviewer like sinzui who's always willing to help you split your branches :-) | 15:45 |
| sinzui | I suck as saying No | 15:45 |
| barry | right :) | 15:46 |
| barry | oops, we're out of time and have one more item | 15:46 |
| barry | apoligies for going over | 15:46 |
| barry | i like this idea, and we should flesh it out on the ml | 15:46 |
| * barry will start the discussion going | 15:46 | |
| barry | * (Julian) Since I seem to be finding it hard to get my Soyuz comrades to follow our own informal coding standards, when reviewing Soyuz code please make sure you don't let code of the form: ` if archive.purpose == ArchivePurpose.PPA:` land, instead it should be the simpler: `if archive.is_ppa:` which not only encapsulates the decision in IArchive, it should remove an import of the DBEnum. | 15:46 |
| bigjools | sorry for the long item :) | 15:47 |
| bigjools | but it says it all | 15:47 |
| barry | bigjools: +1 | 15:47 |
| bigjools | kthxbye | 15:47 |
| barry | that's it from me. anything else for today? | 15:47 |
| schwuk | So do we need domain specific cheat sheets for reviewers? | 15:47 |
| bigjools | thanks in advance to the reviewers if you can police this | 15:47 |
| barry | schwuk: that's not a bad idea | 15:47 |
| bigjools | +1 | 15:47 |
| barry | links off of TipsForReviewers i think | 15:48 |
| bigjools | mwh once started an informal SoyuzReviewerCheatSheet or somesuch | 15:48 |
| barry | bigjools: wanna get that started for soyuz? | 15:48 |
| bigjools | sure thing | 15:48 |
| barry | great, thanks | 15:48 |
| barry | okay, that's it then. sorry for going long | 15:48 |
| barry | #endmeeting | 15:48 |
| barry | thanks everyone | 15:48 |
| schwuk | Thanks barry | 15:48 |
| bigjools | thank you | 15:49 |
| === mwhudson_ is now known as mwhudson | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!