| === mwhudson_ is now known as mwhudson | ||
| === mwhudson__ is now known as mwhudson | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === EdwinGrub is now known as EdwinGrubbs | ||
| === vednis is now known as mars | ||
| barry | #startmeeting | 15:00 |
|---|---|---|
| * barry looks to the ceiling and cries "moootbooottttt" | 15:00 | |
| barry | hello everyone and welcome to this week's ameu reviewer's meeting. who's here today? | 15:01 |
| gmb | me | 15:01 |
| statik | me | 15:01 |
| sinzui | him? | 15:01 |
| EdwinGrubbs | me | 15:01 |
| sinzui | me | 15:01 |
| bac | me | 15:01 |
| BjornT | me | 15:01 |
| intellectronica | me | 15:01 |
| allenap | me | 15:01 |
| flacoste | me | 15:01 |
| schwuk | me | 15:01 |
| salgado | me | 15:01 |
| barry | bigjools: ping | 15:02 |
| bigjools | me | 15:02 |
| barry | danilos: ping | 15:02 |
| danilos | me | 15:03 |
| danilos | barry: thanks | 15:03 |
| bigjools | sorry, was deep in a page test | 15:03 |
| barry | :) | 15:03 |
| barry | i think that's everyone | 15:03 |
| 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:04 |
| barry | * Review process | 15:04 |
| barry | * Discourage use of 'hasattr' (bac) | 15:04 |
| barry | * Use of 'assert' in doctests (BjornT) | 15:04 |
| barry | * Help people learn how big branches can be split up (BjornT) | 15:04 |
| barry | * Final vote multiline import | 15:04 |
| barry | * Next meeting | 15:04 |
| barry | same time and place + weeks(1) ? | 15:04 |
| barry | anybody know they will be missing or sprinting? | 15:04 |
| intellectronica | the bugies will be sprinting | 15:04 |
| gmb | barry: BjornT, intellectronica, bigjools, cprov, allenap and I will be at UDS. | 15:04 |
| intellectronica | but maybe we can still make it | 15:04 |
| barry | gmb, intellectronica cool | 15:05 |
| bigjools | we might make it, depends if a BoF gets in the way or not | 15:05 |
| * danilos makes a note of 'bugies' | 15:05 | |
| sinzui | I'm sprinting and am here | 15:05 |
| barry | * Action items | 15:05 |
| BjornT | sinzui: you don't have a bunch of external people to adjust to, though | 15:05 |
| * gmb thought we'd established it was Bjötches... | 15:05 | |
| barry | * barry drive to decision about multiline sequences | 15:06 |
| barry | BjornT: you should put the irc session up on a big screen and let everyone watch | 15:06 |
| statik | that was fun in the florida sprint | 15:06 |
| barry | anyway, we're very close to closing this one out. see upcoming vote later today | 15:06 |
| * barry can't wait to put that one to bed | 15:07 | |
| barry | * barry to solicit ideas to better handle review scheduling and workload | 15:07 |
| barry | nothing really happening there | 15:07 |
| barry | do people still seem overwhelmed by review workload? | 15:07 |
| bigjools | depends on how much real work I have | 15:08 |
| bigjools | which is always a lot :) | 15:08 |
| barry | bigjools: what? reviewing isn't real work? :) | 15:08 |
| bigjools | barry: hell no, don't you sit with your feet up all day until you get pinged for a review? ;) | 15:08 |
| intellectronica | barry: sharing with allenap seems to make it much better for me | 15:09 |
| barry | bigjools: sure i do, but it's such a pain to have to go refilling that gin&tonic all the time while i do it | 15:09 |
| bac | yesterday was very light -- probably b/c danilo did all of the EU reviews before i started. | 15:10 |
| sinzui | I assume I only write code 4 days of the week. I assume I can only review 2 hours if I am mentoring. That brought me back to an equilibrium of time | 15:10 |
| barry | intellectronica: great! hopefully we'll have lots of good pairing as we get closer to our goal of everyone is a reviewer | 15:10 |
| barry | sinzui: yep | 15:11 |
| barry | anyway... | 15:11 |
| barry | * let's remind people in review replies to clean up their PR branches | 15:11 |
| barry | i sent out an email today on this... (thanks gmb! :) | 15:11 |
| danilos | that will mean we'll also need to change that suggestion of "what do we do while we wait for review requests to pop up while on call" | 15:11 |
| gmb | :) | 15:11 |
| barry | danilos: can you elaborate? | 15:11 |
| danilos | barry: well, we have so far considered that it's best not to work on anything "hard" while you are on-call; if we get two reviewers on-call at the same time, review workload would be much smaller | 15:12 |
| danilos | so, should this change when we get more reviewers or not | 15:13 |
| barry | danilos: ah, i see. for the past few weeks i keep thinking i might get something else done while waiting for reviews, but my dance card always seems to fill up | 15:13 |
| barry | danilos: we'll have to see | 15:13 |
| barry | * bac to add a link to his PR notification script from TipsForReviewers | 15:14 |
| bac | done | 15:14 |
| danilos | barry: yeah, it happened to me yesterday as well, but the way day started I definitely could have gotten more work in at the start of day (empty general queue, nobody wanting anything :)) | 15:14 |
| barry | bac: thanks! | 15:14 |
| barry | danilos: waiting for all those crazy western hemispheroids to wake up? :) | 15:14 |
| barry | * flacoste to add `safe_hasattr()` to `lazr.utils` | 15:14 |
| danilos | barry: not really, I guess waiting for those EU guys to actually write some code in the morning :)) | 15:15 |
| flacoste | barry: hmm, that was changed to file a bug, no? | 15:15 |
| flacoste | barry: which i think i did... at least i filed a bug related to a reviewer action item | 15:15 |
| barry | flacoste: i might have missed that. if you filed the bug, thanks! can you double check so i can remove this action item? | 15:16 |
| flacoste | i guess i need an action item: "flacoste to get his act together" | 15:16 |
| barry | * bac to write up a warning on `hasattr()` in PythonStyleGuide | 15:16 |
| bac | done | 15:16 |
| barry | bac: awesome | 15:16 |
| bac | also added to reviewer checklist | 15:16 |
| barry | * gmb to add lpreview to sourcecode and hack rf-setup to link it in | 15:17 |
| barry | bac: thanks! | 15:17 |
| barry | gmb: done, right? we can finally remove this one? | 15:17 |
| gmb | barry: It's now in SC and the rf-setup hack needs to be landed after I've made the changes that you and bigjools suggested. But it'll land today, so feel free to remove it. | 15:17 |
| barry | gmb: great! we are on a roll :) | 15:18 |
| barry | * gmb to prod mwh again about the 800-line limit patch | 15:18 |
| gmb | Also done. | 15:18 |
| gmb | He has one suggestion. | 15:18 |
| gmb | Basically, my fix was to only allow a > 800 line branch if you'd agreed it with a review (and so specified one on the command line) | 15:18 |
| gmb | However, mwh felt that was - in his words- over the top | 15:19 |
| gmb | So he suggested just having a --force flag. | 15:19 |
| gmb | I'll make the changes today unless there are any objections to that. | 15:19 |
| intellectronica | gmb: how about doing what we do for lint? | 15:19 |
| gmb | intellectronica: ? | 15:19 |
| bigjools | a warning | 15:19 |
| bac | i like a force flag. people often specify a reviewer but might not know the branch is too big. | 15:19 |
| intellectronica | gmb: so that if it's longer you're forced to explain why in words? | 15:20 |
| barry | intellectronica: +1 | 15:20 |
| gmb | Sure, that can be done. | 15:20 |
| intellectronica | you know [This diff is too long, if you still want to send it remove this line and explain why it's OK] | 15:20 |
| * barry likes these kinds of "subtle" social reminders :) | 15:21 | |
| barry | gmb: thanks for doing this. i'm going to remove the action item | 15:21 |
| barry | * sinzui to update js style guide page with helpful resources | 15:21 |
| sinzui | Done. | 15:21 |
| barry | sinzui: thanks! | 15:22 |
| barry | * sinzui or flacoste to add sampledata check to lpreview or make lint | 15:22 |
| flacoste | ah! | 15:22 |
| flacoste | that was the bug i filed! | 15:22 |
| barry | flacoste: cool! i'll take this one off and leave the other on for now | 15:22 |
| flacoste | bug 227779 | 15:23 |
| ubottu | flacoste: Bug 227779 on http://launchpad.net/bugs/227779 is private | 15:23 |
| barry | wow, i'm impressed at how many action items we crushed this week. thanks everyone! | 15:23 |
| flacoste | Add a test for out-of-date sampledata | 15:23 |
| barry | flacoste: great, thanks | 15:23 |
| barry | * Queue status | 15:24 |
| barry | nothing from me here. things look generally good to me. any comments? | 15:24 |
| sinzui | Do we have a plan for Carlo's many branches? | 15:25 |
| barry | you mean his many wips? | 15:25 |
| sinzui | Yes | 15:26 |
| barry | i think his team is going to have to handle that. i don't know what we as reviewers can do | 15:27 |
| barry | * Mentoring update | 15:28 |
| barry | any feedback from mentors or mentorees? | 15:28 |
| * bigjools would like the chance to do a translations review | 15:28 | |
| bigjools | and code | 15:29 |
| danilos | bigjools: not had a chance to do that yet? | 15:29 |
| sinzui | I think the Europeans should take advantage of schwuk when he is available. | 15:29 |
| bigjools | nope | 15:29 |
| intellectronica | bigjools: it's quite rare that anyone here gets to do -code reviews | 15:29 |
| danilos | bigjools: we can make something happen in the next few days, I'll come directly to you :) | 15:29 |
| bigjools | intellectronica: I gathered, they're a bit cliquey down there :) | 15:29 |
| schwuk | sinzui: Very little has come through on-call the last couple of weeks. | 15:29 |
| schwuk | Maybe everyone is waiting for week 3... :) | 15:30 |
| sinzui | schwuk: Indeed. | 15:30 |
| danilos | well, everybody now has a reviewer on their own team, and that cuts on the explaining, meaning people tend to use that more | 15:30 |
| bigjools | danilos: thanks, make it a big nasty one ok? :) | 15:30 |
| danilos | bigjools: don't worry, just the kind I've got in store for you :) | 15:30 |
| bigjools | /o\ | 15:30 |
| intellectronica | bigjools: you think that will redeem you for all those incomprehensible soyuz branches you always give us? ;) | 15:31 |
| * bigjools chuckles | 15:31 | |
| barry | :) | 15:31 |
| bigjools | soyuz is like a pubic hair on a toilet seat | 15:32 |
| bigjools | you're gonna get pissed off sooner or later | 15:32 |
| intellectronica | but less sexy | 15:32 |
| gmb | That's what I like about this team, the highbrow humour... | 15:32 |
| barry | bigjools: as my dad always says: it's better to be pissed off than pissed on | 15:33 |
| bigjools | your dad sounds wise | 15:33 |
| sinzui | I think that remark was worthy of mneptok | 15:33 |
| barry | :) | 15:33 |
| barry | * Mentoring update | 15:33 |
| barry | oops | 15:34 |
| intellectronica | mneptoring update? | 15:34 |
| barry | stoopid keyboard | 15:34 |
| barry | * Review process | 15:34 |
| barry | let's do the fun one first | 15:34 |
| barry | * Final vote multiline import | 15:34 |
| * bigjools hides | 15:34 | |
| barry | all in favor of switching to one-line-per-import say "aye". all opposed, say "nay" | 15:34 |
| flacoste | nay | 15:35 |
| bigjools | I just had a conflict in the imports at the top of security.py - how's that for coincidence | 15:35 |
| intellectronica | nay | 15:35 |
| gmb | nay | 15:35 |
| salgado | nay | 15:35 |
| BjornT | nay | 15:35 |
| barry | nay | 15:35 |
| sinzui | nay | 15:35 |
| danilos | nay | 15:35 |
| bigjools | abstain | 15:35 |
| EdwinGrubbs | aye | 15:35 |
| schwuk | aye | 15:35 |
| allenap | aye | 15:36 |
| bac | aye | 15:36 |
| barry | wow, interesting | 15:37 |
| flacoste | mars has another style suggestion to minimize conflicts | 15:37 |
| flacoste | he'll send it off to the list | 15:37 |
| bigjools | tbh I would prefer bzr to resolve these automatically | 15:37 |
| barry | given where i think asiapac is on this, i think we're at a standoff | 15:37 |
| flacoste | but instead of importing from interfaces which is the hot-spot, we import from interfaces.person | 15:37 |
| flacoste | or interfaces.bugs | 15:37 |
| barry | flacoste: maybe that means we can finally get rid of the import-*'s in package __Inits__? | 15:38 |
| danilos | flacoste: could we then also stop updating interfaces/__init__.py | 15:38 |
| danilos | barry: :) | 15:38 |
| flacoste | right | 15:38 |
| flacoste | praise goes to mars | 15:38 |
| barry | danilos: <wink> | 15:38 |
| flacoste | you can send complaints to me | 15:38 |
| danilos | flacoste: about anything, or just about this? ;) | 15:39 |
| BjornT | flacoste: you mean creating new files, or importing from the file where the interface is defined? | 15:39 |
| flacoste | the latter | 15:39 |
| barry | flacoste: great. we'll see how that's received on the list | 15:39 |
| flacoste | i mean, these conflicts always happen when importing from interfaces | 15:39 |
| bigjools | sounds reasonable | 15:39 |
| barry | we have 6 minutes left and a couple of items so let's move on and follow up to mars's post | 15:39 |
| BjornT | flacoste: right, i though so. it was just that interfaces/bugs.py doesn't exist. | 15:39 |
| flacoste | lol | 15:40 |
| barry | * Discourage use of 'hasattr' (bac) | 15:40 |
| bac | sorry i forgot to remove that from the agenda. nothing to see here. | 15:40 |
| barry | bac: cool | 15:40 |
| barry | * Use of 'assert' in doctests (BjornT) | 15:40 |
| BjornT | this is from the review of bac's fix to the broken assert statement in a doctest | 15:40 |
| BjornT | i commented on that we shouldn't use assert at all in doctests, since imo, all tests should be in the form of 1) do something 2) check the output | 15:41 |
| BjornT | bac or danilo said this wasn't mentioned in the tests style guide | 15:41 |
| barry | BjornT: +1 | 15:42 |
| sinzui | +1 | 15:42 |
| barry | BjornT: can you add an item to the test style guide covering this? | 15:42 |
| BjornT | barry: sure | 15:42 |
| intellectronica | as a last resort, you can always print out a boolean | 15:42 |
| barry | BjornT: thanks | 15:43 |
| danilos | bac wanted to have an explanation printed out in case of failure, as far as I could understand at least | 15:43 |
| bac | i'm +1 on the new policy. note there are almost 100 instances of assert in our pagetests | 15:43 |
| barry | bac: fertile ground for drive byes :) | 15:43 |
| BjornT | in doctests the explanation can go in the narrative, though. it's also possible using if statements. | 15:44 |
| danilos | BjornT: agreed | 15:44 |
| barry | we have one minute left, i apologize for going over | 15:44 |
| barry | * Help people learn how big branches can be split up (BjornT) | 15:44 |
| BjornT | so, we have a limit on how big a branch should be | 15:44 |
| BjornT | i it bigger, the coder has to negotiate with a reviewer to get it reviewed | 15:45 |
| BjornT | i don't think this works that well in practise. all that happens is that the reviewer says something like: "you can't make it any smaller? ok, then" | 15:45 |
| danilos | and then "now you'll see how it _can't_ be made any smaller" :) | 15:46 |
| BjornT | i think we need to be better at teaching people how to break up the work into smaller pieces | 15:46 |
| BjornT | this is something that people need to learn to think about before starting coding, rather than after the branch has gotten too big | 15:46 |
| * gmb has started using looms for just this purpose | 15:47 | |
| BjornT | my proposal is that if you have a branch that is too big, you have to spend time with something to go through it, discussing how the branch could have been split up | 15:47 |
| sinzui | BjornT: while I agree with you, that may best start with the pre-implementation call. | 15:47 |
| intellectronica | sinzui: right, but if it didn't? | 15:47 |
| bigjools | Are we ok with reviewing unlandable diffs? That way someone can have different parts reviewed and then merge them before landing. | 15:47 |
| * barry observes that pre-impls have been happening less and less | 15:47 | |
| BjornT | bigjools: yes. as long as the diff makes sense of its own. | 15:48 |
| danilos | I'd be +1 on actually going with a developer through the branch and seeing what could have been split up, -1 on reviewing unlandable diffs | 15:48 |
| bigjools | BjornT: sure, that goes without saying :) | 15:48 |
| intellectronica | +1, i benefited a lot from going through this kind of stuff, and think it's time very well spent for any developer | 15:48 |
| barry | -1 on unlandable diffs | 15:48 |
| danilos | well, in my definition "unlandable" == doesn't work (i.e. make check or make run fails) | 15:49 |
| bigjools | when I say unlandable, I mean things that make sense on their own but would fail tests | 15:49 |
| intellectronica | danilos: sometimes there's no choice, and it's still better to split the work | 15:49 |
| BjornT | bigjools: well, maybe not quit unlandable. i was thinking of diffs that could be landed, but shouldn't be. | 15:49 |
| bigjools | BjornT: yeah, so we need a definition of "shouldn't be" | 15:49 |
| * mars tends to create those when refactoring | 15:50 | |
| danilos | intellectronica, bigjools: I feel every new code should be accompanied by at least documentation that passes | 15:50 |
| barry | BjornT: the way to think about that is a loom thread i think. something that's part of a larger whole, but that stands on its own and is reviewable on its own, but maybe not landed until other threads are also approved | 15:50 |
| bigjools | danilos: I agree | 15:50 |
| intellectronica | danilos: right, so that's something to take into consideration when planning how to split the work! | 15:50 |
| BjornT | barry: yes, that's my thinking as well. | 15:50 |
| barry | the risk though is that those larger things will end up landing at the end of week three | 15:51 |
| bigjools | I am extremely wary of creating loads of small branches given PQM's run time | 15:51 |
| danilos | barry: doesn't everything land at the end of week three? (and mostly Sundays :)) | 15:51 |
| barry | danilos: :-D ;-} | 15:51 |
| gmb | bigjools: You don't have to land them all separately though. | 15:51 |
| danilos | one can merge branches before landing, using [r=one,two,three] | 15:51 |
| BjornT | barry: compared to what? we're talking about breaking up large branches. | 15:51 |
| bigjools | gmb: right, hence my "merge them" comment | 15:52 |
| barry | gmb: exactly! combine-thread | 15:52 |
| barry | then land | 15:52 |
| * gmb *actually reads* the scrollback. | 15:52 | |
| barry | BjornT: good point | 15:52 |
| danilos | the only thing to worry about is [log] messages for those actually processing logs, but since we are talking about related branches, this should only reduce the number of [!log] landings | 15:52 |
| barry | BjornT: so we need a plan to help devs learn how to use looms effectively | 15:52 |
| barry | danilos: hopefully | 15:53 |
| sinzui | bigjools: The developer can delay landing the branch until all the feature is assembled. | 15:53 |
| BjornT | barry: right. another thing to help them learn is how to actually split up the work itself. you don't really need looms to do it, it just makes the workflow easier. | 15:53 |
| bigjools | I quite often find my self doing that - landing separate fixes for a big bug fix | 15:53 |
| barry | we're way over, but i'd like to keep this one on the agenda for next week. in the meantime, can we think about this issue some more, specific ways to accomplish this? | 15:53 |
| bigjools | I don't think looms are necessary, you just need to plan your work | 15:54 |
| BjornT | it's the splitting up of the work that is the hardest part. not how you manage the branches. | 15:54 |
| bigjools | anyone with the intelligence to work here should be able to split the work, they just need to remember to do it | 15:54 |
| barry | bigjools: agreed that looms are just a tool to help you organize things, but you're right that its the organization that's key | 15:55 |
| BjornT | that's why i suggested forcing people with big branches learn how the branch could have been split up. they are the ones the probably need the help the most :) | 15:55 |
| barry | BjornT: thanks for bringing up an important topic. because we're 10 minutes over, i'd like to cut this short for now. apologies for that and for going so far over today | 15:56 |
| intellectronica | bigjools: right, so, if they don't do it, they can be asked to go and do it now, and if they say that they don't know how to they should be able to get help | 15:56 |
| bigjools | pre-imp calls would help.... | 15:56 |
| barry | agreed. | 15:56 |
| bigjools | intellectronica: yup - and remind them that a pre-imp call would have helped avoid having to break it up :) | 15:56 |
| barry | #endmeeting | 15:57 |
| barry | thanks everyone! | 15:57 |
| bigjools | thanks barry | 15:57 |
| intellectronica | thanks barry | 15:57 |
| BjornT | bigjools: unfortunately, reminders don't work that well :( | 15:57 |
| bigjools | BjornT: threats? :) | 15:57 |
| BjornT | bigjools: yes, that would be better :) | 15:57 |
| bigjools | BjornT: I agree though, people should get some help, the best way to do that is to plan the work via a pre-imp | 15:58 |
| danilos | basically, a requirement to go through how-to-split-up-discussion with a reviewer online should make everybody do their split ups beforehand :) | 15:58 |
| BjornT | bigjools: right. although, i do think that looking at a real diff which is too big is also helpful. | 15:59 |
| bigjools | yup | 15:59 |
| danilos | BjornT: people usually have blueprints which document the big picture as well | 15:59 |
| bigjools | I get the impression that massive diffs arise when starting to work on something that's not Ready To Code | 16:00 |
| sinzui | BjornT: I had a branch that I explained I did not have time to review, but I could review part of it if it was divided into: components and vocabularies, model changes, view changes. I got the components and vocabularies branch 20 minutes latter. | 16:00 |
| bigjools | sinzui: right - some stuff like that is so easy to split out | 16:00 |
| sinzui | I think cprov didn't see that he could split them out because they were dead code until the mode or view changes landed | 16:01 |
| BjornT | bigjools: still, most people don't do it. they only do it if the branch gets too big, and then it's usually quite a lot of work splitting it up. | 16:01 |
| bigjools | BjornT: agreed - we could be more efficient here | 16:02 |
| cprov | sinzui: sorry, split-up which code ? | 16:02 |
| sinzui | cprov: You had a large branch once that I did not have time to review, but you kindly broke it into 3 or 4 parts so i could start the review. | 16:03 |
| * bigjools heads off for caffeine | 16:04 | |
| cprov | sinzui: a previous branch, right, I remember something. It actually happened several time with me. | 16:05 |
| cprov | and I'm proud of it :-/ | 16:05 |
| cprov | it's happen again, actually, `copy-ui_constraints` r=barry. The diff has grown up to 900 lines. | 16:06 |
| === salgado is now known as salgado-lunch | ||
| === salgado-lunch is now known as salgado | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!