| === salgado-afk is now known as salgado | ||
| barry | #startmeeting | 15:04 |
|---|---|---|
| MootBot | Meeting started at 09:04. The chair is barry. | 15:04 |
| MootBot | Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] | 15:04 |
| barry | hello everyone and welcome to this week's ameu reviewers meeting. who's here today? | 15:04 |
| rockstar | me | 15:04 |
| flacoste | me | 15:04 |
| EdwinGrubbs | me | 15:05 |
| intellectronica | me | 15:05 |
| danilos | me | 15:05 |
| al-maisan | me | 15:05 |
| salgado | me | 15:05 |
| barry | adeuring: ping | 15:06 |
| barry | allenap: ping | 15:06 |
| adeuring | me | 15:06 |
| barry | bac: ping | 15:06 |
| barry | BjornT: ping | 15:06 |
| bac | me | 15:06 |
| barry | cprov: ping | 15:06 |
| allenap | me | 15:06 |
| barry | mars: ping | 15:06 |
| barry | sinzui: ping | 15:07 |
| barry | [TOPIC] agenda | 15:07 |
| MootBot | New Topic: agenda | 15:07 |
| barry | * Roll call | 15:07 |
| barry | * Peanut gallery (anything not on the agenda) | 15:07 |
| barry | * Action items | 15:07 |
| barry | * Mentoring update | 15:07 |
| barry | * Code must be used once or more to land (excluding tests) (intellectronica) | 15:07 |
| barry | * Avoid refactoring both code and tests in the same branch (intellectronica) | 15:07 |
| cprov | me | 15:08 |
| barry | let's skip around a bit | 15:08 |
| sinzui | me | 15:08 |
| barry | [TOPIC] * Code must be used once or more to land (excluding tests) (intellectronica) | 15:08 |
| MootBot | New Topic: * Code must be used once or more to land (excluding tests) (intellectronica) | 15:08 |
| barry | intellectronica: the floor is yours | 15:08 |
| intellectronica | this follows from a discussion we had a couple of weeks ago in the ajax swat team | 15:09 |
| intellectronica | working on general purpose solutions before they are being used is risky and hard | 15:09 |
| BjornT | me | 15:09 |
| intellectronica | and we want to avoid that as much as possible, especially when working on JS code | 15:09 |
| danilos | intellectronica: what if we are not talking about general purpose solutions but parts of solutions? | 15:09 |
| intellectronica | danilos: same thing | 15:10 |
| 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) | 15:10 |
| intellectronica | anyway, this is pretty much policy now, and shouldn't be debated here. the only relevant point for reviews is: | 15:10 |
| danilos | well, I strongly disagree | 15:10 |
| salgado | so do I | 15:10 |
| BjornT | intellectronica: i think i agree with danilos | 15:10 |
| BjornT | it's ok to land something that isn't used, if you intend to use it in the next branch | 15:11 |
| BjornT | it helps keeping the branches small | 15:11 |
| danilos | BjornT: +1 | 15:11 |
| adeuring | me too; the rule is in many cases useful, but i think there are many exceptions | 15:11 |
| 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 | 15:11 |
| BjornT | landing something that you think will be useful, but don't have any plans to use should of course be discouraged | 15:11 |
| barry | BjornT: i agree, i just wouldn't call that "not used" :) i.e. it's just an artificial decomposition to keep branches manageable | 15:11 |
| al-maisan | database patches are a classic example -- you land them first and use them later | 15:12 |
| 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"? | 15:12 |
| intellectronica | lazr code, for example, will, by-definition, not be in the same branch as the launchpad code that uses it | 15:12 |
| cprov | al-maisan: db-patches are always *used* since the current code has to cope with it. | 15:13 |
| 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 | 15:13 |
| 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" | 15:13 |
| al-maisan | I guess we need to agree on what constitutes "use" then | 15:13 |
| 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) | 15:13 |
| BjornT | barry: right. it's ok to question it, as long as it's allowed to land such branches | 15:14 |
| danilos | barry: we've done that for ages, but I am willing to hear out intellectronica concrete recommendations for JS | 15:14 |
| danilos | :) | 15:14 |
| barry | BjornT: +1 | 15:14 |
| barry | danilos: i'm all for avoiding premature generalizations :) | 15:15 |
| danilos | :) | 15:15 |
| 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 | 15:15 |
| 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 | 15:16 |
| 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 | 15:16 |
| intellectronica | what we really want to do is think about this pre-implementation | 15:16 |
| danilos | intellectronica: it feels to me reviews are a bit too late to think about this | 15:16 |
| intellectronica | danilos: i agree | 15:17 |
| intellectronica | but if this wasn't done before, reviews are our last chance, before unused, and possibly unusable, code, lands | 15:17 |
| 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 | 15:17 |
| intellectronica | danilos: you can gain something from identifying this in a review. the acknowledgedment that the work is incomplete | 15:18 |
| intellectronica | the direct result will be that the work can be completed immediately, instead of shelved for a while, which is almost always worse | 15:18 |
| 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 | 15:19 |
| barry | i actually don't think this is a big departure from current practice, unless intellectronica is saying we MUST reject such branches | 15:19 |
| danilos | barry: I was under the impression he was, which is why you hear me complaining :) | 15:19 |
| intellectronica | barry: we only MUST reject lazr-js branches like this. everything else is open for negotiation | 15:19 |
| barry | intellectronica: isn't this why we have the ui= tag now? <wink> | 15:20 |
| 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. | 15:20 |
| danilos | I think we are departing from agile development as much as we can, and I don't like that | 15:20 |
| 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 | 15:20 |
| flacoste | danilos: hmm, not really, continuous integration is agile | 15:20 |
| intellectronica | danilos: how so? that sounds very agile to me | 15:21 |
| 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 | 15:21 |
| flacoste | danilos: and basically what intellectronica is arguing against is landing unintegrated code | 15:21 |
| flacoste | danilos: no, it's not about that | 15:21 |
| intellectronica | flacoste: exactly! | 15:21 |
| flacoste | danilos: it's about not developping widgets that don't have an integration scenario | 15:21 |
| danilos | flacoste: well, the code is used in the sample page in lazr-js | 15:21 |
| danilos | flacoste: as I said, I see the point, I think the approach to solving it is completely wrong | 15: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 | 15:22 |
| 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 | 15:22 |
| 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 | 15:22 |
| barry | intellectronica: that seems like a reasonable approach to me | 15:22 |
| intellectronica | danilos: right, and that's exactly what we're trying to avoid | 15:22 |
| danilos | intellectronica: the problem is that it's not clear what the best practice is | 15:23 |
| intellectronica | danilos: please refer to the email thread, if you haven't read it yet | 15:23 |
| intellectronica | the best practice is "integration first" | 15:23 |
| 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 | 15:23 |
| danilos | intellectronica: as I said, I agree with that, but don't see how it's relevant to blocking at review level | 15:23 |
| barry | i think you're right that it's better to discuss this policy further on the ml thread | 15:24 |
| 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 | 15:24 |
| danilos | i.e. "you *should* have done this completely differently" -- if we get to that point, we are doing something badly | 15:24 |
| flacoste | well, the policy is kind of in place | 15:24 |
| intellectronica | barry: i agree | 15:24 |
| flacoste | reviewers should just be mindful of it | 15:24 |
| flacoste | and remember | 15:24 |
| flacoste | reviewers are good candidates for pre-impl call | 15:25 |
| flacoste | so i think that this topic was more about | 15:25 |
| flacoste | "make sure you are up-to-date with the mailing-list thread" :-) | 15:25 |
| barry | flacoste: +1 | 15:25 |
| intellectronica | flacoste: +1 | 15:25 |
| intellectronica | with that, i think we can move on | 15:25 |
| barry | thanks. let's move on | 15:25 |
| barry | [TOPIC] * Avoid refactoring both code and tests in the same branch (intellectronica) | 15:26 |
| MootBot | New Topic: * Avoid refactoring both code and tests in the same branch (intellectronica) | 15:26 |
| intellectronica | ah yes, another issue that's not really about reviews :) | 15:26 |
| danilos | well, I feel this one is, because it's much harder to review stuff that combines refactoring of both | 15:26 |
| danilos | :) | 15:26 |
| intellectronica | danilos: yes. it came up in a review :) | 15:27 |
| barry | as long as the refactoring isn't necessary to actually keep the test suite happy | 15:27 |
| danilos | barry: of course | 15:27 |
| barry | or if the refactoring is "minor" | 15:27 |
| intellectronica | of course | 15:27 |
| cprov | danilos: will you disagree of anything intellectronica says today ? :) | 15:27 |
| flacoste | danilos is looking for a fight i think ;-) | 15:27 |
| danilos | cprov: you mean agree? :) | 15:28 |
| * barry wonders who's gonna get voted off the channel today | 15:28 | |
| 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 :) | 15:28 |
| cprov | danilos: I dunno, you tell me ... | 15:28 |
| barry | do we need to discuss this topic more? | 15:29 |
| intellectronica | not as far as i'm concerned | 15:29 |
| 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 | 15:29 |
| danilos | anyway, this is a good think to point out, but we should make this a bit more well advertised imo | 15:29 |
| danilos | salgado: ah, sounds really nice! | 15:29 |
| danilos | salgado: +1 | 15:29 |
| salgado | however, if you refactor both code and tests, you'll need a real review that may take much longer | 15:30 |
| danilos | provided there's existing test coverage for the code | 15:30 |
| salgado | that way we encourage them to do what's more appropriate | 15:30 |
| salgado | s/them/us | 15:30 |
| al-maisan | stupid question: why are we not to refactor both code and tests in the same branch? Because of the size? | 15:31 |
| barry | salgado: i'm all for making it easier to land tech debt reduction branches | 15:31 |
| 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) | 15:31 |
| al-maisan | aha | 15:32 |
| barry | danilos: so maybe we need even more scrutiny of test refactoring branches? | 15:32 |
| salgado | al-maisan, IMO, it makes it harder to find whether or not your test refactoring left any code untested | 15:32 |
| intellectronica | al-maisan: no, because part of the utility provided by the test suite is confirming that your refactoring is ok | 15:32 |
| al-maisan | thanks! | 15:32 |
| * noodles775 would love to have a refactor-only branch :) | 15:32 | |
| 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 | 15:32 |
| danilos | barry: maybe, but I'd be scared of proposing that :) | 15:32 |
| barry | :) | 15:33 |
| salgado | sinzui, I'd be glad to give you them, but Friday is a holiday here | 15:33 |
| barry | sinzui: i think it's only /not/ a holiday in the us | 15:33 |
| sinzui | salgado: then I will ask your your rs tomorrow | 15:33 |
| danilos | anyway, I think salgado's idea is a nice one, and we can start offering rs=me for code refactoring only branches | 15:33 |
| barry | danilos: i'd be okay with that | 15:34 |
| intellectronica | i'm not sure i understand what the difference is, in this case | 15:34 |
| intellectronica | you're still required to actually review the code, right? | 15:34 |
| danilos | intellectronica: this is just a way of promoting it | 15:34 |
| barry | intellectronica: well, if the tests pass and aren't touched... | 15:35 |
| danilos | intellectronica: I think that's what's missing, telling people how we like our code baked | 15:35 |
| intellectronica | seems confusing to me, and i don't see the benefits | 15:35 |
| 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 | 15:35 |
| intellectronica | salgado: but there are other aspects of the code you'll want to review, no? | 15:36 |
| noodles775 | and it might still be worth a 3rd party review to ensure there is good test coverage in the first place. | 15:36 |
| danilos | salgado: I'd also like to be pointed at existing test for that particular code :) | 15:36 |
| noodles775 | danilos: exactly. | 15:36 |
| salgado | intellectronica, depending on the kind of the refactoring, yes | 15:36 |
| intellectronica | that it's well documented, orgnised in a maintainable fashion, formatted correctly, etc | 15:36 |
| cprov | salgado: that sounds more like an easy r=me than an rs | 15:37 |
| salgado | intellectronica, I'd decide that after reading the cover letter and glancing through the diff | 15:37 |
| intellectronica | salgado: sure, makes sense | 15:37 |
| barry | anyway, shall we move on? | 15:38 |
| barry | [TOPIC] action items | 15:38 |
| MootBot | New Topic: action items | 15:38 |
| salgado | cprov, I don't agree. even when I give an rs=me, I usually glance at the diff | 15:38 |
| barry | * flacoste to look into storm/sqlobject result set compatibility | 15:38 |
| salgado | they're imcompatible | 15:38 |
| salgado | and not meant to be compatible | 15:38 |
| barry | * flacoste to work on API reviewer cheat sheet | 15:39 |
| flacoste | i started | 15:39 |
| flacoste | will finish for next week | 15:39 |
| flacoste | i promise | 15:40 |
| flacoste | although it's not worth much these days | 15:40 |
| barry | flacoste: which one? | 15:40 |
| flacoste | cheat sheet | 15:40 |
| barry | cool | 15:40 |
| danilos | flacoste: because of the economy crisis, or? :) | 15:40 |
| flacoste | the previous is actually on allenap plate | 15:40 |
| salgado | lol | 15:40 |
| 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. | 15:40 |
| barry | flacoste: okay, i'll change that | 15:40 |
| cprov | salgado: (not necessarily disagreeing on what you said, now.) | 15:41 |
| barry | cprov: rs= almost always means "i didn't look at it, i trust you" | 15:41 |
| allenap | flacoste: Do we want to try and update the Storm tree we're using, or shall I back port the IResultSet adapter? | 15:41 |
| flacoste | allenap: better to backport | 15:41 |
| flacoste | allenap: we are looking at updating Storm, but there was a bunch test failures | 15:41 |
| 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? | 15:42 |
| 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) | 15:42 |
| allenap | flacoste: Yep, it merges cleanly, or at least it did last time I tried. | 15:42 |
| danilos | anyway, not the right place for this discussion, I guess | 15:42 |
| barry | yep, we'll keep it on the agenda for next week | 15:43 |
| barry | gary's not here so we'll skip his action item | 15:43 |
| barry | since we're almost out of time, i'll just ask if there's anything quick anybody else has? | 15:44 |
| barry | 5...4...3...2...1 | 15:45 |
| barry | #endmeeting | 15:45 |
| MootBot | Meeting finished at 09:45. | 15:45 |
| barry | thanks everyone! | 15:45 |
| === salgado is now known as salgado-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === mrevell is now known as mrevell-dinner | ||
| === salgado is now known as salgado-afk | ||
| === _thumper_ is now known as thumper | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!