| === mrevell-lunch is now known as mrevell | ||
| === salgado is now known as salgado-afk | ||
| === salgado-afk is now known as salgado-lunch | ||
| bac | #startmeeting | 15:00 |
|---|---|---|
| bac | hi -- welcome to the AMEU reviewer's meeting. who is here? | 15:00 |
| flacoste | me | 15:00 |
| gary_poster | me | 15:00 |
| bac | where is mootbot? | 15:00 |
| bac | sinzui, EdwinGrubbs, mars, salgado-lunch, bigjools, allenap`: ping | 15:02 |
| sinzui | me | 15:02 |
| bac | BjornT: ping | 15:02 |
| bigjools | me | 15:02 |
| EdwinGrubbs | me | 15:02 |
| allenap` | me | 15:02 |
| bac | danilos ping | 15:02 |
| * allenap` is sprinting too | 15:03 | |
| henninge | me | 15:03 |
| mars | me | 15:03 |
| bac | allenap`: your cohorts joining us? | 15:03 |
| allenap` | bac: Apologies from all. | 15:04 |
| bac | [TOPIC] agenda | 15:04 |
| flacoste | bac: BjornT is sleeping | 15:04 |
| bac | * Roll call | 15:04 |
| bac | * Action items | 15:04 |
| bac | * Verifying that community contributions don't do harm (henninge) | 15:04 |
| bac | * Peanut gallery (anything not on the agenda) | 15:04 |
| mars | bac, "other teams to review lazr-js branches" from last week? | 15:05 |
| bac | [topic] action items | 15:05 |
| bac | mars: i'm getting there... | 15:05 |
| bac | * Maris to bring up cross-team reviews in the lazr-js task force meeting and report back. | 15:05 |
| danilos | me | 15:05 |
| bac | mars did you have that discussion? | 15:05 |
| danilos | :) | 15:05 |
| mars | bac, done! Zac and Sidnei both would like to start doing lazr-js branch reviews. | 15:06 |
| bac | mars: great. | 15:07 |
| bac | mars: how will we facilitate that/ | 15:07 |
| mars | so, if you have a lazr-js patch, you can ping them on IRC for a review | 15:08 |
| mars | as well as the ususal suspects | 15:08 |
| bac | mars: what is zac's irc nick? | 15:08 |
| flacoste | mars: do we have up-to-date review guidelines for lazr-js? | 15:08 |
| flacoste | bac: urbanape | 15:08 |
| mars | bac, Zac is urbanape, sidnei is sidnei | 15:08 |
| mars | flacoste, we are still using the JS guidelines on dev.lp.net | 15:09 |
| bac | ok, thanks for following up maris | 15:09 |
| bac | * Gavin to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc | 15:10 |
| allenap` | bac: I haven't done that yet; please keep that action. | 15:10 |
| bac | allenap`: ok, we'll roll it over | 15:10 |
| bac | allenap`: can you try to do that early next week since you're sprinting? | 15:10 |
| allenap` | bac: Yes, absolutely. | 15:11 |
| bac | great | 15:11 |
| bac | * Gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. | 15:11 |
| * gary_poster did not do the timeit tests yet. Next week. | 15:11 | |
| bac | gary_poster: any progress? | 15:11 |
| bac | ok | 15:11 |
| bac | * Curtis to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. | 15:11 |
| bac | sinzui: you did get that branch landed, correct? | 15:11 |
| sinzui | done | 15:11 |
| bac | great | 15:11 |
| bac | while OCR yesterday we discovered one outstanding but it should be fixed when branches land today or tomorrow | 15:12 |
| bac | we have one new item today from henninge | 15:13 |
| bac | * Verifying that community contributions don't do harm (henninge) | 15:13 |
| mars | ? | 15:13 |
| henninge | yes | 15:13 |
| bac | henninge: the floor is yours | 15:13 |
| henninge | IThis came up after a review I did for a community developer on Monday. | 15:13 |
| henninge | Heeding danilos reminder mail I checked that he had had a pre-imp discussion with an LP developer. | 15:13 |
| henninge | It turned out though, that the team of the affected application (soyuz) did not agree with the change, at least not the way it was done. | 15:13 |
| henninge | So, one lesson to learn would be to make sure the pre-imp was with some-one from the right team that would have domain knowledge. | 15:13 |
| henninge | But I am wondering if we should also rquire a second "sanity-check" review from a developer of the affected team. | 15:14 |
| henninge | Not a second code review. | 15:14 |
| henninge | To me, that would be the clearest sign that all agree. | 15:14 |
| bac | henninge: in the past, before we had community contributions, we tried to do the opposite -- pre-imps across the team. but for community fixes perhaps it is a good idea. | 15:15 |
| bigjools | I think that in the soyuz case it requires a lot more domain knowledge | 15:15 |
| flacoste | i'm not sure pre-imp across the team is a sane choice anyway | 15:15 |
| bigjools | and I would encourage a pre-imp with a soyuz dev even if the dev was an LP team member | 15:15 |
| flacoste | pre-impl is where you want the more domain knowledge | 15:15 |
| flacoste | so it makes most sense to do within team | 15:16 |
| flacoste | i agree with bigjools with the extension to every domain | 15:16 |
| bac | while we always encourage doing pre-imp calls do we agree they should be mandatory for community contributions? | 15:17 |
| bigjools | yes | 15:17 |
| henninge | +1 | 15:17 |
| bac | +1 | 15:17 |
| bigjools | unless we give someone an exception | 15:17 |
| bigjools | I can think of one person who knows quite a lot :) | 15:17 |
| barry | as long as exceptions are only given in a pre-imp call <wink> | 15:17 |
| flacoste | agreed | 15:18 |
| bac | [agreed] community contributions require a pre-imp call | 15:18 |
| henninge | a pre-imp call with a domain dev | 15:19 |
| bac | henninge had the further proposal of a second domain-specific review. thoughts? | 15:19 |
| henninge | there is still the chance that what was discussed in the pre-imp does not end up in the actual code. | 15:19 |
| bigjools | I think that the first pre-imp should be with the domain specialist | 15:19 |
| henninge | not out of malice, just misunderstandings etc. | 15:20 |
| bigjools | we don't need 2 pre-imps | 15:20 |
| danilos | a late +1 for me :) | 15:20 |
| henninge | bigjools: not 2 pre-imps | 15:20 |
| bac | bigjools: yes, that was implied. we aren't talking about two pre-imps | 15:20 |
| bigjools | ok | 15:20 |
| henninge | we are talking about that each mp from a community developer should have been looked at by a domain dev. | 15:20 |
| barry | henninge: +1 | 15:21 |
| henninge | not for a full code review but just to make sure it's sane. | 15:21 |
| danilos | henninge, my concern there would be just that reviewers check that nothing was implemented which is not agreed upon (i.e. permission changes on certain objects, which is what I've seen) | 15:21 |
| henninge | +1 | 15:21 |
| danilos | at this time, I think that's sane, but as we get more contributions, it will become unwieldy | 15:22 |
| danilos | so +1/+0 from me (+1 now, +0 for the future :) | 15:22 |
| bac | danilos: how does the reviewer know what has been agreed upon? | 15:22 |
| sinzui | I have already had one branch to stole a day of my life | 15:22 |
| danilos | bac, i.e. reading a bug report | 15:22 |
| bac | danilos: that's great if the bug report is specific enough | 15:22 |
| flacoste | i think we could use some discretion | 15:23 |
| henninge | Maybe it's a case-to-case decission. | 15:23 |
| flacoste | the reviewer could ask the pre-impl person to have a look | 15:23 |
| danilos | bac, at least that was the case with one example where I've seen it fail; bug report was very specific about what fields should be exposed, and branch exposed others as well | 15:23 |
| barry | if the domain specialist does the pre-imp they should do the sanity check review, and it can be up to them how thorough it is | 15:23 |
| flacoste | if he sees it as a tricky change | 15:23 |
| danilos | flacoste, this was a very simple change, for example | 15:23 |
| bac | danilos: ok, that's cause for concern | 15:23 |
| danilos | barry, yeah, agreed | 15:23 |
| flacoste | well | 15:23 |
| flacoste | to be honest | 15:23 |
| flacoste | i think the mistake that happened there could have been done by any lp dev | 15:24 |
| flacoste | not familiar with the domain | 15:24 |
| flacoste | so it's not a big change | 15:24 |
| bigjools | my point also | 15:24 |
| danilos | bac, though, I believe pre-imp would have caught this, so I don't think it's cause for another review, or a problem with the review | 15:24 |
| henninge | flacoste: but we usually just code within our domain. | 15:24 |
| flacoste | more or less | 15:24 |
| flacoste | usually | 15:24 |
| flacoste | but not exclusively | 15:24 |
| flacoste | and that's fine | 15:24 |
| henninge | I know | 15:24 |
| flacoste | and we want more of it | 15:24 |
| henninge | I like it, too ;) | 15:24 |
| bac | to me it seems the guiding principle should be that reviewers should always be open to asking for domain specific help. | 15:25 |
| flacoste | i think a pre-impl with someone with domain expertise is always mandatory | 15:25 |
| danilos | yeah, anyway, let's not introduce too many barriers; let's assume it's common wisdom that you should suggest a domain-specific reviewer if code looks too hard | 15:25 |
| bigjools | more cross-domain experience would have helped the pre-imp ;) | 15:25 |
| danilos | flacoste, +1 | 15:25 |
| danilos | flacoste, though, we agreed on that already :) | 15:25 |
| henninge | bac: +1 | 15:25 |
| flacoste | yeah | 15:25 |
| danilos | bac, +1 | 15:25 |
| flacoste | and i like bac formulation | 15:25 |
| danilos | (I tried to say that, but you put it more nicely :) | 15:26 |
| bac | ok, so we have no real action here except to be sensitive to getting domain specific eyes when necessary | 15:26 |
| danilos | yeah | 15:26 |
| bac | moving on... | 15:26 |
| flacoste | well, that and enforcing pre-impl | 15:26 |
| flacoste | on community contrib | 15:26 |
| bac | flacoste: well that was done above | 15:26 |
| bac | * Peanut gallery (anything not on the agenda) | 15:26 |
| bac | anyone? | 15:27 |
| henninge | bac, flacoste: I'll update the wiki ipage | 15:27 |
| bac | henninge: thanks | 15:27 |
| henninge | iPhone, iGoogle, iPage ;-) | 15:27 |
| bac | [action] henning to update wiki page regarding pre-imps for community contributions | 15:27 |
| bac | ok, well if there's nothing else we can end the meeting. | 15:28 |
| bac | thanks everyone for coming, except you mootbot | 15:28 |
| bac | #endmeeting | 15:28 |
| mars | :) | 15:28 |
| mars | thanks bac | 15:28 |
| henninge | thanks bac | 15:28 |
| gary_poster | thanks bac | 15:28 |
| danilos | thanks bac :) | 15:30 |
| === salgado-lunch is now known as salgado | ||
| salgado | crap, am I 1h late? | 15:43 |
| salgado | is this meeting not 1545TUC? | 15:43 |
| henninge | salgado: which? reviewer's? | 15:43 |
| salgado | yeah | 15:43 |
| salgado | oh, it's from 1500 to 1545 | 15:45 |
| salgado | damn | 15:45 |
| henninge | salgado: sorry, mate ;-) | 15:45 |
| salgado | bac, although I'm late I had an item in the agenda. did you not see it or just skipped because I wasn't here? | 15:48 |
| === bigjools is now known as bigjools-otp | ||
| bac | salgado: i didn't see it at https://dev.launchpad.net/ReviewerMeetingAgenda | 16:09 |
| bac | salgado: perhaps it got removed in an edit. sorry. can you put it back for next week? | 16:10 |
| salgado | sure, I'll do that | 16:10 |
| === bigjools-otp is now known as bigjools | ||
| === salgado is now known as salgado-afk | ||
| === mwhudson_ is now known as mwhudson | ||
| === mwhudson_ is now known as mwhudson | ||
| === Chex_ is now known as Chex | ||
| === mwhudson_ is now known as mwhudson | ||
| === EdwinGrubbs is now known as Edwin-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!