=== salgado-afk is now known as salgado
MootBotMeeting started at 09:04. The chair is barry.15:04
MootBotCommands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]15:04
barryhello everyone and welcome to this week's ameu reviewers meeting.  who's here today?15:04
barryadeuring: ping15:06
barryallenap: ping15:06
barrybac: ping15:06
barryBjornT: ping15:06
barrycprov: ping15:06
barrymars: ping15:06
barrysinzui: ping15:07
barry[TOPIC] agenda15:07
MootBotNew Topic:  agenda15:07
barry * Roll call15:07
barry * Peanut gallery (anything not on the agenda)15:07
barry * Action items15:07
barry * Mentoring update15: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
barrylet's skip around a bit15:08
barry[TOPIC]  * Code must be used once or more to land (excluding tests) (intellectronica)15:08
MootBotNew Topic:   * Code must be used once or more to land (excluding tests) (intellectronica)15:08
barryintellectronica: the floor is yours15:08
intellectronicathis follows from a discussion we had a couple of weeks ago in the ajax swat team15:09
intellectronicaworking on general purpose solutions before they are being used is risky and hard15:09
intellectronicaand we want to avoid that as much as possible, especially when working on JS code15:09
danilosintellectronica: what if we are not talking about general purpose solutions but parts of solutions?15:09
intellectronicadanilos: same thing15: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
intellectronicaanyway, this is pretty much policy now, and shouldn't be debated here. the only relevant point for reviews is:15:10
daniloswell, I strongly disagree15:10
salgadoso do I15:10
BjornTintellectronica: i think i agree with danilos15:10
BjornTit's ok to land something that isn't used, if you intend to use it in the next branch15:11
BjornTit helps keeping the branches small15:11
danilosBjornT: +115:11
adeuringme too; the rule is in many cases useful, but i think there are many exceptions15:11
intellectronicaBjornT: 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 used15:11
BjornTlanding something that you think will be useful, but don't have any plans to use should of course be discouraged15:11
barryBjornT: i agree, i just wouldn't call that "not used" :)  i.e. it's just an artificial decomposition to keep branches manageable15:11
al-maisandatabase patches are a classic example -- you land them first and use them later15:12
daniloshow 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
intellectronicalazr code, for example, will, by-definition, not be in the same branch as the launchpad code that uses it15:12
cproval-maisan: db-patches are always *used* since the current code has to cope with it.15:13
intellectronicadanilos: 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 risk15:13
barryi 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-maisanI guess we need to agree on what constitutes "use" then15:13
danilosintellectronica: 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
BjornTbarry: right. it's ok to question it, as long as it's allowed to land such branches15:14
danilosbarry: we've done that for ages, but I am willing to hear out intellectronica concrete recommendations for JS15:14
barryBjornT: +115:14
barrydanilos: i'm all for avoiding premature generalizations :)15:15
intellectronicalet'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 high15:15
barryintellectronica: i get that, esp. for js stuff.  you think you see a general pattern when doing some ui stuff before it's proven itself15:16
intellectronicab.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 this15:16
intellectronicawhat we really want to do is think about this pre-implementation15:16
danilosintellectronica: it feels to me reviews are a bit too late to think about this15:16
intellectronicadanilos: i agree15:17
intellectronicabut if this wasn't done before, reviews are our last chance, before unused, and possibly unusable, code, lands15:17
danilosintellectronica: 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 calls15:17
intellectronicadanilos: you can gain something from identifying this in a review. the acknowledgedment that the work is incomplete15:18
intellectronicathe direct result will be that the work can be completed immediately, instead of shelved for a while, which is almost always worse15:18
danilosintellectronica: 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' discretion15:19
barryi actually don't think this is a big departure from current practice, unless intellectronica is saying we MUST reject such branches15:19
danilosbarry: I was under the impression he was, which is why you hear me complaining :)15:19
intellectronicabarry: we only MUST reject lazr-js branches like this. everything else is open for negotiation15:19
barryintellectronica: isn't this why we have the ui= tag now?  <wink>15:20
EdwinGrubbsI 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
danilosI think we are departing from agile development as much as we can, and I don't like that15:20
intellectronicabarry: no. beuno is unlikely to pay attention to this in his review. he reviews the UI itself, not the code or how it's structured15:20
flacostedanilos: hmm, not really, continuous integration is agile15:20
intellectronicadanilos: how so? that sounds very agile to me15:21
danilosfor lazr-js, we should definitely have nicer way to add JS directly into LP tree, and then we wouldn't even be having this discussion15:21
flacostedanilos: and basically what intellectronica is arguing against is landing unintegrated code15:21
flacostedanilos: no, it's not about that15:21
intellectronicaflacoste: exactly!15:21
flacostedanilos: it's about not developping widgets that don't have an integration scenario15:21
danilosflacoste: well, the code is used in the sample page in lazr-js15:21
danilosflacoste: as I said, I see the point, I think the approach to solving it is completely wrong15:22
flacostedanilos: yes, but that's what intellectronica is arguing against, the example page should be developped later, not as the first integration case15:22
intellectronicadanilos: 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 basis15:22
danilosflacoste: in practice, you'll have people develop lazr-js widgets, then having that sit (with small changes) while they fight over integrating it15:22
barryintellectronica: that seems like a reasonable approach to me15:22
intellectronicadanilos: right, and that's exactly what we're trying to avoid15:22
danilosintellectronica: the problem is that it's not clear what the best practice is15:23
intellectronicadanilos: please refer to the email thread, if you haven't read it yet15:23
intellectronicathe best practice is "integration first"15:23
danilosintellectronica: 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 stage15:23
danilosintellectronica: as I said, I agree with that, but don't see how it's relevant to blocking at review level15:23
barryi think you're right that it's better to discuss this policy further on the ml thread15:24
intellectronicai 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 often15:24
danilosi.e. "you *should* have done this completely differently" -- if we get to that point, we are doing something badly15:24
flacostewell, the policy is kind of in place15:24
intellectronicabarry: i agree15:24
flacostereviewers should just be mindful of it15:24
flacosteand remember15:24
flacostereviewers are good candidates for pre-impl call15:25
flacosteso i think that this topic was more about15:25
flacoste"make sure you are up-to-date with the mailing-list thread" :-)15:25
barryflacoste: +115:25
intellectronicaflacoste: +115:25
intellectronicawith that, i think we can move on15:25
barrythanks.  let's move on15:25
barry[TOPIC]  * Avoid refactoring both code and tests in the same branch (intellectronica)15:26
MootBotNew Topic:   * Avoid refactoring both code and tests in the same branch (intellectronica)15:26
intellectronicaah yes, another issue that's not really about reviews :)15:26
daniloswell, I feel this one is, because it's much harder to review stuff that combines refactoring of both15:26
intellectronicadanilos: yes. it came up in a review :)15:27
barryas long as the refactoring isn't necessary to actually keep the test suite happy15:27
danilosbarry: of course15:27
barryor if the refactoring is "minor"15:27
intellectronicaof course15:27
cprovdanilos: will you disagree of anything intellectronica says today ? :)15:27
flacostedanilos is looking for a fight i think ;-)15:27
daniloscprov: you mean agree? :)15:28
* barry wonders who's gonna get voted off the channel today15:28
daniloswell, 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
cprovdanilos: I dunno, you tell me ...15:28
barrydo we need to discuss this topic more?15:29
intellectronicanot as far as i'm concerned15:29
salgadomaybe we could just put up some ads saying that if you (developer) refactor just code, you may get an rs= for some reviewer15:29
danilosanyway, this is a good think to point out, but we should make this a bit more well advertised imo15:29
danilossalgado: ah, sounds really nice!15:29
danilossalgado: +115:29
salgadohowever, if you refactor both code and tests, you'll need a real review that may take much longer15:30
danilosprovided there's existing test coverage for the code15:30
salgadothat way we encourage them to do what's more appropriate15:30
al-maisanstupid question: why are we not to refactor both code and tests in the same branch? Because of the size?15:31
barrysalgado: i'm all for making it easier to land tech debt reduction branches15:31
danilosal-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
barrydanilos: so maybe we need even more scrutiny of test refactoring branches?15:32
salgadoal-maisan, IMO, it makes it harder to find whether or not your test refactoring left any code untested15:32
intellectronicaal-maisan: no, because part of the utility provided by the test suite is confirming that your refactoring is ok15:32
* noodles775 would love to have a refactor-only branch :)15:32
sinzuisalgado: 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 script15:32
danilosbarry: maybe, but I'd be scared of proposing that :)15:32
salgadosinzui, I'd be glad to give you them, but Friday is a holiday here15:33
barrysinzui: i think it's only /not/ a holiday in the us15:33
sinzuisalgado: then I will ask your your rs tomorrow15:33
danilosanyway, I think salgado's idea is a nice one, and we can start offering rs=me for code refactoring only branches15:33
barrydanilos: i'd be okay with that15:34
intellectronicai'm not sure i understand what the difference is, in this case15:34
intellectronicayou're still required to actually review the code, right?15:34
danilosintellectronica: this is just a way of promoting it15:34
barryintellectronica: well, if the tests pass and aren't touched...15:35
danilosintellectronica: I think that's what's missing, telling people how we like our code baked15:35
intellectronicaseems confusing to me, and i don't see the benefits15:35
salgadothe 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 pass15:35
intellectronicasalgado: but there are other aspects of the code you'll want to review, no?15:36
noodles775and it might still be worth a 3rd party review to ensure there is good test coverage in the first place.15:36
danilossalgado: I'd also like to be pointed at existing test for that particular code :)15:36
noodles775danilos: exactly.15:36
salgadointellectronica, depending on the kind of the refactoring, yes15:36
intellectronicathat it's well documented, orgnised in a maintainable fashion, formatted correctly, etc15:36
cprovsalgado: that sounds more like an easy r=me than an rs15:37
salgadointellectronica, I'd decide that after reading the cover letter and glancing through the diff15:37
intellectronicasalgado: sure, makes sense15:37
barryanyway, shall we move on?15:38
barry[TOPIC] action items15:38
MootBotNew Topic:  action items15:38
salgadocprov, I don't agree.  even when I give an rs=me, I usually glance at the diff15:38
barry * flacoste to look into storm/sqlobject result set compatibility15:38
salgadothey're imcompatible15:38
salgadoand not meant to be compatible15:38
barry * flacoste to work on API reviewer cheat sheet15:39
flacostei started15:39
flacostewill finish for next week15:39
flacostei promise15:40
flacostealthough it's not worth much these days15:40
barryflacoste: which one?15:40
flacostecheat sheet15:40
danilosflacoste: because of the economy crisis, or? :)15:40
flacostethe previous is actually on allenap plate15:40
cprovsalgado: 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
barryflacoste: okay, i'll change that15:40
cprovsalgado: (not necessarily disagreeing on what you said, now.)15:41
barrycprov: rs= almost always means "i didn't look at it, i trust you"15:41
allenapflacoste: Do we want to try and update the Storm tree we're using, or shall I back port the IResultSet adapter?15:41
flacosteallenap: better to backport15:41
flacosteallenap: we are looking at updating Storm, but there was a bunch test failures15:41
flacosteallenap: 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
danilosflacoste: 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
allenapflacoste: Yep, it merges cleanly, or at least it did last time I tried.15:42
danilosanyway, not the right place for this discussion, I guess15:42
barryyep, we'll keep it on the agenda for next week15:43
barrygary's not here so we'll skip his action item15:43
barrysince we're almost out of time, i'll just ask if there's anything quick anybody else has?15:44
MootBotMeeting finished at 09:45.15:45
barrythanks 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!