=== wgrant changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [06:22] wgrant: I'll trade you if you like—https://code.launchpad.net/~jtv/launchpad/recife-test_translatablemessage-cleanup/+merge/40930 [06:23] jtv: If only. [06:23] If only what? [06:23] Ah [06:23] * wgrant is no reviewer. [06:23] Right. [06:24] Forgot that for a moment. [06:24] Heh. [06:24] Maybe soon. [06:26] Indeed. [06:27] Anyone else for a tiny, tiny review? bac? === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [wgrant, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === Guest86531 is now known as jelmer === jtv changed the topic of #launchpad-reviews to: The topic for #launchpad-reviews is: On call: - || Reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:17] danilos: had a flurry of phone calls; getting to your branch now [10:18] jtv, if you are busy, we can agree on a general direction and I can find another reviewer [10:19] jtv, basically, I've asked you since you already looked at it :) [10:19] danilos: why does that sound like "honey if you're too busy to do the dishes, don't worry I can find another wife"? :-) [10:19] jtv, heh, because I wouldn't mind you making better progress on updateTranslation killing spree [10:20] but you already reviewed mine [10:20] jtv, instead of spending time on this review :) [10:20] jtv, that one was slightly easier :) [10:21] jtv, and I also don't mind helping get you unstuck so you can make better pro... :) [10:21] …zac? [10:21] smthng lk that [10:23] oh well—sure, go run off to some other reviewer then. I just want you to be happy. I'll have to re-read the last comments anyway though to see where we stand on mixing "intelligent side-sensitivity" with "passing a specific side." [10:24] jtv, yeah, in short this is what I said: I don't mind removing it, it's mostly useful for tests (where we'll probably have more of these calls, especially in the future) [10:25] jtv, we don't really need the functionality there fwiw, and with or without it we can get all the relevant messages (iow, there's no need to introduce ignore_diverged parameter) [10:25] To be clear, I'm not opposed at all to either of the ways this method can be used—only saying I think we'd be happier if they were separate calls. [10:25] Ah, good to know that that's not an issue. [10:25] jtv, my point is that we'll only use this in one place inside production code, and we won't use auto-detection there [10:26] Only one place? [10:26] jtv, so, such a "separate call with auto-detection" should probably be a test helper if it exists [10:27] jtv, uhm, sorry, a small finite number of places in all of which we won't use auto-detection anyway [10:27] "O(1)"? :-) [10:27] jtv, yes :) [10:28] But the idea was always to replace get{Current,Imported}TranslationMessage with something that auto-detected, wasn't it? [10:28] Or are you saying you want to layer that on top of this one? [10:29] Maybe the auto-detection that's there just made me think that this method was meant to do more than it actually is. [10:29] jtv, that will be layered on top of this one, especially since you'll always have a pofile and potemplate when you are doing these things, and then potemplate.translation_side is a short type away (and even then, we'll want to figure out the side only once) [10:29] Ah now I see. [10:29] jtv, yeah, perhaps :) [10:30] So then I'd just leave the auto-detection out of this method, yes, and just have a thin wrapper with the auto-detection as a separate thing wherever it's useful (test helper or potmsgset) [10:32] jtv, right, I don't mind that at all, and I won't even worry about the helper method until it's actually needed [10:33] Perfect. [10:33] jtv, and I won't worry you about the review then, cheers :) [10:33] Thanks. :-) I'll be done for the day soon. I'll land the one you reviewed, and I'm waiting for test results. I may just have time for that small potmsgset.potemplate fix in the meantime. === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:17] gmb: hey, up for an eye-opening soyuz review? It's onlky 1k lines of diff. :) [11:17] actually when I say soyuz - it's really the buildmaster. [11:18] bigjools: Why not? I've not had my brain dribble out of my ears for a while. [11:18] https://code.launchpad.net/~julian-edwards/launchpad/async-file-uploads-bug-662631/+merge/40883 [11:18] grassy ass === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:18] gmb: it's not that bad actually, most of it's mechanical. If you don't know Twisted you might need me to explain some stuff. [11:19] bigjools: Okay. I'll do this in two passes anyway - one for Spelling, punctuation and grammar and one for actual technical issues, so if I've got questions it'll be later on. [11:20] alreet [11:24] gmb, heya, I've got one up for review myself: https://code.launchpad.net/~danilo/launchpad/get-current-translation/+merge/40885 [11:26] danilos: I'll add it to the queue. Might be a while before I get to it though. === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [danilos] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:32] gmb, sure, thanks [11:33] bigjools: Where does the function "method" come from? Is it a twisted thing or something else (I'm looking at the diff rather than the code here). [11:33] diff line 258, for reference. [11:33] gmb: it uses getattr on the same class [11:33] and appends the status that the builder returns to 'method' [11:33] Ah, right. [11:33] Ta. [11:34] gmb: so in that diff, one of the methods is right below [11:34] "_handleStatus_OK" [11:35] bigjools: Yep, with you. Ta. [12:08] hi gmb! I've got one for you that ties in with the one you're reviewing now. [12:08] sorry, with the one you're reviewing _next_. === jtv changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [danilos, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:29] bigjools: So, I've been over the code and I don't really have any questions beyond what I've asked alread. So, since that generally means that I might have missed something, what did you expect questions about? [12:29] Also, kill me for using "so" at the start of a sentence twice. [12:30] gmb: my main concerns are that I didn't cock up the translations stuff [12:30] I've run it on dogfood and it worked. but ... [12:30] jtv, I've glanced over your branch, nice way to limit damage you inflict :) [12:31] gmb: I had to change the logic around, the code was a little weird. It now reads into files on disk instead of memory. [12:31] bigjools: I'm not convinced that I can answer that question without domain knowledge. Maybe one of the translations team would be able to review that bit for you. [12:31] gmb: so if you see anything that looks like a red flag in their code... [12:32] danilos: this is my chance to return the favor: "it's your fault" :-P You should have implemented the new methods earlier. [12:32] gmb: I actually think it's better looking at it w/o domain knowledge as too much familiarity can be dangerous, IYKWIM [12:32] bigjools: Nothing jumps out at me. I'll give it one more pass to be sure. [12:32] jtv, it is, I admit... but still, changing one updateTranslation call site at a time is probably the way to go [12:32] jtv, so, good job there :) [12:33] gmb: cheers. It should be just a case of following the flow and making sure it's not going to do anything stupid in different conditions. [12:33] danilos: thanks! I'm actually aiming for a conflict with your branch, so as to make it impossible not to remove the XXX—but was slightly too lazy to check if I put mine in the same place as yours. [12:33] gmb: although I'd like to think that you not having many comments means my code's freaking awesome. Sadly, I've been disproved on that many times. [12:34] Nice basis for a review process: "if you have nothing nice to say, keep quiet." :-) [12:34] jtv, yeah, tests will start failing if we don't merge them the right way (otoh, we can merge them in such a way that your method is before mine when tests won't fail, but we'll notice that soon :) [12:35] bigjools, your code is freaking, if that's any consolation [12:35] danilos: I'm not _too_ worried. :) [12:35] ho ho! [12:36] bigjools: Heh. The only bugbear I've got is the repeated use of "d" and "dl" as variable names. But I concede that that's a pretty common pattern with Twisted, so I'm not going to demand that you go and fix them all. [12:37] gmb: yeah, I had the same reaction on my first review of a twisted branch, but it's a *very* strong twisted idiom. [12:48] bigjools: I think the translations stuff looks okay. It's going to be one of them see-if-it-blows-up jobs, I think. [12:48] gmb: such is life with the buildd-manager :/ [12:49] the last cockup happened even though I borrowed production builders on dogfood and ran through a couple of hundred builds. C'est la vie. [12:49] bigjools: Yeah. r=me anyway, with one typo to fix. [12:49] gmb, we are not really looking for any blow-up jobs in translations, thankyouverymuch :) [12:50] danilos: I'll leave the Translations and Soyuz teams to duke it out on that score. [12:50] * danilos resisted the temptation to put "-up" in parenthesis [12:51] gmb: thanks matey. And I certainly don't want any blow-up jobs from Translations. === mrevell is now known as mrevell-lunch [12:54] Arf arf [12:55] danilos: I'm going to make a cup of coffee and then I'll take a look at your branch. === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [danilos, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:55] gmb, make it strong :) [12:55] Oh, gods. [12:55] gmb, j/k [12:55] * bigjools chuckles [12:55] Thank goodness. [12:55] gmb, the background is complicated, the branch is pretty straightforward :) [12:55] * gmb sometimes thinks he should have a bottle of "Review whisky" [12:56] ok [13:00] I grabbed some Ardbeg at Gatwick on the way back from Orlando. It's bloody fantastic. [13:01] bigjools: then you should be in the mood for my "kill 'em all" branch—I don't think the OCR should do that right now. :-) [13:02] 1090 lines removed, no lines added, no lines modified. [13:02] bigjools: are you game for reviewing that? [13:02] jtv: I am heading to lunch and then I have some urgent OOPSes to fix, sorry :( [13:02] :( [13:02] Okay, who else..? [13:03] jtv: does it *need* reviewing? [13:03] land it rs=you [13:03] henninge, do you want to see 1090 lines of dead code go into our trash bin? [13:03] jtv: please, please! ;) [13:03] henninge: thanks! https://code.launchpad.net/~jtv/launchpad/pre-675426/+merge/40953 [13:03] bigjools: r=you, not ts [13:03] blah, not *rs* [13:04] bigjools: the problem with that is that you never know _when_ you're making the silly, impossible mistake that anyone (else) would have spotted. [13:04] At least I don't. [13:04] furry muff [13:05] you're mumbling… did you grow a beard or is that someone else's curly hair around your lips? [13:05] jtv: don't we need "remove-obsolete-translations" any more? [13:06] henninge: if we don't test it, it's broken, so even if we need it we don't have it. [13:06] * jtv had a brief gust of dogmatism there [13:06] henninge, we need remove-translations-by, but the obsolete one, not really [13:07] henninge, apart from the fact that it's most likely broken :) [13:07] Are we sharing amont all Ubuntu series by now? [13:07] among [13:08] henninge, yes [13:08] henninge, for most of things, OOo is still being migrated I think [13:09] oh right, that long-running script ;) [13:09] We haven't seen any noticeable drops in TMs lately, so not sure it's still running. [13:10] (There were some big drops over the past month though) === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: danilos || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:12] I just remembered that we initially shared between just two Ubuntu series and only added new ones after that. [13:13] jtv: other than that, you can remove those lines. r=me [13:13] henninge: great, thanks! === Ursinha-afk is now known as Ursinha [13:34] gmb, I'll go fetch something to eat, I'll be back soon if you've got any questions [13:34] danilos: Okay. going smoothly so far. [13:34] gmb, cool === mrevell-lunch is now known as mrevell [13:55] danilos: r=me. Thanks to jtv for taking a look first; I wouldn't have picked up on those items :) === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:55] gmb: so you don't eat, drink & dream our specialty? How odd! :) [13:56] jtv: I have nightmares about Soyuz, though. [13:56] gmb, then you are excused :) [13:56] gmb: Heh. I actually have 3 waiting for a vote now. henninge approved one of them verbally but didn't vote formally. [13:56] gmb, thanks, btw [13:56] np [13:56] jtv: sorry, got distracted by lunch [13:56] jtv: Okay, so give me a link to the one you want reviewed first. [13:56] henninge: yes, that can distract. :) [13:56] gmb: coming up [13:57] gmb: this one → https://code.launchpad.net/~jtv/launchpad/recife-kill-updatetranslation-1/+merge/40948 [13:59] * gmb looks [14:01] thanks henninge [14:19] jtv: Please add some comments or docstrings to the start of your unit test functions. Other than that, r=me. [14:19] gmb: cool, thanks. I was trying to be self-explanatory but it's hard to judge that when you're already in the know. [14:19] Yeah. [14:20] I usually write the comments first on the basis that I want to read my own code as little as possible. [14:20] jtv: So, is there a second branch for me to look at? [14:20] absolutely! https://code.launchpad.net/~jtv/launchpad/bug-675426/+merge/40958 === Ursinha is now known as Ursinha-lunch [14:36] jtv: r=me. [14:36] splendid, thanks! === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:36] That nicely rounds out my evening. [14:36] np. === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: late lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:37] danilos: I think I accidentally moved a kanban card… is your "re-enable global suggestions" still in the right place? [14:38] jtv, don't know, I'll check [14:38] sorry for the inconv. === matsubara is now known as matsubara-lunch === Ursinha-lunch is now known as Ursinha === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:35] danilos: shouldn't we make global suggestions a feature flag? [15:35] flacoste, we should! [15:36] flacoste, I already had it in the back of my mind (it basically is a feature flag, just implemented through a config option) [15:36] flacoste, unfortunately, there's no room in our backlog === salgado is now known as salgado-physio === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: : === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:21] * gmb fails at IRC === You're now known as ubuntulog === You're now known as ubuntulog_ === You're now known as ubuntulog === matsubara-lunch is now known as matsubara === benji is now known as benji-lunch === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === salgado-physio is now known as salgado === benji-lunch is now known as benji === matsubara is now known as matsubara-afk === bryceh changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === Ursinha is now known as Ursinha-dinner