| === 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 | ||
| jtv | wgrant: I'll trade you if you like—https://code.launchpad.net/~jtv/launchpad/recife-test_translatablemessage-cleanup/+merge/40930 | 06:22 |
|---|---|---|
| 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:23 |
| jtv | Forgot that for a moment. | 06:24 |
| wgrant | Heh. | 06:24 |
| wgrant | Maybe soon. | 06:24 |
| jtv | Indeed. | 06:26 |
| jtv | Anyone else for a tiny, tiny review? bac? | 06:27 |
| === 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 | ||
| jtv | danilos: had a flurry of phone calls; getting to your branch now | 10:17 |
| danilos | jtv, if you are busy, we can agree on a general direction and I can find another reviewer | 10:18 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:23 |
| 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:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:32 |
| 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. | 10:33 |
| === 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 | ||
| 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:17 |
| 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 |
| === 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 | ||
| 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:18 |
| 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:19 |
| bigjools | alreet | 11:20 |
| danilos | gmb, heya, I've got one up for review myself: https://code.launchpad.net/~danilo/launchpad/get-current-translation/+merge/40885 | 11:24 |
| gmb | danilos: I'll add it to the queue. Might be a while before I get to it though. | 11:26 |
| === 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 | ||
| danilos | gmb, sure, thanks | 11:32 |
| 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:33 |
| bigjools | gmb: so in that diff, one of the methods is right below | 11:34 |
| bigjools | "_handleStatus_OK" | 11:34 |
| gmb | bigjools: Yep, with you. Ta. | 11:35 |
| 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:08 |
| === 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 | ||
| 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:29 |
| 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:30 |
| 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:31 |
| 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:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| 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:36 |
| 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:37 |
| 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:48 |
| 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:49 |
| 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:50 | |
| bigjools | gmb: thanks matey. And I certainly don't want any blow-up jobs from Translations. | 12:51 |
| === mrevell is now known as mrevell-lunch | ||
| gmb | Arf arf | 12:54 |
| gmb | danilos: I'm going to make a cup of coffee and then I'll take a look at your branch. | 12:55 |
| === 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 | ||
| 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:55 | |
| gmb | ok | 12:56 |
| bigjools | I grabbed some Ardbeg at Gatwick on the way back from Orlando. It's bloody fantastic. | 13:00 |
| 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:01 |
| 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:02 |
| 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:03 |
| 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:04 |
| 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:05 |
| 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:06 |
| 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:07 |
| danilos | henninge, yes | 13:08 |
| danilos | henninge, for most of things, OOo is still being migrated I think | 13:08 |
| 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:09 |
| jtv | (There were some big drops over the past month though) | 13:10 |
| === 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 | ||
| henninge | I just remembered that we initially shared between just two Ubuntu series and only added new ones after that. | 13:12 |
| henninge | jtv: other than that, you can remove those lines. r=me | 13:13 |
| jtv | henninge: great, thanks! | 13:13 |
| === Ursinha-afk is now known as Ursinha | ||
| 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:34 |
| === mrevell-lunch is now known as mrevell | ||
| gmb | danilos: r=me. Thanks to jtv for taking a look first; I wouldn't have picked up on those items :) | 13:55 |
| === 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 | ||
| jtv | gmb: so you don't eat, drink & dream our specialty? How odd! :) | 13:55 |
| 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:56 |
| jtv | gmb: this one → https://code.launchpad.net/~jtv/launchpad/recife-kill-updatetranslation-1/+merge/40948 | 13:57 |
| * gmb looks | 13:59 | |
| jtv | thanks henninge | 14:01 |
| 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:19 |
| 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:20 |
| === Ursinha is now known as Ursinha-lunch | ||
| gmb | jtv: r=me. | 14:36 |
| jtv | splendid, thanks! | 14:36 |
| === 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 | ||
| jtv | That nicely rounds out my evening. | 14:36 |
| gmb | np. | 14:36 |
| === 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 | ||
| jtv | danilos: I think I accidentally moved a kanban card… is your "re-enable global suggestions" still in the right place? | 14:37 |
| danilos | jtv, don't know, I'll check | 14:38 |
| jtv | sorry for the inconv. | 14:38 |
| === 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 | ||
| flacoste | danilos: shouldn't we make global suggestions a feature flag? | 15:35 |
| danilos | flacoste, we should! | 15:35 |
| 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 | 15:36 |
| === 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 | ||
| * gmb fails at IRC | 16:21 | |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!