ReviewerMeeting20101006

Not logged in - Log In / Register

summary

logs

ameu

[15:00] <bac> #startmeeting
[15:00] <MootBot> Meeting started at 09:00. The chair is bac.
[15:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[15:00]  * bigjools is here
[15:00] <bac> who is here?
[15:00] <mars> me
[15:00] <wgrant> me
[15:00] <bigjools> ha, I read your mind!
[15:00] <noodles775> me
[15:00] <henninge> me
[15:00] <sinzui> me
[15:00] <mars> bac, gary-dentist sends his apologies
[15:01] <jelmer> me
[15:01] <abentley> me
[15:01] <bac> mars: i was assuming that would be the case
[15:01] <bac> and deryck as well
[15:02] <bac> EdwinGrubbs, danilos, gmb: pings
[15:02] <danilos> me
[15:02] <bac> flacoste: ping
[15:02] <EdwinGrubbs> me
[15:02] <flacoste> me
[15:03] <bac> allenap: yo
[15:03]  * bac thought the mass invite was such a good idea...
[15:03] <bac> [topic] agenda
[15:03] <bac> * Roll call
[15:03] <bac>  * Agenda
[15:03] <bac>  * Outstanding actions
[15:03] <bac>  * Mentat update.
[15:03] <bac>    * salgado (ui)
[15:03] <MootBot> New Topic:  agenda
[15:03] <bac>    * henninge (ui)
[15:03] <bac>    * stevenk (code)
[15:03] <bac>  * New topics
[15:03] <bac>   * Nice Storm gotcha (bigjools)
[15:03] <bac>   * canonical.launchpad is still deprecated, sinzui (AsiaPac meeting only)
[15:03] <bac>   * [[http://paste.ubuntu.com/507156/|Truth conditionals and the new "all" and "any" functions]], henninge
[15:04] <adeuring> me
[15:04] <allenap> me
[15:04]  * sinzui read that as "sinzui is deprecated" At least he is not obsolete
[15:04] <bac> salgado: ping
[15:04] <bac> [topic] mentoring update
[15:04] <MootBot> New Topic:  mentoring update
[15:05] <salgado> me!
[15:05] <sinzui> we are getting lots of UI reviews
[15:06] <sinzui> I do mean a lot, more than last month
[15:06] <sinzui> well, more than aug and jul
[15:06] <salgado> some weeks I get more, others I get less
[15:06] <henninge> I enjoy them but please try to provide screenshots ...
[15:06] <henninge> ... or even screencasts, like noodles775 does ;-)
[15:07] <salgado> yeah, screen[shots,casts] help a lot for small changes
[15:08] <salgado> but when they're not trivial I like to exercise them manually to test for corner cases and all that
[15:08] <henninge> true. Did not have any of those yet, though ... ;)
[15:10] <noodles775> Yeah, I think it's nearly always worth exercising manually - I just provide the screencasts to demo and get the context.
[15:11] <salgado> it's also nice when devs add demo data (aka dev sample data) that makes it easy to exercise the changes
[15:11] <bac> sorry about that, my internet died
[15:11] <bac> what did i miss?
[15:11] <sinzui> Paris is in flames
[15:11] <salgado> bac, http://paste.ubuntu.com/507276/
[15:11] <bigjools> how did they do that when they're on strike?
[15:12] <danilos> salgado, +1 for adding dev sampledata! I want to see much more of it
[15:12] <bac> noodles775: can you provide a link to one of your screencasts?  i've never seen one
[15:13] <noodles775> bac: https://code.edge.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572
[15:13] <bac> noodles775: thanks
[15:13] <noodles775> (the mp has the screencast as well as a script for sample data setup).
[15:13] <bac> [topic] new topics
[15:13] <MootBot> New Topic:  new topics
[15:13] <bac> [topic] Nice Storm gotcha (bigjools)
[15:13] <MootBot> New Topic:  Nice Storm gotcha (bigjools)
[15:13] <wgrant> :(
[15:13] <bigjools> heh
[15:14] <bigjools> I'd like to point you all to revision 11670 of devel
[15:14] <bigjools> notice the change - a storm expression was using Python's "in" instead of Storm's is_in
[15:15] <bigjools> this has been quite catastrophic for soyuz
[15:15] <bigjools> because the former was evaluating to True all the time
[15:15] <danilos> :(
[15:15] <sinzui> yuck
[15:15] <bigjools> so this is just a warning to not fuck up quite as spectacularly
[15:15] <wgrant> You'd think it would fail to False, but nooo.
[15:16] <bigjools> I'm wondering if we can get it lint checked?
[15:16] <sinzui> it's in a string, and pyflakes/pep8 ignore string content
[15:17] <wgrant> It's not in a string.
[15:17] <bigjools> or perhaps make Storm DTRT
[15:17] <wgrant> I'm not sure that Storm can be fixed.
[15:17] <bac> bigjools: is there a storm bug?
[15:17] <sinzui> but I think I can write a regexp that locates problems like this
[15:17] <wgrant> It's probably just seeing that __eq__ returns something that isn't False.
[15:17] <bigjools> bac: I don't know if it can be a bug or not
[15:17] <bac> bigjools: i mean, have we filed a bug or discussed with the stormers
[15:18] <bigjools> bac: not yet, I'm still in the middle of cleaning up the mess
[15:18] <bac> right
[15:18] <bigjools> when things are calmer I can do that
[15:18] <bigjools> anyway, that's it from me
[15:19] <bac> sinzui: are you going to take a look to see if lint can catch it?  can i assign you an item?
[15:19] <sinzui> I am
[15:19] <bac> [action] sinzui to try to fix lint wrt storm gotcha
[15:19] <MootBot> ACTION received:  sinzui to try to fix lint wrt storm gotcha
[15:19] <bigjools> it would be inside a .find() of course
[15:19] <bac> [topic] * [[http://paste.ubuntu.com/507156/|Truth conditionals and the new "all" and "any" functions]], henninge
[15:20] <MootBot> New Topic:  * [[http://paste.ubuntu.com/507156/|Truth conditionals and the new "all" and "any" functions]], henninge
[15:20] <henninge> everybody please read the paste ;-)
[15:20] <henninge> http://paste.ubuntu.com/507156/
[15:20] <bac> here's the link again http://paste.ubuntu.com/507156
[15:20] <MootBot> LINK received:  http://paste.ubuntu.com/507156/
[15:21] <allenap> bigjools, sinzui: Storm should probably not compile a raw True. It's a smell that something is wrong.
[15:21]  * sinzui has used if val not in (None, '')
[15:21] <allenap> henninge: any() and all() can consume generator expressions.
[15:21] <henninge> yes, any iterable
[15:21] <allenap> any(translation is not None and translation != "" for translation in translations)
[15:22] <henninge> yeah, ok ...
[15:22] <henninge> ;-)
[15:22] <bigjools> allenap: yes, good point
[15:22] <henninge> although is that not a 2.6ism with out the [] ?
[15:23] <allenap> henninge: I was against the explicit truthiness testing originally, but if we're having it then I feel we should stick to it.
[15:23] <allenap> No, it worked on 2.5 I think.
[15:23] <bigjools> FWIW jelmer and I found a bug in Soyuz code earlier where it was using "if thing:"
[15:23] <bigjools> instead of something more explicit
[15:24] <abentley> allenap, but also: len([translation is not None and translation != "" for translation in translations]) != 0
[15:24] <allenap> And, fwiw, I'm on-board with explicit conditionals now.
[15:24] <henninge> ok, I had not thought of allenap's solution,
[15:24] <henninge> that would work well for me, too.
[15:25] <henninge> No softening the policy, then?
[15:25] <allenap> abentley: That's cool, but I think any() and all() do add to readability, and they can shortcut too.
[15:25] <bac> henninge: sounds like there is not much support
[15:26] <danilos> I kind of feel like we should losen it
[15:26] <bigjools> I think they are dangerous
[15:26] <danilos> any() sounds quite to the point to me (i.e. it's not exactly like "if something" vs "if something is None", where being explicit does add value; with any() you are being explicit to a point)
[15:27] <henninge> danilos: but it *is* doing "if something" internally and you cannot do anything about it.
[15:27] <danilos> henninge, right, that's why I agree we should see what everybody feels about it :)
[15:28] <henninge> ;-)
[15:28] <sinzui> I see any as explicit. It looks like it is removing redundancy from the code
[15:28] <abentley> allenap: I'm okay with the semantics of your revised version, but I don't agree it's more readable.  It's one more thing to remember how it works.
[15:29] <henninge> abentley: I find the word "any" quite obvious ...
[15:29] <allenap> danilos: henninge's example could be split into two lines to enhance readability: valid_translations = (t is not None and t != "" for t in translations); if any(valid_translations): ...
[15:29] <noodles775> +1 for allenap's suggestion.
[15:29] <bac> allenap: but at that point you could just see if you have an empty list, no?
[15:29] <allenap> It's a builtin too; no hiding from it.
[15:30] <abentley> henninge: not me.  Does it mean "length > 1" or does it mean "anything in this iterable is true"?
[15:30] <allenap> bac: The generator expression combined with any() means you can shortcut as soon as a translation is available.
[15:30] <henninge> abentley: well, the latter.
[15:31] <allenap> If translations is expensive to iterate over then it could make a difference.
[15:31] <abentley> henninge: That's not the most obvious meaning to me, but with storm, "any" has yet another meaning.
[15:31] <danilos> I don't think we need a solution to replace bare "any()" use in this meeting, we should just decide whether we want to stop basic usage as non-explicit
[15:31] <allenap> abentley: Yeah, that's unfortunate.
[15:31] <danilos> allenap, it is not, it's a list of at most 6 items
[15:32] <allenap> danilos: Here, okay, but in general it might be useful to shortcut evaluation.
[15:32] <allenap> danilos: Which is what the for loop does.
[15:33] <danilos> allenap, right, but as I said, I am not interested in valid use cases of any(), but rather, invalid, in terms of our "be-explicit" policy :)
[15:33] <allenap> Anyway, I thought this was just about being explicit in conditionals? I think we should be explicit everywhere, including in any() and all().
[15:33] <danilos> allenap, yeah, but not many people have an opinion on any() in particular
[15:34] <henninge> allenap: right, that was my questions and that is an answer ... ;)
[15:34] <henninge> danilos: no opinion, no change ...
[15:34] <danilos> henninge, that's right, let's move on bac :)
[15:35] <bac> henninge: right.  would you still update the page to include the summary?
[15:35] <henninge> bac: I am done. I will add a comment to the style guide to suggest allenaps code.
[15:35] <bac> thanks
[15:35] <henninge> :)
[15:35] <bac> [topic] anything else?
[15:35] <MootBot> New Topic:  anything else?
[15:36] <bac> no?
[15:36] <bac> #endmeeting
[15:36] <MootBot> Meeting finished at 09:36.
[15:36] <bac> thanks for coming
[15:36] <henninge> thanks bac!
[15:39] <bigjools> cheers bac

asiapac

[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:02] <mwhudson> me
[01:02] <mwhudson> thumper won't be here
[01:02] <wgrant> me
[01:03]  * wallyworld here
[01:04] <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:05] <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:06] <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:07] <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:08] <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:09] <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:10] <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:12] <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:13] <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:14] <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:15] <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:16] <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:17] <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:18] <lifeless> that would be nice, *if* when that is found, we always write a test (because it implies missing test coverage)
[01:19] <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:20] <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:21] <bac> sinzui was disturbed to see some people adding to the code in canonical.launchpad
[01:22] <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:23] <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:24] <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:25] <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:26] <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:27] <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:28] <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:29] <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:30] <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:32] <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:33] <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:34] <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:35] <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:36] <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:37] <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:38] <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:39] <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:40] <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:41] <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:42] <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:43] <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:44] <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:45] <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:46] <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:47] <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:48] <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:49] <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:50] <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:51] <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:52] <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:53] <lifeless> wallyworld: I couldn't parse that, sorry.
[01:54] <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:55] <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:56] <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:57] <bac> this has been good, but let's move on
[01:58] <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:59] <bac> after some good discussion we decided not to change the contional rules and to incorporate them into the use of any() if desired
[02:00] <bac> henning was agreeable and is going to update the style guide
[02:00] <lifeless> one sec
[02:01] <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:03] <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:04] <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:05] <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:06] <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:08] <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:09] <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:10] <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:11] <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:12] <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:13] <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:14] <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:15] <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:16] <lifeless> bac: if you have suggestions about getting it to happen, I'm all ears.
[02:17] <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:18] <lifeless> my day is all confused today
[02:18] <lifeless> ping me
[02:18] <bac> ok

ReviewerMeeting20101006 (last edited 2010-10-13 12:19:16 by bac)