[00:55] thumper: can you review a branch for me? [00:55] it's about 1400 lines [00:55] sure [01:00] thumper: you should have mail shortly [02:30] thumper: having fun yet? [02:30] mwhudson: trying to keep my eyes open [02:31] heh [02:31] mwhudson: just finished going through it all [02:31] it wasn't any more fun to write... [02:31] I think that the code_import_helpers should move out of canonical.launchpad.testing [02:31] _everything_ should move out of canonical.launchpad.testing [02:31] so yeah [02:32] well yeah [02:33] thumper: i have another branch to review too, its much easier... [02:36] ok [02:36] thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/incremental-bzr-svn-imports/+merge/21209 [02:36] I haven't got to any coding this afternoon :-| [02:38] mwhudson: both r=thumper [02:38] thumper: yeah, sorrya bout that :-) [02:38] i'm very glad to get that branch under way though [02:39] it isn't (entirely) your fault [02:39] thanks for the reviews [02:39] :) [02:39] I spent quite a bit of time talking to lifeless === jamalta-afk is now known as jamalta === jamalta is now known as jamalta-afk === jtv1 is now known as jtv === wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [henninge,jelmer,NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [henninge,jelmer,NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [08:40] henninge: where is the branch you want reviewed? === henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [jelmer,NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [08:41] adeuring: gone ;-) [08:41] adeuring: I guess rockstar did a bad job at cleaning out the queue ... [08:41] henninge: no problem [08:43] https://code.launchpad.net/~wgrant/launchpad/get-bpr-by-filename/+merge/21211 is mine. [08:51] wgrant: ok, noted. But let me follow the FIFO principle, so I'll start with jelmer's branch ;) [08:51] adeuring: Right, I was just putting it there for future reference. === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue: [NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === wgrant changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue: [NCommander,wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: NCommander || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:43] jelmer_: review sent [10:08] adeuring: thanks! === jelmer_ is now known as jelmer === noodles785 is now known as noodles775 === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara === salgado-afk is now known as salgado [12:09] adiroiban, you didn't get any email about that bug-531261 branch, did you? [12:19] wgrant: r=me, one suggetsion [12:20] adeuring: I don't think there are any open at the moment. [12:21] wgrant: OK, the I'll start the ec2 test [12:21] adeuring: Thanks. [12:21] wgrant: welcome :) And thanks for all your hard work! === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:24] wgrant, do you have another one in the queue? [12:24] salgado: https://code.edge.launchpad.net/~wgrant/launchpad/anonymous-irc-nicks-and-wiki-names/+merge/21219 [12:24] Nice tiny one. [12:25] salgado: can you review it? I need a lunch break ;) [12:25] adeuring, sure [12:25] Thanks salgado. === salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,wgrant || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:26] adeuring, but I'll have one waiting for you to get back from lunch. :) [12:26] salgado: ok, === mrevell is now known as mrevell-lunch === salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:41] salgado: Thanks. Will you land it? [12:42] wgrant, I'll try, but ec2test is not behaving these days... [12:43] salgado: I guess we'll see. === mrevell-lunch is now known as mrevell === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: salgado,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [13:38] noodles775: I've got some buildfarm fixes, some of them generic (based gratefully on discussion with wgrant) that you might like to review! https://code.launchpad.net/~jtv/launchpad/bug-537866/+merge/21238 [13:40] BjornT: Do you think you'll have some time to revisit my isolate-tests branch. I think it's in pretty good shape now. [13:40] jtv: I'm certainly keen to look through it, but probably over the weekend. It might be best to get the OCRs to review it, and I'll take a look later, if that's ok. [13:41] noodles775: sure, I'll try that [13:41] jtv: btw, there's a tiny conflict there too. [13:41] adeuring, salgado: either you free for a buildfarm branch? https://code.launchpad.net/~jtv/launchpad/bug-537866/+merge/21238 [13:42] grr [13:42] thanks for spotting that [13:42] jtv: sure [13:42] adeuring: michael just pointed out a small conflict in a block of imports; take it as read that those will be fixed. [13:43] adeuring, I can take jtv's so that you can take mine; that ok with you? [13:43] salgado: sure :) [13:43] BTW, I'm already reading you changes [13:43] cool [13:57] allenap: yes, i think i'll have time today [13:58] BjornT: Thanks. Do you also have 30s to approve a db query in LPS please? I need to run it ASAP. === salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: salgado,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:04] salgado: r=me [14:05] thanks adeuring === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:06] allenap: why is the query needed? won't checkwatches choose a subset of bug watches to update anyway? [14:08] BjornT: It will, but there are 15k watches, there have been problems with batching before, and I'm not 100% sure they're all solved (I think they are, but every time I've thought that something else happens). I just want to be safer than sorry. [14:10] allenap: you probably shouldn't update the ones that are NULL, though, should you? [14:11] BjornT: There are probably a lot of those because this tracker has been disabled for weeks, but I guess not. I'll update the query. [14:16] allenap: thanks for the review this morning [14:17] NCommander: Did you mean someone else? [14:17] allenap: er, whopos, autocomplete got the wrong person :-/ [14:17] * NCommander goes to find his coffee pop [14:18] :) [14:18] *pot [14:18] wow [14:18] adeuring: thanks for the review this morning [14:18] NCommander: welcome :) [14:19] NCommander: plase ping me when/if BjornT and stub approved the branch, then I'll land it [14:19] adeuring: Thank you, I was wondering if you could help me understand something first though: I'm not very knowledgable about LIbrarian ATM, how much of a hit is it to query librarian for a file then parse its data directly in a UI method? [14:20] abentley: the other thing that was discussed was the possibility of moving the changelogs to a new table then enforcing a forgein key restriction (and then possibly moving copyright files at some point in the future) [14:20] NCommander: I have no real clue. But IIRC the Soyuz folks did something like this [14:21] NCommander: yes, having another table would address most of BjornT's concerns, sounds like a good idea [14:21] NCommander, you mean me? [14:21] abentley: I think he meant me [14:22] * NCommander isn't so sure anymore [14:22] I'm welcome to accept all opinions :-) === jamalta-afk is now known as jamalta [14:27] NCommander: Can the stuff the needs to be displayed from changlog change without a change in the changelog data? [14:28] garrr... that's unreadable... let me try to avoid the repetiton of "change".... [14:29] NCommander: can it happen that different things from the changelog must be displayed without any modification in the changelog data? If not, the parsed data could be stored instead in the DB [14:31] adeuring: that's what we currently do (sorta, we pull changelog exerts from the .changes file). [14:31] ok... [15:06] salgado: sorry for the delay. No, no email. Most probably some tests are failing, but unfortunalty I can not run the full LP test suite on my computer [15:07] adiroiban, yeah, I'm just running the full test suite now. once it finishes I'll check if anything failed and let you know [15:08] salgado: thanks! [15:42] danilos: can you please take a quick look at this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 [15:42] it is not ready yet [15:43] but I would like to know your comments for the potmsgset.resetCurrentTranslation() [15:44] adiroiban, sure, I'll take a look in a bit === salgado is now known as salgado-lunch [15:46] BjornT: would using a separate table for changelog information help with your concerns on increased memory usage? [16:00] salgado-lunch: yup, "make lint" catches that bad character that got in... it must have happened when I thought I was done and closed the window. :-( [16:01] huh wait, there's more lint now... wtf? [16:01] adiroiban, hey, looking at your branch now === matsubara is now known as matsubara-lunch [16:01] adiroiban, updateTranslations is off limits: no changes allowed there ;) [16:02] adiroiban, and date_created should probably not be modified [16:02] danilos: a I agree with date_created [16:03] but I don't know how to modify a potmsgset to be listed again as needs review [16:03] adiroiban, logic to determine which method to call should be in the browser code [16:03] adiroiban, potmsgset shouldn't be modified at all [16:04] adiroiban, it's a TM you are modifying, and https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/13 gives you a suggestion [16:04] Bug #201749: Impossible to mark as needing review strings already translated or with suggestions [16:04] danilos: ok I will move the logic to browser code [16:04] adiroiban, and now, on to the resetCurrentTranslation method :) [16:07] adiroiban, the way you do it won't do: we don't have the luxury of fetching a bunch of messages for every single message on +translate page [16:07] adiroiban, you need to get only the matching message if it exists [16:08] ok. but the logic is ok ? [16:08] adiroiban, and the thing you are doing there is not what you should be doing [16:09] adiroiban, no, sorry [16:09] ok [16:09] I still don't know how diverged messages shoudl be handled [16:10] adiroiban, basically, your code does this: "current, diverged translation 'blah' needs review, let's find all other diverged translations 'blah' and make them 'shared' [16:12] and what it should do? [16:12] adiroiban, well, the only thing code should do is find an identical (to current) suggestion (meaning .is_current=False, .potemplate=None) if it exists, and then only discard the older one [16:12] adiroiban, let me prepare an example [16:15] adiroiban, http://paste.ubuntu.com/394099/ [16:15] adiroiban, fwiw, removing a message is impossible from web UI anyway, so you really can't fix this bug properly [16:17] adiroiban, another example to be wary of: http://paste.ubuntu.com/394100/ [16:18] how we should proceed with the removal of a message? [16:30] adiroiban, well, the best way would be to somehow tag it an then remove it later [16:31] danilos: and when tag it... we should tag it in a wait it will not be selected by any other existing query [16:31] adiroiban, what do you mean? [16:32] adiroiban, perhaps we'll have to allow removal from the web UI as well, though that sounds very, very bad [16:32] adiroiban, there's also the transfer of all the flags, like is_imported, from the deleted message to the preserved one [16:32] danilos: i guess there are many SELECT queries that will need to be modified to take into acount the tag [16:33] adiroiban, yeah, it'd end up being a pretty big change, with many performance implications [16:34] adiroiban, as such, I wouldn't tackle this before many other things are cleaned up first [16:35] danilos: but this is a big issues if someone is using Launchpad translations for reviewing or doing QA related tasks [16:36] adiroiban, I am not disagreeing, I am just saying that it'll take a lot of work to get this done properly; or, we can ignore some of the issues and live with some problems we introduce, but we've got to be aware of them first [16:37] adiroiban, for example, perhaps it's not a big deal if we've got duplicate identical suggestions in the DB, so let's not block on that [16:38] danilos: for the duplicate suggestions [16:38] adiroiban, we'll get some weird behaviour occasionally, but let's say they are of lower priority [16:38] maybe we can have a cron job that will do the cleaning [16:38] adiroiban, now, there's still some things to worry about [16:38] adiroiban, maybe, but that'll be very complex as well [16:39] danilos: what are are the other issues? [16:39] adiroiban, and it won't stop any weirdness [16:40] adiroiban, the other issue is that we need to carefully consider what will happen when somebody asks a diverged translation to be reviewed, and there is an existing shared one [16:41] adiroiban, i.e. http://paste.ubuntu.com/394130/ [16:41] adiroiban, also, note that you can't unset potemplate if is_imported is true either [16:43] danilos: for lates example, I think result 1 is fine [16:44] and then another reviewer can decide if the shared suggestion is better [16:44] or it will need a new diverged translation [16:45] adiroiban, right [16:46] adiroiban, ok, so, then it won't be that complicated [16:46] danilos: what are the consequences of setting potemplate = None for a msgset with is_imported = True ? [16:47] adiroiban, basically, we'll have to watch if current message is diverged or not; if it is, and it isn't is_imported, then you unset is_current and .potemplate and how you don't hit a DB constraint :) [16:47] adiroiban, it won't be diverged is_imported message, and if there's another shared is_imported message, you'd be in for a ride [16:47] adiroiban, basically, many places where only one is expected would assert/traceback [16:48] adiroiban, provided you don't hit a DB constraint first (I am not sure if we have one like that) [16:48] danilos: I see [16:50] danilos: ok. thanks. one more question regarding potmsgset.updateTranslation() [16:50] adiroiban, sure (though, you still can't touch it: nobody can, functionality needs to be slowly factored out of it :) [16:50] should I add a new bug for the case when you tick „needs” for a already reviewd translation? [16:51] or we already have a bug for that? [16:51] adiroiban, what do you mean? [16:51] if you add a new suggestion [16:51] adiroiban, ok? [16:51] and you tick „needs review” [16:51] it will be listed as a new suggestion [16:51] adiroiban, right, that's how it should work [16:52] but if in fact it is not a new suggesion [16:52] salgado-lunch: I've fixed up the weirdness in my branch. I'm going to reboot now, hoping that that'll fix it. [16:52] and _findTranslationMessage [16:52] finds that this new suggestion [16:52] adiroiban, ah, right, it's an already discarded suggestion [16:52] is in fact an already existing suggestion [16:52] it will not mark it as needs review [16:52] this is why I touched the updateTranslation code [16:53] adiroiban, I am pretty sure we have a bug about that as well [16:53] ok [16:54] adiroiban, if you ever want to fix that, you'll have to provide a new method addASuggestion() or something, similar to what you did for this bug, and move the functionality out of updateTranslation method and add new functionality you want :) [16:54] adiroiban, and again, moved the logic to decide what to do out into browser code [16:54] danilos: yes. I will move that logic. [16:54] s/moved/move [16:55] that was the initial implimentation [16:55] but then I didn't know where to put the tests [16:55] for that code [16:55] adiroiban, right, I am talking about the other bug where discared suggestions don't show up as suggestions afterwards [16:55] danilos: ah. ok... do you have any suggestion for where should I put the integration tests for resetCurrentTranslation ? [16:56] lib/lp/translations/browser/tests/translationmessage-views.txt ? [16:56] I am very puzzled by the way unit tests and integration tests are structured [16:56] adiroiban, integration test? that would probably be very tricky, I'd look for existing needs review tests [16:57] adiroiban, good unit tests are most important here; single integration test is good enough [16:57] danilos: lib/lp/translations/tests/potmsgset-update-translation.txt [16:57] adiroiban, the problem is that all these translationmessage and pofile views are a mess, just like updateTranslation is: there is no clear separation of anything [16:58] danilos: ok [16:58] thanks! [16:58] adiroiban, that'd be for documentation on updateTranslation method, do it in potmsgset.txt with a single example for how to use resetCurrentTranslation (and please make it accept identical parameters as the rest of the methods: i.e. don't use pofile if other methods are passing potemplate, language and variant) [16:59] I will try to fix the current problems and come back with more questions [16:59] adiroiban, sure, I am likely to be out, but I can probably find some time over the weekend (email me :) [17:00] danilos: no emails for the weekend... and I don't know if I will have time over the weekend [17:00] adiroiban, heh, ok :) [17:01] adiroiban, anyway, to be honest, this should probably be a new checkbox closer to the current translation; hopefully redesign of +translate views will fix all this [17:02] danilos: I agree, as this is not „needs review” , but reset reviewed state === deryck is now known as deryck[lunch] [17:03] danilos: did you have time to look over the POTemplate API ? === matsubara-lunch is now known as matsubara === salgado-lunch is now known as salgado [17:10] adiroiban, did you get a failure email this time? [17:12] salgado: yes. thanks [17:14] salgado: are the same windmill tests that on my computer are also failing on mainline [17:14] adiroiban, are you sure your mainline branch is pristine? [17:15] because these tests don't fail in other branches I've submitted to ec2 [17:15] e.g. the one I submitted for wgrant which is on PQM right now [17:16] salgado: yes , the mainline is pristine... but my computer is slow and all waits.forElemets are exceded [17:16] but most probably, my changes are introducing more delays [17:17] and this is why those tests are failing on ec2 [17:17] could be [17:17] and they are not failing all the time [17:18] on my computer, there are 50% chances of a windmill test to pass [17:18] and this was since the beginning of my hacking on launchpad [17:28] adiroiban, sorry, I don't remember seeing anything about the potemplate API, where was that? [17:29] danilos: hm... I have added them as comment to the bug [17:29] and I remember sending an notification email to you, henning and jtv [17:30] the subject was POTemplate Details API [17:30] https://bugs.edge.launchpad.net/rosetta/+bug/525371 [17:30] Bug #525371: API for reading POTemplates details === deryck[lunch] is now known as deryck === gary_poster is now known as gary-lunch [18:47] salgado, care to look at a branch really quick? [18:47] rockstar, sure, where's it? [18:47] https://code.edge.launchpad.net/~rockstar/launchpad/recipe-canonical_url/+merge/21271 [18:47] It's really little. [18:49] rockstar, I wonder if isn't there a better place for that test... [18:49] also, you don't really need to add that empty file, do you? [18:49] not really empty, but with no actual code [18:49] salgado, I do because abentley and I both need it very soon, and we wanted to avoid conflicts. [18:50] oh, ok. can you s/2009/2010 there, then? [18:50] Huh, I thought I did... [18:51] rockstar, how about moving the test to ./lib/lp/code/tests/test_sourcepackagerecipe.py ? [18:51] salgado, according to https://dev.launchpad.net/Web/URLTraversal that's where the test should go. I originally had a unit test there. [18:52] oh, didn't know about that [18:53] r=me === kfogel is now known as jimb_intestines === jimb_intestines is now known as kfogel [18:53] salgado, cheers! [18:58] salgado: I fixed those weird problems with my branch, but now I found an unintended consequence: too much is working now, meaning that a doctest is actually trying to talk to build-farm slaves, which of course we can't do in tests. I'm fixing that up now. [19:05] jtv, ok, I've just sent you a second round of feedback [19:05] thanks === gary-lunch is now known as gary_poster [19:59] jtv, will you let me know once you're done with that branch? [19:59] salgado: yes === jamalta is now known as jamalta-afk === jamalta-afk is now known as jamalta [20:24] salgado: over to you. It's deep night here, so I won't be around for much longer [20:24] whoops, maybe I should push those changes first :) [20:29] jtv, that's a good idea. :) [20:29] salgado: it's taking a while for the diff to update... [20:30] salgado: updated now. [20:31] cool === salgado changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-afk [21:25] salgado: thanks! === salgado is now known as salgado-afk [21:50] Anybody around for a review of https://code.edge.launchpad.net/~wgrant/launchpad/bprdc/+merge/21213? === wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === jamalta is now known as jamalta-afk