• Immutable Page
• Info
• Attachments
• More Actions: Raw Text Print View Render as Docbook Delete Cache ------------------------ Check Spelling Like Pages Local Site Map ------------------------ Rename Page Delete Page ------------------------ Subscribe User ------------------------ Remove Spam Revert to this revision Package Pages Sync Pages ------------------------ Load Save SlideShow

ReviewerMeeting20090408

summary

• reviewers should scrutinize and question any code in a branch that isn't used. this is general policy, more important now as we move to 3.0, especially with javascript branches. e.g. avoid general purpose solutions.
• no problem adding js code to launchpad, but the above especially applies to lazr-js branches.

• read the email

• try to refactor code before refactoring tests. even think about splitting up such branches.

logs

Apr 08 10:04:15 <barry> #startmeeting
Apr 08 10:04:16 <MootBot>       Meeting started at 09:04. The chair is barry.
Apr 08 10:04:16 <MootBot>       Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
Apr 08 10:04:36 <barry> hello everyone and welcome to this week's ameu reviewers meeting.  who's here today?
Apr 08 10:04:39 <rockstar>      me
Apr 08 10:04:59 <flacoste>      me
Apr 08 10:05:07 <EdwinGrubbs>   me
Apr 08 10:05:27 <intellectronica>       me
Apr 08 10:05:35 *       al-maisan (n=al-maisa@p5087BD46.dip.t-dialin.net) has joined #launchpad-meeting
Apr 08 10:05:39 <danilos>       me
Apr 08 10:05:42 <al-maisan>     me
Apr 08 10:05:52 <salgado>       me
Apr 08 10:06:22 <barry> adeuring: ping
Apr 08 10:06:27 <barry> allenap: ping
Apr 08 10:06:29 <adeuring>      me
Apr 08 10:06:34 <barry> bac: ping
Apr 08 10:06:37 <barry> BjornT: ping
Apr 08 10:06:43 <bac>   me
Apr 08 10:06:44 <barry> cprov: ping
Apr 08 10:06:46 <allenap>       me
Apr 08 10:06:54 <barry> mars: ping
Apr 08 10:07:07 <barry> sinzui: ping
Apr 08 10:07:36 <barry> [TOPIC] agenda
Apr 08 10:07:37 <MootBot>       New Topic:  agenda
Apr 08 10:07:48 <barry>  * Roll call
Apr 08 10:07:48 <barry>  * Peanut gallery (anything not on the agenda)
Apr 08 10:07:48 <barry>  * Action items
Apr 08 10:07:48 <barry>  * Mentoring update
Apr 08 10:07:48 <barry>  * Code must be used once or more to land (excluding tests) (intellectronica)
Apr 08 10:07:48 <barry>  * Avoid refactoring both code and tests in the same branch (intellectronica)
Apr 08 10:08:02 <cprov> me
Apr 08 10:08:10 <barry> let's skip around a bit
Apr 08 10:08:30 <sinzui>        me
Apr 08 10:08:34 <barry> [TOPIC]  * Code must be used once or more to land (excluding tests) (intellectronica)
Apr 08 10:08:35 <MootBot>       New Topic:   * Code must be used once or more to land (excluding tests) (intellectronica)
Apr 08 10:08:50 <barry> intellectronica: the floor is yours
Apr 08 10:09:00 <intellectronica>       this follows from a discussion we had a couple of weeks ago in the ajax swat team
Apr 08 10:09:25 <intellectronica>       working on general purpose solutions before they are being used is risky and hard
Apr 08 10:09:37 <BjornT>        me
Apr 08 10:09:43 <intellectronica>       and we want to avoid that as much as possible, especially when working on JS code
Apr 08 10:09:46 <danilos>       intellectronica: what if we are not talking about general purpose solutions but parts of solutions?
Apr 08 10:10:08 <intellectronica>       danilos: same thing
Apr 08 10:10:31 <danilos>       (I know what you are aiming at, but the problem is more generic: just like there's a rule to not do premature optimization, we should not over-design stuff up-front, but let the design evolve from actual use cases)
Apr 08 10:10:34 <intellectronica>       anyway, this is pretty much policy now, and shouldn't be debated here. the only relevant point for reviews is:
Apr 08 10:10:48 <danilos>       well, I strongly disagree
Apr 08 10:10:53 <salgado>       so do I
Apr 08 10:10:59 <BjornT>        intellectronica: i think i agree with danilos
Apr 08 10:11:14 <BjornT>        it's ok to land something that isn't used, if you intend to use it in the next branch
Apr 08 10:11:20 <BjornT>        it helps keeping the branches small
Apr 08 10:11:21 <danilos>       BjornT: +1
Apr 08 10:11:22 <adeuring>      me too; the rule is in many cases useful, but i think there are many exceptions
Apr 08 10:11:49 <intellectronica>       BjornT: it's ok if it's not in the same branch, but the reviewer should inquire about that, and maybe ask to see the branch where it's being used
Apr 08 10:11:50 <BjornT>        landing something that you think will be useful, but don't have any plans to use should of course be discouraged
Apr 08 10:11:58 <barry> BjornT: i agree, i just wouldn't call that "not used" :)  i.e. it's just an artificial decomposition to keep branches manageable
Apr 08 10:12:12 <al-maisan>     database patches are a classic example -- you land them first and use them later
Apr 08 10:12:34 <danilos>       how would this work with landing DB patches a cycle before? that's a clear violation of the rule, or it means waiting two cycles "for nothing"?
Apr 08 10:12:36 <intellectronica>       lazr code, for example, will, by-definition, not be in the same branch as the launchpad code that uses it
Apr 08 10:13:05 <cprov> al-maisan: db-patches are always *used* since the current code has to cope with it.
Apr 08 10:13:20 <intellectronica>       danilos: this is particulary relevant for javascript code. deviations are ok at your discretion. and of course stuff like db patches must me done that way, and is pretty low risk
Apr 08 10:13:25 <barry> i also think it's fine to question code that you don't see used in a branch, and it's fine if the dev says "oh, that's in the next branch"
Apr 08 10:13:33 <al-maisan>     I guess we need to agree on what constitutes "use" then
Apr 08 10:13:50 <danilos>       intellectronica: which means it's not a rule, but a recommendation, specifically aimed at JS work, and should be described as such (IMO, at least)
Apr 08 10:14:15 <BjornT>        barry: right. it's ok to question it, as long as it's allowed to land such branches
Apr 08 10:14:29 <danilos>       barry: we've done that for ages, but I am willing to hear out intellectronica concrete recommendations for JS
Apr 08 10:14:35 <danilos>       :)
Apr 08 10:14:38 <barry> BjornT: +1
Apr 08 10:15:05 <barry> danilos: i'm all for avoiding premature generalizations :)
Apr 08 10:15:17 <danilos>       :)
Apr 08 10:15:21 <intellectronica>       let's use our common sense. what we're trying to discourage is the development of general-purpose solutions separately from using them, where the risk is high
Apr 08 10:16:07 <barry> intellectronica: i get that, esp. for js stuff.  you think you see a general pattern when doing some ui stuff before it's proven itself
Apr 08 10:16:09 <intellectronica>       b.t.w if you haven't already, please read the relevant post to the mailing list. i only wanted to mention this here because reviews are the last point in the development cycle where we can stop this
Apr 08 10:16:39 <intellectronica>       what we really want to do is think about this pre-implementation
Apr 08 10:16:53 <danilos>       intellectronica: it feels to me reviews are a bit too late to think about this
Apr 08 10:17:01 <intellectronica>       danilos: i agree
Apr 08 10:17:29 <intellectronica>       but if this wasn't done before, reviews are our last chance, before unused, and possibly unusable, code, lands
Apr 08 10:17:33 <danilos>       intellectronica: you are only going to get negative opinions of the review process if you get blocked at that final step; I think that's something we should encourage in pre-imp calls
Apr 08 10:17:35 *       e-jat (n=fenris@ubuntu/member/fenris-) has joined #launchpad-meeting
Apr 08 10:18:18 <intellectronica>       danilos: you can gain something from identifying this in a review. the acknowledgedment that the work is incomplete
Apr 08 10:18:51 <intellectronica>       the direct result will be that the work can be completed immediately, instead of shelved for a while, which is almost always worse
Apr 08 10:19:02 <danilos>       intellectronica: that's ok as long as it's not a de facto blocker for landing stuff... I see the point, but it should clearly be at reviewers' discretion
Apr 08 10:19:03 <barry> i actually don't think this is a big departure from current practice, unless intellectronica is saying we MUST reject such branches
Apr 08 10:19:30 <danilos>       barry: I was under the impression he was, which is why you hear me complaining :)
Apr 08 10:19:42 <intellectronica>       barry: we only MUST reject lazr-js branches like this. everything else is open for negotiation
Apr 08 10:19:42 *       kfogel (n=Karl@cpe-24-193-42-111.nyc.res.rr.com) has joined #launchpad-meeting
Apr 08 10:20:18 <barry> intellectronica: isn't this why we have the ui= tag now?  <wink>
Apr 08 10:20:21 <EdwinGrubbs>   I think the JS general purpose solutions are little different, since we usually know that we are going to re-use them, but this increases the size of the project to several weeks. I don't think that whether we know we will use the general purpose solution is a good question to help decide where to split up the increments.
Apr 08 10:20:23 <danilos>       I think we are departing from agile development as much as we can, and I don't like that
Apr 08 10:20:53 <intellectronica>       barry: no. beuno is unlikely to pay attention to this in his review. he reviews the UI itself, not the code or how it's structured
Apr 08 10:20:55 <flacoste>      danilos: hmm, not really, continuous integration is agile
Apr 08 10:21:10 <intellectronica>       danilos: how so? that sounds very agile to me
Apr 08 10:21:13 <danilos>       for lazr-js, we should definitely have nicer way to add JS directly into LP tree, and then we wouldn't even be having this discussion
Apr 08 10:21:13 <flacoste>      danilos: and basically what intellectronica is arguing against is landing unintegrated code
Apr 08 10:21:28 <flacoste>      danilos: no, it's not about that
Apr 08 10:21:34 <intellectronica>       flacoste: exactly!
Apr 08 10:21:52 <flacoste>      danilos: it's about not developping widgets that don't have an integration scenario
Apr 08 10:21:54 <danilos>       flacoste: well, the code is used in the sample page in lazr-js
Apr 08 10:22:08 <danilos>       flacoste: as I said, I see the point, I think the approach to solving it is completely wrong
Apr 08 10:22:22 <flacoste>      danilos: yes, but that's what intellectronica is arguing against, the example page should be developped later, not as the first integration case
Apr 08 10:22:23 <intellectronica>       danilos: there's no problem adding js code to the LP tree, and in fact my recommendation is that you develop like that and only move the code to lazr-js when it's ready. but that's up to the developer, on a case by case basis
Apr 08 10:22:37 <danilos>       flacoste: in practice, you'll have people develop lazr-js widgets, then having that sit (with small changes) while they fight over integrating it
Apr 08 10:22:58 <barry> intellectronica: that seems like a reasonable approach to me
Apr 08 10:22:59 <intellectronica>       danilos: right, and that's exactly what we're trying to avoid
Apr 08 10:23:00 <danilos>       intellectronica: the problem is that it's not clear what the best practice is
Apr 08 10:23:23 <intellectronica>       danilos: please refer to the email thread, if you haven't read it yet
Apr 08 10:23:33 <intellectronica>       the best practice is "integration first"
Apr 08 10:23:38 <danilos>       intellectronica: well, then reviewers meeting is completely wrong place to discuss it, and other than engaging in pre-imp calls, I don't want to block branches at review stage
Apr 08 10:23:57 <danilos>       intellectronica: as I said, I agree with that, but don't see how it's relevant to blocking at review level
Apr 08 10:24:24 <barry> i think you're right that it's better to discuss this policy further on the ml thread
Apr 08 10:24:36 <intellectronica>       i agree that this is not relevant _just_ to reviews, i only wanted to mention this because this is our last opportunity to deal with this. ideally, we should plan to work like that from the beginning, and you won't see this in reviews very often
Apr 08 10:24:38 <danilos>       i.e. "you *should* have done this completely differently" -- if we get to that point, we are doing something badly
Apr 08 10:24:39 <flacoste>      well, the policy is kind of in place
Apr 08 10:24:41 <intellectronica>       barry: i agree
Apr 08 10:24:50 <flacoste>      reviewers should just be mindful of it
Apr 08 10:24:55 <flacoste>      and remember
Apr 08 10:25:04 <flacoste>      reviewers are good candidates for pre-impl call
Apr 08 10:25:10 <flacoste>      so i think that this topic was more about
Apr 08 10:25:23 <flacoste>      "make sure you are up-to-date with the mailing-list thread" :-)
Apr 08 10:25:30 <barry> flacoste: +1
Apr 08 10:25:35 <intellectronica>       flacoste: +1
Apr 08 10:25:45 <intellectronica>       with that, i think we can move on
Apr 08 10:25:49 <barry> thanks.  let's move on
Apr 08 10:26:04 <barry> [TOPIC]  * Avoid refactoring both code and tests in the same branch (intellectronica)
Apr 08 10:26:04 <MootBot>       New Topic:   * Avoid refactoring both code and tests in the same branch (intellectronica)
Apr 08 10:26:23 <intellectronica>       ah yes, another issue that's not really about reviews :)
Apr 08 10:26:43 <danilos>       well, I feel this one is, because it's much harder to review stuff that combines refactoring of both
Apr 08 10:26:48 <danilos>       :)
Apr 08 10:27:08 <intellectronica>       danilos: yes. it came up in a review :)
Apr 08 10:27:14 <barry> as long as the refactoring isn't necessary to actually keep the test suite happy
Apr 08 10:27:24 <danilos>       barry: of course
Apr 08 10:27:25 <barry> or if the refactoring is "minor"
Apr 08 10:27:26 <intellectronica>       of course
Apr 08 10:27:37 <cprov> danilos: will you disagree of anything intellectronica says today ? :)
Apr 08 10:27:58 <flacoste>      danilos is looking for a fight i think ;-)
Apr 08 10:28:01 <danilos>       cprov: you mean agree? :)
Apr 08 10:28:33 *       barry wonders who's gonna get voted off the channel today
Apr 08 10:28:42 <danilos>       well, I am agreeing with intellectronica this time (at least the topic he proposed :), it's just that he seemed a bit uncertain after the first one :)
Apr 08 10:28:43 <cprov> danilos: I dunno, you tell me ...
Apr 08 10:29:17 <barry> do we need to discuss this topic more?
Apr 08 10:29:28 <intellectronica>       not as far as i'm concerned
Apr 08 10:29:31 <salgado>       maybe we could just put up some ads saying that if you (developer) refactor just code, you may get an rs= for some reviewer
Apr 08 10:29:32 <danilos>       anyway, this is a good think to point out, but we should make this a bit more well advertised imo
Apr 08 10:29:50 <danilos>       salgado: ah, sounds really nice!
Apr 08 10:29:54 <danilos>       salgado: +1
Apr 08 10:30:02 <salgado>       however, if you refactor both code and tests, you'll need a real review that may take much longer
Apr 08 10:30:07 <danilos>       provided there's existing test coverage for the code
Apr 08 10:30:14 <salgado>       that way we encourage them to do what's more appropriate
Apr 08 10:30:19 <salgado>       s/them/us
Apr 08 10:31:06 <al-maisan>     stupid question: why are we not to refactor both code and tests in the same branch? Because of the size?
Apr 08 10:31:10 <barry> salgado: i'm all for making it easier to land tech debt reduction branches
Apr 08 10:31:47 <danilos>       al-maisan: no, because when you refactor only code, and tests still pass, we have much bigger certainty that your refactoring is good (even if we don't understand all the gory details of your code)
Apr 08 10:32:10 <al-maisan>     aha
Apr 08 10:32:11 <barry> danilos: so maybe we need even more scrutiny of test refactoring branches?
Apr 08 10:32:14 <salgado>       al-maisan, IMO, it makes it harder to find whether or not your test refactoring left any code untested
Apr 08 10:32:15 <intellectronica>       al-maisan: no, because part of the utility provided by the test suite is confirming that your refactoring is ok
Apr 08 10:32:28 <al-maisan>     thanks!
Apr 08 10:32:32 *       noodles775 would love to have a refactor-only branch :)
Apr 08 10:32:40 <sinzui>        salgado: I was planning to get lots of rs's from you on Friday to move some tests and rm some empty directories created by the migration script
Apr 08 10:32:53 <danilos>       barry: maybe, but I'd be scared of proposing that :)
Apr 08 10:33:05 <barry> :)
Apr 08 10:33:12 <salgado>       sinzui, I'd be glad to give you them, but Friday is a holiday here
Apr 08 10:33:28 <barry> sinzui: i think it's only /not/ a holiday in the us
Apr 08 10:33:54 <sinzui>        salgado: then I will ask your your rs tomorrow
Apr 08 10:33:54 <danilos>       anyway, I think salgado's idea is a nice one, and we can start offering rs=me for code refactoring only branches
Apr 08 10:34:15 <barry> danilos: i'd be okay with that
Apr 08 10:34:25 <intellectronica>       i'm not sure i understand what the difference is, in this case
Apr 08 10:34:37 <intellectronica>       you're still required to actually review the code, right?
Apr 08 10:34:45 <danilos>       intellectronica: this is just a way of promoting it
Apr 08 10:35:01 <barry> intellectronica: well, if the tests pass and aren't touched...
Apr 08 10:35:02 <danilos>       intellectronica: I think that's what's missing, telling people how we like our code baked
Apr 08 10:35:04 <intellectronica>       seems confusing to me, and i don't see the benefits
Apr 08 10:35:46 <salgado>       the way I was thinking, I'd glance through the code, check that there are no changes to tests and give an rs=me if the test suite pass
Apr 08 10:36:06 <intellectronica>       salgado: but there are other aspects of the code you'll want to review, no?
Apr 08 10:36:11 <noodles775>    and it might still be worth a 3rd party review to ensure there is good test coverage in the first place.
Apr 08 10:36:17 <danilos>       salgado: I'd also like to be pointed at existing test for that particular code :)
Apr 08 10:36:30 <noodles775>    danilos: exactly.
Apr 08 10:36:33 <salgado>       intellectronica, depending on the kind of the refactoring, yes
Apr 08 10:36:35 <intellectronica>       that it's well documented, orgnised in a maintainable fashion, formatted correctly, etc
Apr 08 10:37:19 <cprov> salgado: that sounds more like an easy r=me than an rs
Apr 08 10:37:29 <salgado>       intellectronica, I'd decide that after reading the cover letter and glancing through the diff
Apr 08 10:37:50 <intellectronica>       salgado: sure, makes sense
Apr 08 10:38:07 <barry> anyway, shall we move on?
Apr 08 10:38:32 <barry> [TOPIC] action items
Apr 08 10:38:32 <MootBot>       New Topic:  action items
Apr 08 10:38:41 <barry>  * flacoste to look into storm/sqlobject result set compatibility
Apr 08 10:38:41 <salgado>       cprov, I don't agree.  even when I give an rs=me, I usually glance at the diff
Apr 08 10:38:52 <salgado>       they're imcompatible
Apr 08 10:38:59 <salgado>       and not meant to be compatible
Apr 08 10:39:26 <barry>  * flacoste to work on API reviewer cheat sheet
Apr 08 10:39:53 <flacoste>      i started
Apr 08 10:39:57 <flacoste>      will finish for next week
Apr 08 10:40:03 <flacoste>      i promise
Apr 08 10:40:09 <flacoste>      although it's not worth much these days
Apr 08 10:40:18 <barry> flacoste: which one?
Apr 08 10:40:22 <flacoste>      cheat sheet
Apr 08 10:40:26 <barry> cool
Apr 08 10:40:29 <danilos>       flacoste: because of the economy crisis, or? :)
Apr 08 10:40:33 <flacoste>      the previous is actually on allenap plate
Apr 08 10:40:34 <salgado>       lol
Apr 08 10:40:35 <cprov> salgado: maybe, but it's seems to be all about offering review-friendly code, sometimes r sometimes rs, the difference isn't very clear.
Apr 08 10:40:44 <barry> flacoste: okay, i'll change that
Apr 08 10:41:03 <cprov> salgado: (not necessarily disagreeing on what you said, now.)
Apr 08 10:41:09 <barry> cprov: rs= almost always means "i didn't look at it, i trust you"
Apr 08 10:41:26 <allenap>       flacoste: Do we want to try and update the Storm tree we're using, or shall I back port the IResultSet adapter?
Apr 08 10:41:43 <flacoste>      allenap: better to backport
Apr 08 10:41:55 <flacoste>      allenap: we are looking at updating Storm, but there was a bunch test failures
Apr 08 10:42:13 <flacoste>      allenap: so if we want it now, it's a lot easier to backport and I think that it's a fairly small change?
Apr 08 10:42:32 <danilos>       flacoste: maybe it's something for each team to look into (if you have a branch with updated Storm, I'd be happy to have translations-related tests looked over)
Apr 08 10:42:51 <allenap>       flacoste: Yep, it merges cleanly, or at least it did last time I tried.
Apr 08 10:42:54 <danilos>       anyway, not the right place for this discussion, I guess
Apr 08 10:43:38 <barry> yep, we'll keep it on the agenda for next week
Apr 08 10:43:53 <barry> gary's not here so we'll skip his action item
Apr 08 10:44:09 <barry> since we're almost out of time, i'll just ask if there's anything quick anybody else has?
Apr 08 10:45:05 <barry> 5...4...3...2...1
Apr 08 10:45:07 <barry> #endmeeting