[06:22] <jtv> wgrant: I'll trade you if you like—https://code.launchpad.net/~jtv/launchpad/recife-test_translatablemessage-cleanup/+merge/40930
[06:23] <wgrant> jtv: If only.
[06:23] <jtv> If only what?
[06:23] <jtv> Ah
[06:23]  * wgrant is no reviewer.
[06:23] <jtv> Right.
[06:24] <jtv> Forgot that for a moment.
[06:24] <wgrant> Heh.
[06:24] <wgrant> Maybe soon.
[06:26] <jtv> Indeed.
[06:27] <jtv> Anyone else for a tiny, tiny review?  bac?
[10:17] <jtv> danilos: had a flurry of phone calls; getting to your branch now
[10:18] <danilos> jtv, if you are busy, we can agree on a general direction and I can find another reviewer
[10:19] <danilos> jtv, basically, I've asked you since you already looked at it :)
[10:19] <jtv> 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] <danilos> jtv, heh, because I wouldn't mind you making better progress on updateTranslation killing spree
[10:20] <jtv> but you already reviewed mine
[10:20] <danilos> jtv, instead of spending time on this review :)
[10:20] <danilos> jtv, that one was slightly easier :)
[10:21] <danilos> jtv, and I also don't mind helping get you unstuck so you can make better pro... :)
[10:21] <jtv> …zac?
[10:21] <danilos> smthng lk that
[10:23] <jtv> 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] <danilos> 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] <danilos> 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] <jtv> 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] <jtv> Ah, good to know that that's not an issue.
[10:25] <danilos> 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] <jtv> Only one place?
[10:26] <danilos> jtv, so, such a "separate call with auto-detection" should probably be a test helper if it exists
[10:27] <danilos> jtv, uhm, sorry, a small finite number of places in all of which we won't use auto-detection anyway
[10:27] <jtv> "O(1)"?  :-)
[10:27] <danilos> jtv, yes :)
[10:28] <jtv> But the idea was always to replace get{Current,Imported}TranslationMessage with something that auto-detected, wasn't it?
[10:28] <jtv> Or are you saying you want to layer that on top of this one?
[10:29] <jtv> 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] <danilos> 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] <jtv> Ah now I see.
[10:29] <danilos> jtv, yeah, perhaps :)
[10:30] <jtv> 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] <danilos> 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] <jtv> Perfect.
[10:33] <danilos> jtv, and I won't worry you about the review then, cheers :)
[10:33] <jtv> 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.
[11:17] <bigjools> gmb: hey, up for an eye-opening soyuz review?  It's onlky 1k lines of diff. :)
[11:17] <bigjools> actually when I say soyuz - it's really the buildmaster.
[11:18] <gmb> bigjools: Why not? I've not had my brain dribble out of my ears for a while.
[11:18] <bigjools> https://code.launchpad.net/~julian-edwards/launchpad/async-file-uploads-bug-662631/+merge/40883
[11:18] <bigjools> grassy ass
[11:18] <bigjools> 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] <gmb> 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] <bigjools> alreet
[11:24] <danilos> gmb, heya, I've got one up for review myself: https://code.launchpad.net/~danilo/launchpad/get-current-translation/+merge/40885
[11:26] <gmb> danilos: I'll add it to the queue. Might be a while before I get to it though.
[11:32] <danilos> gmb, sure, thanks
[11:33] <gmb> 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] <gmb> diff line 258, for reference.
[11:33] <bigjools> gmb: it uses getattr on the same class
[11:33] <bigjools> and appends the status that the builder returns to 'method'
[11:33] <gmb> Ah, right.
[11:33] <gmb> Ta.
[11:34] <bigjools> gmb: so in that diff, one of the methods is right below
[11:34] <bigjools> "_handleStatus_OK"
[11:35] <gmb> bigjools: Yep, with you. Ta.
[12:08] <jtv> hi gmb!  I've got one for you that ties in with the one you're reviewing now.
[12:08] <jtv> sorry, with the one you're reviewing _next_.
[12:29] <gmb> 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] <gmb> Also, kill me for using "so" at the start of a sentence twice.
[12:30] <bigjools> gmb: my main concerns are that I didn't cock up the translations stuff
[12:30] <bigjools> I've run it on dogfood and it worked. but ...
[12:30] <danilos> jtv, I've glanced over your branch, nice way to limit damage you inflict :)
[12:31] <bigjools> 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] <gmb> 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] <bigjools> gmb: so if you see anything that looks like a red flag in their code...
[12:32] <jtv> danilos: this is my chance to return the favor: "it's your fault"  :-P  You should have implemented the new methods earlier.
[12:32] <bigjools> gmb: I actually think it's better looking at it w/o domain knowledge as too much familiarity can be dangerous, IYKWIM
[12:32] <gmb> bigjools: Nothing jumps out at me. I'll give it one more pass to be sure.
[12:32] <danilos> jtv, it is, I admit... but still, changing one updateTranslation call site at a time is probably the way to go
[12:32] <danilos> jtv, so, good job there :)
[12:33] <bigjools> 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] <jtv> 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] <bigjools> 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] <jtv> Nice basis for a review process: "if you have nothing nice to say, keep quiet."  :-)
[12:34] <danilos> 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] <danilos> bigjools, your code is freaking, if that's any consolation
[12:35] <jtv> danilos: I'm not _too_ worried. :)
[12:35] <bigjools> ho ho!
[12:36] <gmb> 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] <bigjools> 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] <gmb> 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] <bigjools> gmb: such is life with the buildd-manager :/
[12:49] <bigjools> 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] <gmb> bigjools: Yeah. r=me anyway, with one typo to fix.
[12:49] <danilos> gmb, we are not really looking for any blow-up jobs in translations, thankyouverymuch :)
[12:50] <gmb> 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] <bigjools> gmb: thanks matey.  And I certainly don't want any blow-up jobs from Translations.
[12:54] <gmb> Arf arf
[12:55] <gmb> danilos: I'm going to make a cup of coffee and then I'll take a look at your branch.
[12:55] <danilos> gmb, make it strong :)
[12:55] <gmb> Oh, gods.
[12:55] <danilos> gmb, j/k
[12:55]  * bigjools chuckles
[12:55] <gmb> Thank goodness.
[12:55] <danilos> 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] <gmb> ok
[13:00] <bigjools> I grabbed some Ardbeg at Gatwick on the way back from Orlando.  It's bloody fantastic.
[13:01] <jtv> 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] <jtv> 1090 lines removed, no lines added, no lines modified.
[13:02] <jtv> bigjools: are you game for reviewing that?
[13:02] <bigjools> jtv: I am heading to lunch and then I have some urgent OOPSes to fix, sorry :(
[13:02] <jtv> :(
[13:02] <jtv> Okay, who else..?
[13:03] <bigjools> jtv: does it *need* reviewing?
[13:03] <bigjools> land it rs=you
[13:03] <jtv> henninge, do you want to see 1090 lines of dead code go into our trash bin?
[13:03] <henninge> jtv: please, please! ;)
[13:03] <jtv> henninge: thanks!  https://code.launchpad.net/~jtv/launchpad/pre-675426/+merge/40953
[13:03] <lifeless> bigjools: r=you, not ts
[13:03] <lifeless> blah, not *rs*
[13:04] <jtv> 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] <jtv> At least I don't.
[13:04] <bigjools> furry muff
[13:05] <jtv> you're mumbling… did you grow a beard or is that someone else's curly hair around your lips?
[13:05] <henninge> jtv: don't we need "remove-obsolete-translations" any more?
[13:06] <jtv> 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] <danilos> henninge, we need remove-translations-by, but the obsolete one, not really
[13:07] <danilos> henninge, apart from the fact that it's most likely broken :)
[13:07] <henninge> Are we sharing amont all Ubuntu series by now?
[13:07] <henninge> among
[13:08] <danilos> henninge, yes
[13:08] <danilos> henninge, for most of things, OOo is still being migrated I think
[13:09] <henninge> oh right, that long-running script ;)
[13:09] <jtv> We haven't seen any noticeable drops in TMs lately, so not sure it's still running.
[13:10] <jtv> (There were some big drops over the past month though)
[13:12] <henninge> I just remembered that we initially shared  between just two Ubuntu series and only added new ones after that.
[13:13] <henninge> jtv: other than that, you can remove those lines. r=me
[13:13] <jtv> henninge: great, thanks!
[13:34] <danilos> gmb, I'll go fetch something to eat, I'll be back soon if you've got any questions
[13:34] <gmb> danilos: Okay. going smoothly so far.
[13:34] <danilos> gmb, cool
[13:55] <gmb> danilos: r=me. Thanks to jtv for taking a look first; I wouldn't have picked up on those items :)
[13:55] <jtv> gmb: so you don't eat, drink & dream our specialty?  How odd!  :)
[13:56] <gmb> jtv: I have nightmares about Soyuz, though.
[13:56] <danilos> gmb, then you are excused :)
[13:56] <jtv> gmb: Heh.  I actually have 3 waiting for a vote now.  henninge approved one of them verbally but didn't vote formally.
[13:56] <danilos> gmb, thanks, btw
[13:56] <gmb> np
[13:56] <henninge> jtv: sorry, got distracted by lunch
[13:56] <gmb> jtv: Okay, so give me a link to the one you want reviewed first.
[13:56] <jtv> henninge: yes, that can distract.  :)
[13:56] <jtv> gmb: coming up
[13:57] <jtv> gmb: this one → https://code.launchpad.net/~jtv/launchpad/recife-kill-updatetranslation-1/+merge/40948
[13:59]  * gmb looks
[14:01] <jtv> thanks henninge
[14:19] <gmb> jtv: Please add some comments or docstrings to the start of your unit test functions. Other than that, r=me.
[14:19] <jtv> 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] <gmb> Yeah.
[14:20] <gmb> I usually write the comments first on the basis that I want to read my own code as little as possible.
[14:20] <gmb> jtv: So, is there a second branch for me to look at?
[14:20] <jtv> absolutely!  https://code.launchpad.net/~jtv/launchpad/bug-675426/+merge/40958
[14:36] <gmb> jtv: r=me.
[14:36] <jtv> splendid, thanks!
[14:36] <jtv> That nicely rounds out my evening.
[14:36] <gmb> np.
[14:37] <jtv> danilos: I think I accidentally moved a kanban card… is your "re-enable global suggestions" still in the right place?
[14:38] <danilos> jtv, don't know, I'll check
[14:38] <jtv> sorry for the inconv.
[15:35] <flacoste> danilos: shouldn't we make global suggestions a feature flag?
[15:35] <danilos> flacoste, we should!
[15:36] <danilos> 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] <danilos> flacoste, unfortunately, there's no room in our backlog
[16:21]  * gmb fails at IRC