| === Ursinha-afk is now known as Ursinha | ||
| bac | thumper, rockstar, stub, mwhudson, stevek, lifeless -- Reviewer Meeting in a few minutes | 00:59 |
|---|---|---|
| mwhudson | hi bac | 00:59 |
| bac | hi mwhudson | 00:59 |
| mwhudson | bac: how is asia treating you? | 00:59 |
| bac | mwhudson: good. having fun. eating well. not ill. | 01:00 |
| mwhudson | bac: all good | 01:01 |
| bac | #startmeeting | 01:01 |
| MootBot | Meeting started at 19:01. The chair is bac. | 01:01 |
| MootBot | Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] | 01:01 |
| bac | good morning...who is here? | 01:01 |
| bac | me | 01:01 |
| mwhudson | me | 01:02 |
| mwhudson | thumper won't be here | 01:02 |
| wgrant | me | 01:02 |
| * wallyworld here | 01:03 | |
| bac | [topic] agenda | 01:04 |
| MootBot | New Topic: agenda | 01:04 |
| bac | * Roll call | 01:04 |
| bac | * Agenda | 01:04 |
| bac | * Outstanding actions | 01:04 |
| bac | * Mentat update. | 01:04 |
| bac | * salgado (ui) | 01:04 |
| bac | * henninge (ui) | 01:04 |
| bac | * stevenk (code) | 01:04 |
| bac | * New topics | 01:04 |
| bac | * Nice Storm gotcha (bigjools) | 01:04 |
| bac | * canonical.launchpad is still deprecated, sinzui (AsiaPac meeting only) | 01:04 |
| bac | * [[http://paste.ubuntu.com/507156/|Truth conditionals and the new "all" and "any" functions]], henninge | 01:04 |
| bac | [topic] mentoring update | 01:04 |
| MootBot | New Topic: mentoring update | 01:04 |
| bac | it may be news to you here that henninge and salgado are UI mentats. they are doing really well. please send your UI reviews to them first, if you can spare the extra time mentoring entails. | 01:05 |
| bac | i've been using them both lately and getting really good feedback from them. | 01:05 |
| bac | also, sinzui reports the number of UI reviews overall has gone up quite a bit in the last two months, which is great. | 01:06 |
| bac | there are no UI reviewers in this timezone. do you normally just throw it at rockstar? | 01:06 |
| bac | since stevenk and thumper are both absent we won't get a report out out of them... | 01:07 |
| mwhudson | i haven't had a ui review to do in aaaaages | 01:07 |
| bac | [topic] Nice Storm gotcha (bigjools) | 01:08 |
| MootBot | New Topic: Nice Storm gotcha (bigjools) | 01:08 |
| mwhudson | is this the one that wgrant knows all about? | 01:08 |
| bac | wgrant do you want to discuss this issue? | 01:08 |
| wgrant | Indeed. | 01:08 |
| * wgrant finds the rev. | 01:08 | |
| bac | wgrant: did you discover it or just fix it? | 01:08 |
| wgrant | I broke, discovered, and fixed it. | 01:09 |
| wgrant | https://code.edge.launchpad.net/~wgrant/launchpad/bug-653382-domination-series-restriction/+merge/37333 | 01:09 |
| bac | the trifecta | 01:09 |
| wgrant | Saying "SomeClass.column in (some list of values)" will always evaluate to True. | 01:09 |
| wgrant | This can be... dangerous. | 01:09 |
| wgrant | So please watch for it. | 01:10 |
| wgrant | Probably a Storm bug, but also probably not fixable. | 01:10 |
| wallyworld | all bugs are fixable, no? | 01:10 |
| bac | wgrant: do you have any sense of why it is evaluating to True? makes no sense on the surface | 01:12 |
| lifeless | hi | 01:12 |
| bac | hi lifeless | 01:12 |
| lifeless | sorry for tardiness | 01:12 |
| wgrant | bac: Storm columns have an __eq__ which returns an Expr. | 01:12 |
| wgrant | An Expr is not False. | 01:12 |
| wgrant | I suspect this is the problem. | 01:12 |
| lifeless | whats the exact code in question? | 01:13 |
| mwhudson | oh right, you can't override what the list does for __contains__ | 01:13 |
| wgrant | mwhudson: You can override from the other end. | 01:13 |
| bac | wgrant: and the work-around is to use is_in | 01:13 |
| wgrant | bac: Right. | 01:13 |
| wgrant | And it non-intuitively fails in the wrong direction. | 01:13 |
| mwhudson | lifeless: https://code.edge.launchpad.net/~wgrant/launchpad/bug-653382-domination-series-restriction/+merge/37333 | 01:13 |
| lifeless | right | 01:13 |
| lifeless | theres a broader issue | 01:14 |
| lifeless | storms syntax often looks right but is wrong | 01:14 |
| lifeless | for instance Class.column == otherclass.column | 01:14 |
| lifeless | is nearly always wrong. | 01:14 |
| wgrant | Is it? | 01:14 |
| lifeless | yes, because one or both are Reference objects | 01:15 |
| lifeless | not Int objects | 01:15 |
| wgrant | Oh, right, yeah. | 01:15 |
| lifeless | the right spelling, with SQLBase is Class.columnID == otherclass.columnID | 01:15 |
| wgrant | But that at least generates an error. | 01:15 |
| lifeless | wgrant: not always, for entertainment value. | 01:15 |
| lifeless | (or Class.columnID == otherclass.column, or vice versa) | 01:16 |
| wgrant | That has always irked me, and it seems like it should be fixable. | 01:16 |
| wgrant | Have we talked to Stormers about it? | 01:16 |
| lifeless | comment on the bug I filed | 01:16 |
| bac | regarding the first problem with 'in', sinzui has volunteered to see if he can create a matching regex to throw into lint to catch them. | 01:17 |
| lifeless | that would be nice, *if* when that is found, we always write a test (because it implies missing test coverage) | 01:18 |
| bac | returning a bit to UI reviews, henninge mentioned that a lot of people are including screenshots, which are great. he also said noodles has created some screencasts which have been helpful. an example is at https://code.edge.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572 | 01:19 |
| bac | given my mad video skillz, i could see this easily taking me twice as long as writing the patch so i doubt i'll try it | 01:19 |
| bac | [topic] canonical.launchpad is still deprecated, sinzui (AsiaPac meeting only) | 01:20 |
| MootBot | New Topic: canonical.launchpad is still deprecated, sinzui (AsiaPac meeting only) | 01:20 |
| bac | this topic was from last week in the AmEu meeting. | 01:20 |
| bac | apologies for me having to late cancel the meeting here last week. | 01:20 |
| bac | sinzui was disturbed to see some people adding to the code in canonical.launchpad | 01:21 |
| bac | reviewers should look out for that | 01:22 |
| lifeless | Thats quite likely me, and I'll happily defend doing that if needed. | 01:22 |
| lifeless | moving stuff around doesn't fix bugs or improve performance | 01:22 |
| lifeless | it doesn't help our users | 01:22 |
| lifeless | it *may* make things easier to maintain. | 01:23 |
| lifeless | Not moving things around doesn't increase the cost of maintenance, avoids conflicts with db-stable and focuses on getting things done. | 01:23 |
| bac | lifeless: it is deprecated and the agreed goal is to evacuate it. | 01:24 |
| lifeless | right | 01:24 |
| lifeless | but thats separate to fixing specific bugs or issues. | 01:24 |
| bac | i don't think the issue is with making short-term fixes there but trying to avoid growing a lot of new code | 01:24 |
| bac | with the recognition that it is sometimes very tangled and hard to extract | 01:25 |
| lifeless | As PEP8 says, fitting in with an existing structure is often better than being pure but inconsistent. | 01:25 |
| lifeless | I *have* extracted things from there when it made sense. | 01:25 |
| bac | the appeal was for reviewers to keep their eyes open for it and expect a solid argument for why it makes sense. | 01:26 |
| lifeless | So I think this adds friction to no benefit | 01:26 |
| lifeless | I don't think reviews should be worrying about trivia like this, its not a good use of the reviewer or reviewees time. | 01:26 |
| lifeless | sure, mention it - 'are you aware we're trying to kill that namespace' | 01:27 |
| lifeless | but that should be the limit, IMNSHO. | 01:27 |
| mwhudson | i'm not sure that the proposal was for anything more than that | 01:28 |
| lifeless | mwhudson: 'solid argument' sounds considerably more. | 01:28 |
| lifeless | I would be very sad to see someone double or triple the time to make a change, because a reviewer wouldn't approve the branch unless they moved the code | 01:28 |
| bac | lifeless: well, if i was given a flimsy argument to that question i think i'd be concerned | 01:29 |
| lifeless | why? | 01:29 |
| mwhudson | there is a more general discussion here along the lines of 'how do we make sure changes to our coding standard actually happen'\ | 01:29 |
| mwhudson | lifeless: it sounds like you're very opposed to using reviews for this | 01:29 |
| bac | in my mind it would me the developer hadn't thought it through. | 01:29 |
| lifeless | mwhudson: yes, I am. I don't think coding standards do much for our code. | 01:30 |
| bac | if it were a non-crititical patch in week one and the developer didn't have a good reason, then i'd say it is entirely appropriate to ask them to not add new code there | 01:30 |
| mwhudson | that sounds like a topic for the mailing list :) | 01:30 |
| lifeless | mwhudson: I'm also concerned because I'm not seeing my architectural stuff, which is endemic in the code base, getting airplay. | 01:30 |
| lifeless | mwhudson: I've been letting the hindbrain think about this for a while; yes its probably time to start discussing, I think I broadly know what I want to say now. | 01:32 |
| lifeless | bac: I think requiring them to have a good reason suggests a stop-energy workflow and heavy rather than light touch process. | 01:32 |
| mwhudson | lifeless: in some sense any change that happens as a result review is too late | 01:32 |
| lifeless | bac: driving iteration time down means doing less each time, but taking more bites at it. | 01:33 |
| mwhudson | lifeless: even more so for architectural stuff, if you want a check on that, i think it needs to come earlier | 01:33 |
| lifeless | bac: combining tasks because both need to be done, and both happen to be in the same region of code, reverses the stuff needed to drive iteration time down. | 01:33 |
| lifeless | bac: and there is broad agreement that small chunks are easier to review (both to assess fo rlanding, and simply to keep up with whats changing in the code base); so conflating the long term effort with short term stuff seems purely counterproductive to me. | 01:34 |
| mwhudson | $ python -c 'import this' | grep -i now | 01:34 |
| mwhudson | Now is better than never. | 01:34 |
| mwhudson | Although never is often better than *right* now. | 01:34 |
| bac | lifeless: i guess it depends on the scope. | 01:34 |
| lifeless | mwhudson: Yes, by the time someone has added a function to a class rather than adding a class; its rather late. OTOH asking 'does this scale well... how do you know' is different to asking 'how should we do this to scale well'. | 01:35 |
| lifeless | mwhudson: and both questions are needed. | 01:36 |
| lifeless | (arguably) | 01:36 |
| wallyworld | my take is that the bigger picture architectural stuff should be discussed pre-implementation, FWIW | 01:36 |
| wallyworld | since by the code review stage, it's too late, as i think lifeless said | 01:37 |
| mwhudson | lifeless: yes, that's likely true | 01:37 |
| lifeless | wallyworld: I would love to see folk discussing the details of their goals on the list before doing stuff, OTOH a blocking process isn't very in-the-zone-friendly. | 01:37 |
| lifeless | wallyworld: So I vigorously defend folks right to JFDI | 01:38 |
| wallyworld | yes, in the list of with their team | 01:38 |
| wallyworld | s/of/or | 01:38 |
| lifeless | wallyworld: specifically on the mailing list; team chats on voice and irc chats in channels other than launchpad-dev are invisible to many people. | 01:38 |
| lifeless | There are plenty of studies showing that for group intelligence, the more *participants* the better the outcome. | 01:38 |
| wallyworld | ack | 01:38 |
| lifeless | here's what I want: if someone sees a bug, they can: | 01:39 |
| lifeless | - fix it | 01:39 |
| lifeless | - land the fix | 01:39 |
| lifeless | - in a short period of time | 01:39 |
| lifeless | - get feedback on what | 01:40 |
| lifeless | - *must* be made better before deployment | 01:40 |
| lifeless | - *can* be made better if someone wants to | 01:40 |
| lifeless | - *should* be improved before they forget the context for what they did | 01:40 |
| mwhudson | i also wonder if (in some cases) we should land stuff behind a feature flag | 01:40 |
| mwhudson | as an almost matter of course | 01:40 |
| lifeless | I think we do much of this today, but in an order that has many many handoffs. | 01:41 |
| lifeless | mwhudson: +1 | 01:41 |
| wallyworld | IMHO, this architectural stuff needs to be right up there since with the small incremental changes we tend to do, it's way to easy to get architectural decay over time, like the frog in the boiling water analogy | 01:41 |
| mwhudson | then it becomes "what must be done before the flag turned on" | 01:41 |
| lifeless | mwhudson: I think it would be a sensible default. See my mail to the list later this afternoon. | 01:41 |
| mwhudson | lifeless: :-) | 01:41 |
| mwhudson | because turning on flags out of order is way easier than deploying revisions out of order | 01:41 |
| bac | mwhudson: good point | 01:41 |
| lifeless | wallyworld: I think its all too easy to get gearing-issues. | 01:41 |
| lifeless | wallyworld: Are you familiar with LEAN ? | 01:42 |
| wallyworld | yes, but not an expert | 01:42 |
| lifeless | value stream maps ring a bell ? | 01:42 |
| mwhudson | lifeless: an advantage of blocking landing until something has been done is that it's less likely to be forgotten and less damaging (in some ways) if it is | 01:42 |
| wallyworld | no | 01:42 |
| lifeless | wallyworld: http://en.wikipedia.org/wiki/Value_stream_mapping | 01:42 |
| mwhudson | lifeless: kanban boards probably help with this though | 01:43 |
| lifeless | wallyworld: each /type/ of review, and each time someone blocks/context switches /in/ a review is a handoff | 01:43 |
| lifeless | a VSM of our landing process is scary scary stuff | 01:43 |
| lifeless | mwhudson: a bit, but its also an accomodation to having a painful process. | 01:44 |
| lifeless | concretely, lets say that we asked every team lead to *enforce* an architectural review : transparency, performance, cohesion, etc, on *every landing* | 01:44 |
| wallyworld | lifeless: agree, which is why i mentioned that if the bigger picture stuff were done pre-implementation, it wouldn't hopefully be an issue come review time | 01:45 |
| lifeless | wallyworld: pre-impl is part of the VSM | 01:45 |
| lifeless | bac: so, sorry that this has scaled into a larger discussion. | 01:45 |
| bac | but i think pre-impls are not being done consistently or with much vigor | 01:46 |
| lifeless | I don't think preimpls work. | 01:46 |
| bac | lifeless: not a problem. it is a good discussion | 01:46 |
| lifeless | because they are another blocking step in doing stuff | 01:46 |
| lifeless | people will often chat about things | 01:46 |
| lifeless | but a detailed preimpl at the level needed to catch these things, given our maturity as a project, is actually a big deal. | 01:47 |
| wallyworld | better to put in some effort up front and catch any issue early rather than pay a bigger cost later IMHO | 01:48 |
| bac | lifeless: it seems like you're saying each opportunity for collaboration is a blocking point to be avoided. | 01:48 |
| lifeless | bac: no, I'm saying *requirements to collaborate* are harmful. | 01:49 |
| lifeless | wallyworld: some years ago I would have agreed with you; now I think we need to identify where things could have been done better and train individuals so the next time is better. | 01:49 |
| lifeless | bac: here's an example. If you *had* to have a preimpl call, came up with a good idea on saturday, who would you speak to? or would you wait until sunday when I'm around (my monday) | 01:50 |
| bac | lifeless: i don't see that we have those 'requirements to collaborate'. we have suggested collaboration points, if warranted. | 01:50 |
| wallyworld | hmmm. not sure i agree :-) look at methodologies life orthogonal defect classification etc - all designed to catch issues early since the cost of fixing later is too great | 01:51 |
| lifeless | wallyworld: this ties into the cycle time. if the cycle time is a days work, more or less, how much will you gain/lose ? | 01:51 |
| lifeless | wallyworld: the risk is bounded, the delay is minimal. | 01:51 |
| bac | lifeless: the only time we've *enforced* pre-imp calls is with community contributors and only to ensure their work is on target and desirable | 01:52 |
| lifeless | bac: I'm addressing wallyworld's argument, not what we have today. Today reviews are the mandatory choking point. | 01:52 |
| wallyworld | i would warrant if it takes a day up from to save the collective project cost of bad implementation, recfactoring costs etc later, then it's worth it :-) | 01:52 |
| lifeless | bac: and I'm arguing that we need to take trivia out of that choke point and put things that matter in. | 01:52 |
| lifeless | wallyworld: I couldn't parse that, sorry. | 01:53 |
| wallyworld | sorry, if it takes a day up front to save much more time later due to the cost of bad implementation, factoring etc, then it's worth it | 01:54 |
| wallyworld | but the real answer IMHO it "it depends" - it's a judgement call as to what effort is needed depending on the downstream risk and cost of getting it wrong | 01:54 |
| mwhudson | another thought is that tying some of these discussions to branches is a bit silly | 01:55 |
| bac | wallyworld: and i do see that in practice. the answer to a reviewer request is often "that's a good idea but out of scope. i'll open a tech-debt bug and put in an XXX but i'd like to land what i have now." | 01:55 |
| wallyworld | lifeless: "things that matter" = getting the architecture right | 01:56 |
| wallyworld | bac: yes. i think it comes down to our collective experience as competenent software engineers to do The Right Thing | 01:56 |
| bac | this has been good, but let's move on | 01:57 |
| bac | [topic] [[http://paste.ubuntu.com/507156/ |Truth conditionals and the new "all" and "any" functions]], henninge | 01:58 |
| MootBot | New Topic: [[http://paste.ubuntu.com/507156/ |Truth conditionals and the new "all" and "any" functions]], henninge | 01:58 |
| bac | so, this was a request to change our style guide to account for any() and all() | 01:58 |
| bac | after some good discussion we decided not to change the contional rules and to incorporate them into the use of any() if desired | 01:59 |
| bac | henning was agreeable and is going to update the style guide | 02:00 |
| lifeless | one sec | 02:00 |
| lifeless | so, I think 'if foo:' is perfectly cromulent python and we should use it. | 02:01 |
| lifeless | when its an accurate statement of the desired logic. | 02:01 |
| bac | the concensus from the discussion is that the truth conditional can be problematic as is masks what is being tested. there was strong support for making those tests explicit | 02:03 |
| * mwhudson is on a call now, drifting out | 02:03 | |
| lifeless | bac: by that logic we shouldn't use functions | 02:04 |
| lifeless | because they mask what they do | 02:04 |
| bac | a recent example of where we didn't follow the requirement was your suggestion to wgrant this week | 02:04 |
| lifeless | right | 02:04 |
| lifeless | the long winded version was *wrong* | 02:04 |
| lifeless | (buggy) | 02:05 |
| bac | yes, b/c he really did need to check all of the conditions that are implied by the shorter test | 02:05 |
| lifeless | its a mistake to avoid a powerful idiom in the language simply because its brief. | 02:05 |
| lifeless | there can be good reasons to avoid it, but brevity isn't one IMO. | 02:06 |
| lifeless | (for instance, storm result sets don't define a __nonzero__, and they need to define one which raises, I suspect) | 02:06 |
| bac | the reason for avoiding it isn't the inherent brevity. brevity is good, is readable, and takes less typing. | 02:06 |
| bac | the rationale is that implicitly testing for None and "" and 0 often leads to mistakes. | 02:08 |
| bac | lifeless: i urge you to bring your argument up on the list. | 02:08 |
| bac | i also think we need to have a multisession throw down at the epic to cover a lot of these topics. doing it in person may be the best way to get real concensus with everyone's buy in | 02:09 |
| lifeless | I'll put it in my queue; its not something I feel strongly compelled to debate at this point in time (because to me this falls in the bucket of 'we shouldn't be specifying this in reviews anyway') | 02:10 |
| bac | i think our process may be at a point similar to where we were at MIT and those sessions were tremendously valuable in affecting change | 02:10 |
| lifeless | bac: tell me more? | 02:11 |
| bac | the sessions with the poppen-people | 02:11 |
| bac | mary and tom | 02:11 |
| bac | i think we made fundamental changes to our process that were great | 02:11 |
| bac | and i think we may be equally stagnant now and need something similar to address the problems | 02:12 |
| bac | the reviewers meeting is the wrong forum, especially since it is disjointed | 02:12 |
| bac | the ML doesn't seem to work for big changes | 02:12 |
| bac | anyway, we've had a wide-ranging long discussion here and i appreciate it. | 02:13 |
| bac | but i need to get some breakfast. | 02:13 |
| lifeless | I have one more thing :) | 02:13 |
| bac | thank you all for participating | 02:13 |
| lifeless | sorry! | 02:14 |
| bac | #endmeetin... | 02:14 |
| bac | yes? | 02:14 |
| lifeless | I'd like to find out how reviewers have been going with discussions about the architectural guide stuff | 02:14 |
| lifeless | has is slipped your mind? been tried and too hard? No context? would training help? is it too vague? or too wacky? | 02:15 |
| bac | i cannot say. we discussed the idea and it seemed to be well received. i have not done follow up. | 02:15 |
| lifeless | this is important to me, and I'm being asked about it by mgmt folk :) | 02:15 |
| bac | i don't think this is a change that is going to happen by simple encouragement in a meeting or in a list message | 02:15 |
| lifeless | bac: if you have suggestions about getting it to happen, I'm all ears. | 02:16 |
| lifeless | bac: after your breakfast, perhaps :) | 02:17 |
| bac | lifeless: i have nothing useful ATM | 02:17 |
| bac | lifeless: sure | 02:17 |
| bac | anything else? | 02:17 |
| bac | 3 | 02:17 |
| bac | 2 | 02:17 |
| bac | 1 | 02:17 |
| bac | #endmeeting | 02:17 |
| lifeless | bac: failing that, can we at least add a 'retrospective on arch guide' to the next meeting | 02:17 |
| MootBot | Meeting finished at 20:17. | 02:17 |
| bac | lifeless: let's talk later. i'm pretty busy for the next four hours. does that run past your EOD? | 02:17 |
| lifeless | my day is all confused today | 02:18 |
| lifeless | ping me | 02:18 |
| bac | ok | 02:18 |
| === Ursinha is now known as Ursinha-afk | ||
| === maxb_ is now known as maxb | ||
| === Ursinha-afk is now known as Ursinha | ||
| === salgado is now known as salgado-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === gary_poster_ is now known as gary_poster | ||
| === Ursinha-afk is now known as Ursinha | ||
| Ursinha | OOPS-1741SMS1 | 18:34 |
| ubot5 | https://lp-oops.canonical.com/oops.py/?oopsid=1741SMS1 | 18:34 |
| === salgado is now known as salgado-afk | ||
| === EdwinGrubbs is now known as Edwin-lucnh | ||
| === Edwin-lucnh is now known as Edwin-lunch | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!