ReviewerMeeting20100901

Not logged in - Log In / Register

ReviewerMeeting20100901

summary

logs

ameu

No meeting due to scheduling conflict.

asiapac

[01:00] <bac> #startmeeting
[01:00] <MootBot> Meeting started at 19:00. The chair is bac.
[01:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[01:00]  * rockstar  
[01:00] <bac> y'all are an anxious bunch
[01:00]  * wallyworld 
[01:00]  * mwhudson is a bit hungry
[01:00] <bac> lifeless, StevenK: ping
[01:01]  * StevenK waves
[01:01] <bac> hurrah
[01:01] <lifeless> hi
[01:01]  * thumper is hungry too
[01:01] <bac> woot, we're all here
[01:01] <wallyworld> thumper is always hungry
[01:01]  * bac just had a lovely dinner
[01:01] <bac> [topic] agenda
[01:01] <MootBot> New Topic:  agenda
[01:01] <bac> * Roll call
[01:01] <bac>  * Agenda
[01:01] <bac>  * Outstanding actions
[01:01] <bac>  * New topics
[01:01] <bac>   * Henning Rocks.  One-import-per-line now done and in effect.
[01:01] <bac>   * Mentat update.
[01:01] <bac>   * gary: benji is checking in our generated WADL and adding tests of the WADL generated for our webservice versions.  This is supposed to mean that changing the frozen versions of the webservice in a backwards-incompatible way will be more easily caught by reviewers: don't allow it! It should also mean that running make initially will be noticeably faster.
[01:01] <bac>   * Some of the UI reviewers have graduated, but ReviewerSchedule indicates that no one has graduated.
[01:02] <bac>   * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews?
[01:02] <bac>  * Peanut gallery
[01:02] <bac> [topic] outstanding stuff
[01:02] <MootBot> New Topic:  outstanding stuff
[01:02] <bac> lifeless, can you remind us of the lib/coop mess was settled?
[01:02] <lifeless> yes its settled. We all know what it is and that the name sucks.
[01:02] <thumper> what is it?
[01:02] <thumper> remind me plz
[01:03] <lifeless> it is buganswers, answersbranches etc
[01:03] <thumper> ok
[01:03] <lifeless> I'm in favour of a rename to 'inter' if we bother.
[01:03] <lifeless> but I don't think that that actually removes the need for folk to read the package docstring
[01:03] <lifeless> which is entirely clear.
[01:03] <bac> ok, thanks
[01:04] <lifeless> as I don't think it will remove that need, I'm in favour of doing nothing and worrying about other stuff
[01:04] <bac> i should mention there was no AMEU meeting today due to me having a dr's appt and forgetting all about it
[01:04] <lifeless> no worries
[01:05] <bac> last week we noted henning did a great job on the import script. \o/
[01:05] <bac> [topic] mentat update
[01:05] <MootBot> New Topic:  mentat update
[01:05] <bac> thumper, StevenK: how goes it?
[01:05] <thumper> slowly
[01:05] <StevenK> What he said
[01:06] <bac> i must apologize for breaking the mentoring cycle last week as i had a branch that i really wanted to land.  hope you still got good feedback from edwin, StevenK
[01:06] <StevenK> I've figured out that reviewing is effectively asking yourself or the submitter a bunch of questions. Now to learn what are good questions to ask.
[01:06] <bac> StevenK: geez, you're channeling kiko
[01:06] <StevenK> Argh!
[01:07] <StevenK> bac: It's fine, urgent is well, urgent.
[01:07] <bac> [topic] WADL files
[01:07] <MootBot> New Topic:  WADL files
[01:07] <bac> * gary: benji is checking in our generated WADL and adding tests of the WADL generated for our webservice versions.  This is supposed to mean that changing the frozen versions of the webservice in a backwards-incompatible way will be more easily caught by reviewers: don't allow it! It should also mean that running make initially will be noticeably faster.
[01:07] <bac> did that email go out?  i don't recall seeing it
[01:07]  * gary_poster is not really here (hi!)
[01:07] <bac> anyway, i'm just the messenger
[01:08] <bac> gary_poster: ^^?
[01:08] <mwhudson> i don't recall an email or a landing to that effect yet
[01:08] <gary_poster> benji couldn't land it because it depended on a new version of lazr.restful that was being integrated for another effort
[01:08] <gary_poster> still pending
[01:08] <lifeless> we make no guarantees about devel though, do we ?
[01:08] <gary_poster> correct, lifeless
[01:08] <lifeless> just 1.0
[01:08] <thumper> lifeless: no we dont'
[01:08] <lifeless> phew
[01:08] <gary_poster> and "beta"
[01:08] <lifeless> ah
[01:09] <bac> [topic] * Some of the UI reviewers have graduated, but ReviewerSchedule indicates that no one has graduated.
[01:09] <wgrant> beta is only Karmic, right?
[01:09] <MootBot> New Topic:  * Some of the UI reviewers have graduated, but ReviewerSchedule indicates that no one has graduated.
[01:09] <gary_poster> wgrant: yeah, I think that's right
[01:09] <bac> i sent a request to rockstar to update the wiki
[01:09] <bac> who added this agenda item?
[01:09]  * rockstar completed the request
[01:09] <bac> rockstar: thanks!
[01:09] <lifeless> wgrant: until yesterday beta was still on the 'jahacking the api' wiki page
[01:10] <rockstar> Although I should say that everyone should be a ui* or javascript*
[01:10] <lifeless> bac: bryceh noticed the disconnect
[01:10] <StevenK> bac: I noticed that myself -- also the wiki page still shows jelmer is being mentored by noodles.
[01:10] <bac> lifeless: ok.  it's nice to put your [name] on the items
[01:10]  * rockstar hates wikis
[01:10] <lifeless> bac: sure. I didn't add that one :P
[01:11] <lifeless> bac: I did the next, I forgot toblame myself.
[01:11] <bac> lifeless: no, but...
[01:11] <lifeless> ^W^Wtake credit
[01:11] <bac> [topic]  * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews?
[01:11] <MootBot> New Topic:   * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews?
[01:11] <bac> lifeless: that has your fingerprints!
[01:11] <lifeless> I raised this at the Epic in my presentation
[01:11] <lifeless> I've been slack writing it up.
[01:11] <lifeless> its now in a moderately consumable form.
[01:12] <lifeless> I'm asking the review team to take on a new challenge
[01:12] <lifeless> which is to help people get their code to meet the values on that page, and [its experimental] the metrics.
[01:13] <lifeless> I know the metrics are bad and wrong. But it would be nice to have some, so lets try and iterate.
[01:13] <lifeless> This is a form of review that none of the current guides covered
[01:13] <lifeless> but I feel it is very much in the spirit of review - helping each other end up with code that does what we want and is easy to care for
[01:14] <lifeless> -fin
[01:14] <bac> lifeless: i notice you've been following up on a lot of reviews.  those follow ups are excellent reminders to keep the architecture in mind.
[01:14] <lifeless> bac: I read every review
[01:14] <bac> lifeless: but, of course, you cannot do it yourself.  i'll bring this to the larger crowd.
[01:14] <lifeless> bac: but sometimes I skim :)
[01:15] <lifeless> I've got two goals with that page
[01:15] <lifeless> firstly, I hope that by adding this info explicitly into our memesphere
[01:16] <lifeless> folk will turn up at review with these values already embodied in their code
[01:16] <lifeless> secondly, where they haven't, I hope they have a discussion with the reviewer and can decide whether they want to invest and improve things immediately, or before they move onto the next kanban card, or $schedule-phrase
[01:17] <mwhudson> right, as per usual, the review is too late to catch this sort of problem in an ideal world
[01:17] <lifeless> mwhudson: exactly
[01:17] <rockstar> abentley and I have regular calls every day and they usually spin into pre-implementation calls.  That seems to help.
[01:17] <bac> mwhudson: yes, ideally these issues would be raised in a preimp call
[01:18] <rockstar> It's kinda like swimming with a buddy, only without the swimming, and with coding.
[01:18] <lifeless> :)
[01:18] <bac> and no hurricanes
[01:18] <lifeless> so anyhow, I didn't find anything that matched the structure-of-code in our guidelines
[01:18] <rockstar> bac, oh, there are hurricanes.  Have you seen the buildd code?  :)
[01:18] <lifeless> they were all pretty cosmetic or surface stuff
[01:18] <bac> do you have hurricanes in NZ?
[01:18] <thumper> oh yes
[01:18] <bac> is that what you call them?
[01:18] <thumper> not really tornados though
[01:18] <thumper> yes hurricanes
[01:19] <lifeless> I'm hoping this page can stay focused on structure and outcomes
[01:19] <thumper> in fact that is also the name of the Wellington super 14 rugby team
[01:19]  * rockstar was hoping to learn the Maori word for hurricane.
[01:19] <lifeless> and we can spin specific techniques off to PythonSG / Performance pages etc etc
[01:19]  * mwhudson adds to the distraction: http://en.wikipedia.org/wiki/Cyclone_Bola
[01:19] <rockstar> lifeless, we're listening, we promise.
[01:19] <bac> thanks lifeless for powering through our meteorological distractions
[01:19] <lifeless> thats ok, its all there for mootbot
[01:20] <mwhudson> lifeless: +1 to all that
[01:20] <bac> so you *do* call them something else
[01:20] <bac> thanks robert
[01:21] <bac> that's it for the scheduled items
[01:21] <bac> does anyone have anything else to discuss?
[01:21] <thumper> not today
[01:21] <rockstar> bac, I would like to suggest an experiment.
[01:21] <bac> rockstar: go
=== Ursinha-afk is now known as Ursinha
[01:21] <thumper> oh?
[01:21] <rockstar> bac, I've been thinking about this a lot, and trying it in my own development.
[01:22] <rockstar> I would like to suggest we make our review diff line max much smaller.
[01:22] <rockstar> Like, maybe by half.
[01:22] <lifeless> +1 if I can get a permanent exception
[01:22] <bac> rockstar: is this part of the salgado-hovey cadence grail?
[01:22] <lifeless> :P
[01:22] <rockstar> lifeless, exceptions can be evaluated on a case-by-case basis.
[01:23] <rockstar> bac, well, yeah, if that's what you want to call it.  thumper and I had been experimenting with it before the epic though.
[01:23] <rockstar> Reviews are SO EASY when they are small and concise.  It's easy to tell where you might have missed some tests, etc.
[01:23] <bac> i spent a lot of time thinking about curtis' talk and it is clear it only works if you're riding a fixed gear bike
[01:23] <bac> on the flats
[01:23] <lifeless> rockstar: I think thats true with new code, but its really not true with existing code.
[01:23] <wgrant> But it's sometimes possible to get large refactorings down below 800 lines.
[01:24] <wgrant> 400 lines... I doubt it.
[01:24] <StevenK> I've been discovering most feature branches are easily over 600 lines
[01:24] <lifeless> rockstar: I routinely, at the moment hit 700 lines, on *trivial* things, because there is so much debt.
[01:24] <wgrant> We'd have a *lot* more exceptional branches.
[01:24] <wgrant> lifeless: Exactly.
[01:24] <bac> lifeless: yep
[01:24] <rockstar> lifeless, the debt should be reviewed separately than the actual change.
[01:24] <bac> i had a silly branch today that came in at 602
[01:24] <mwhudson> i worry that LOC is a poor surrogate for complexity
[01:24] <rockstar> mwhudson, this is true.
[01:24] <rockstar> Especially with twisted code.
[01:24] <bac> and all i did was catch an exception and display a nice message
[01:25] <lifeless> rockstar: so, I think its great to *encourage* focused branches, but not great to *penalise* ones that are large but still focused.
[01:25] <lifeless> mwhudson: yes.
[01:25] <bac> make it a goal, but one you an exceed by 100%
[01:25] <StevenK> Perhaps a wording change is in order?
[01:25] <lifeless> rockstar: when we *add* effort to do something *simple*, we're doing it wrong.
[01:25] <rockstar> Like I said, experiment.
[01:25] <lifeless> rockstar: I'm merely pointing out concerns.
[01:25] <StevenK> "We'd prefer diffs to be 400 lines or less, but should review diffs of up to 800 lines."
[01:26] <bac> rockstar: you said you've been doing it a while.  how has it worked out?
[01:26] <rockstar> lifeless, yeah, and I thank you for that.  I know it won't happen all the time, but judging by thumper's branches, we aren't staying under 800 anyway.
[01:26] <rockstar> bac, I think it's brilliant.
[01:26] <StevenK> Over 800 lines may require bribes, at the reviewers discretion?
[01:26] <lifeless> StevenK: already does.
[01:26] <rockstar> StevenK, that's how it works now.
[01:26] <lifeless> but I think that setting a review limit *misses the point*
[01:27] <lifeless> the benefit of focused branches is that they are focused and fast and easy.
[01:27] <rockstar> lifeless, yeah, it's only a limit as far as our current limit.
[01:27] <bac> lifeless: but a review *target* doesn't
[01:27] <lifeless> bac: no, it does.
[01:27] <lifeless> the current limit misses the point.
[01:27] <lifeless> it was a surrogate when we introduced it, and its a surrogate now.
[01:27] <rockstar> You don't have to land your branches separately.  You can keep them in a pipe or whatever.  I think getting them reviewed in chunks is a good idea.
[01:27] <lifeless> Its obviously a poor surrogate because rockstar and others are arguing that its wrong
[01:28] <rockstar> lifeless, not wrong, but sub-optimal maybe.
[01:28] <lifeless> I would rather say 'Have no more than N changes in your thing to be reviewed'
[01:28] <lifeless> where we might say N is - 4.
[01:29] <thumper> I think we should have minus four changes too
[01:29] <lifeless> If you can describe in english, without tricks, your entire branch in 4 simple clauses, its got to be pretty focused.
[01:29] <lifeless> if you need more to describe it, its less focused.
[01:30] <lifeless> (this is off the cuff, I'm simply saying that lets talk about the root issue: our *target for developers* is a surrogate. Maybe there is a better surrogate, if we feel people are failing to achieve focus)
[01:30] <rockstar> lifeless, I think 1 change is enough though.
[01:30] <StevenK> Does that include drive-bys, though?
[01:30] <rockstar> I think one of the problems is that we are trying to fix too many things.
[01:31] <rockstar> StevenK, yes.  Ask thumper about the little drive-by fix in his refactoring branch last week.  :)
[01:31] <thumper> it has always been a soft limit anyway
[01:31] <lifeless> rockstar: 1 is ideal, but is definitely not enough for anything deep. Perhaps I'm seeing a biased slice of the code.
[01:31] <rockstar> Yeah, and I'm saying maybe, as a team, we should ratchet down the soft limit and see if that helps the team.
[01:31] <mwhudson> i have to be honest: i've basically ignored the 800 line limit for years
[01:32] <lifeless> me too
[01:32] <thumper> mwhudson: and done what?
[01:32] <bac> mwhudson: i'm a pretty big offender too.  sinzui will review anything.
[01:32] <mwhudson> thumper: tried to stay focused
[01:32] <mwhudson> i think my branches are mostly under 800 lines
[01:32] <StevenK> I use 800 lines as "Uh oh, I should try and split this monster up."
[01:32] <rockstar> bac, yeah, when my manager asks me to review 2X lines of code, what do I say? No?  :)
[01:32] <mwhudson> i have no idea -- that's what i mean by ignore
[01:33] <bac> rockstar: yeah, but it's the other way around...
[01:33] <rockstar> I think mwhudson's approach is the best.  lifeless illustrated a good way to measure it, but 4 things is too many.
[01:33] <rockstar> MAYBE 2 things.
[01:33]  * bac calls time
[01:33] <lifeless> I can propose another approach
[01:33] <mwhudson> pipelines ftw, as usual
[01:33] <lifeless> bac: if I may
[01:33] <rockstar> mwhudson, yes.
[01:33] <bac> lifeless: ok
[01:33] <StevenK> Why not both, we recommend 2 things, but up to 4, per discretion
[01:33] <lifeless> remove the limit. Gone. Replace it with:
[01:34] <bac> lifeless: wrap it up as mwhudson is hungry
[01:34] <lifeless>  * if the reviewer feels its too big to review sensibly, from *whatever perspectve*, they ask the reviewee to split it with pipelines.
[01:34] <thumper> +1
[01:34] <lifeless> we don't have 5K and 10K branches coming in anymore
[01:34] <lifeless> the cultural is radically different.
[01:34] <thumper> thankfully
[01:35] <mwhudson> lifeless: +1
[01:35] <bac> lifeless: is there an easy way to do that?  even with pipelines?
[01:35] <thumper> add-pipe --before
[01:35] <thumper> merge -i
[01:35]  * bac may need a tutorial
[01:35] <bac> +1 then
[01:36] <lifeless> limits like this are always going to be a discussion, so lets make it explicit about the tradeoffs: ease of review / clarity etc and let the participants have the discussion
[01:36] <mwhudson> didn't aaron do a tutorial at the epic about this?
[01:36] <lifeless> bac: there are various ways; pipelines are pretty nice.
[01:36] <lifeless> bac: or just a regular branch + merge -i
[01:36] <bac> mwhudson: yeah but it assumed lots of familiarity with pipelines already
[01:36] <mwhudson> ok
[01:36] <lifeless> personally, I try to just do enough to be confident in the change and stop there
[01:37] <lifeless> by the time I need a stack of things, I've usually gotten myself tied up in knots already
[01:37] <mwhudson> as always, it depends
[01:37] <mwhudson> my blueprints hacking last week feel easily into three steps: move, sanitize, change
[01:38] <lifeless> mwhudson: you forgot delete.
[01:38] <mwhudson> it helps if you can see the steps ahead of you i guess
[01:38] <bac> rockstar thanks for bringing up the topic.   i think the discussion was helpful.  we'll need more time to figure out a plan.  perhaps lifeless and i can talk before next week to come up with a solid proposal.
[01:38]  * rockstar acks
[01:39] <lifeless> bac: drop me a mail ?
[01:39] <bac> any other things?
[01:39] <bac> lifeless: sure
[01:39] <thumper> lets finish plz
[01:39] <bac> 3...
[01:39] <bac> 2
[01:39] <bac> 1
[01:39] <bac> #endmeeting

ReviewerMeeting20100901 (last edited 2010-09-08 13:41:47 by bac)