/srv/irclogs.ubuntu.com/2010/03/12/#launchpad-reviews.txt

mwhudsonthumper: can you review a branch for me?00:55
mwhudsonit's about 1400 lines00:55
thumpersure00:55
mwhudsonthumper: you should have mail shortly01:00
mwhudsonthumper: having fun yet?02:30
thumpermwhudson: trying to keep my eyes open02:30
mwhudsonheh02:31
thumpermwhudson: just finished going through it all02:31
mwhudsonit wasn't any more fun to write...02:31
thumperI think that the code_import_helpers should move out of canonical.launchpad.testing02:31
mwhudson_everything_ should move out of canonical.launchpad.testing02:31
mwhudsonso yeah02:31
thumperwell yeah02:32
mwhudsonthumper: i have another branch to review too, its much easier...02:33
thumperok02:36
mwhudsonthumper: https://code.edge.launchpad.net/~mwhudson/launchpad/incremental-bzr-svn-imports/+merge/2120902:36
thumperI haven't got to any coding this afternoon :-|02:36
thumpermwhudson: both r=thumper02:38
mwhudsonthumper: yeah, sorrya bout that :-)02:38
mwhudsoni'm very glad to get that branch under way though02:38
thumperit isn't (entirely) your fault02:39
mwhudsonthanks for the reviews02:39
thumper:)02:39
thumperI spent quite a bit of time talking to lifeless02:39
=== 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
adeuringhenninge: where is the branch you want reviewed?08:40
=== 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
henningeadeuring: gone ;-)08:41
henningeadeuring: I guess rockstar did a bad job at cleaning out the queue ...08:41
adeuringhenninge: no problem08:41
wgranthttps://code.launchpad.net/~wgrant/launchpad/get-bpr-by-filename/+merge/21211 is mine.08:43
adeuringwgrant: ok, noted. But let me follow the FIFO principle, so I'll start with jelmer's branch ;)08:51
wgrantadeuring: Right, I was just putting it there for future reference.08:51
=== 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
adeuringjelmer_: review sent09:43
jelmer_adeuring: thanks!10:08
=== 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
salgadoadiroiban, you didn't get any email about that bug-531261 branch, did you?12:09
adeuringwgrant: r=me, one suggetsion12:19
wgrantadeuring: I don't think there are any open at the moment.12:20
adeuringwgrant: OK, the I'll start the ec2 test12:21
wgrantadeuring: Thanks.12:21
adeuringwgrant: welcome :) And thanks for all your hard work!12:21
=== 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
salgadowgrant, do you have another one in the queue?12:24
wgrantsalgado: https://code.edge.launchpad.net/~wgrant/launchpad/anonymous-irc-nicks-and-wiki-names/+merge/2121912:24
wgrantNice tiny one.12:24
adeuringsalgado: can you review it? I need a lunch break ;)12:25
salgadoadeuring, sure12:25
wgrantThanks salgado.12:25
=== 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
salgadoadeuring, but I'll have one waiting for you to get back from lunch. :)12:26
adeuringsalgado: ok,12:26
=== 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
wgrantsalgado: Thanks. Will you land it?12:41
salgadowgrant, I'll try, but ec2test is not behaving these days...12:42
wgrantsalgado: I guess we'll see.12:43
=== 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
jtvnoodles775: 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/2123813:38
allenapBjornT: 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
noodles775jtv: 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:40
jtvnoodles775: sure, I'll try that13:41
noodles775jtv: btw, there's a tiny conflict there too.13:41
jtvadeuring, salgado: either you free for a buildfarm branch?  https://code.launchpad.net/~jtv/launchpad/bug-537866/+merge/2123813:41
jtvgrr13:42
jtvthanks for spotting that13:42
adeuringjtv: sure13:42
jtvadeuring: michael just pointed out a small conflict in a block of imports; take it as read that those will be fixed.13:42
salgadoadeuring, I can take jtv's so that you can take mine; that ok with you?13:43
adeuringsalgado: sure :)13:43
adeuringBTW, I'm already reading you changes13:43
jtvcool13:43
BjornTallenap: yes, i think i'll have time today13:57
allenapBjornT: Thanks. Do you also have 30s to approve a db query in LPS please? I need to run it ASAP.13:58
=== 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
adeuringsalgado: r=me14:04
salgadothanks adeuring14:05
=== 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
BjornTallenap: why is the query needed? won't checkwatches choose a subset of bug watches to update anyway?14:06
allenapBjornT: 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:08
BjornTallenap: you probably shouldn't update the ones that are NULL, though, should you?14:10
allenapBjornT: 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:11
NCommanderallenap: thanks for the review this morning14:16
allenapNCommander: Did you mean someone else?14:17
NCommanderallenap: er, whopos, autocomplete got the wrong person :-/14:17
* NCommander goes to find his coffee pop14:17
allenap:)14:18
NCommander*pot14:18
NCommanderwow14:18
NCommanderadeuring: thanks for the review this morning14:18
adeuringNCommander: welcome :)14:18
adeuringNCommander: plase ping me when/if BjornT and stub approved the branch, then I'll land it14:19
NCommanderadeuring: 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:19
NCommanderabentley: 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
adeuringNCommander: I have no real clue. But IIRC the Soyuz folks did something like this14:20
adeuringNCommander: yes, having another table would address most of BjornT's  concerns, sounds like a good idea14:21
abentleyNCommander, you mean me?14:21
adeuringabentley: I think he meant me14:21
* NCommander isn't so sure anymore14:22
NCommanderI'm welcome to accept all opinions :-)14:22
=== jamalta-afk is now known as jamalta
adeuringNCommander: Can the stuff the needs to be displayed from changlog change without a change in the changelog data?14:27
adeuringgarrr... that's unreadable... let me try to avoid the repetiton of "change"....14:28
adeuringNCommander: 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 DB14:29
NCommanderadeuring: that's what we currently do (sorta, we pull changelog exerts from the .changes file).14:31
adeuringok...14:31
adiroibansalgado: 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 computer15:06
salgadoadiroiban, yeah, I'm just running the full test suite now.  once it finishes I'll check if anything failed and let you know15:07
adiroibansalgado: thanks!15:08
adiroibandanilos: can you please take a quick look at this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/2125015:42
adiroibanit is not ready yet15:42
adiroibanbut I would like to know your comments for the potmsgset.resetCurrentTranslation()15:43
danilosadiroiban, sure, I'll take a look in a bit15:44
=== salgado is now known as salgado-lunch
NCommanderBjornT: would using a separate table for changelog information help with your concerns on increased memory usage?15:46
jtvsalgado-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:00
jtvhuh wait, there's more lint now... wtf?16:01
danilosadiroiban, hey, looking at your branch now16:01
=== matsubara is now known as matsubara-lunch
danilosadiroiban, updateTranslations is off limits: no changes allowed there ;)16:01
danilosadiroiban, and date_created should probably not be modified16:02
adiroibandanilos: a I agree with date_created16:02
adiroibanbut I don't know how to modify a potmsgset to be listed again as needs review16:03
danilosadiroiban, logic to determine which method to call should be in the browser code16:03
danilosadiroiban, potmsgset shouldn't be modified at all16:03
danilosadiroiban, it's a TM you are modifying, and https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/13 gives you a suggestion16:04
mupBug #201749: Impossible to mark as needing review strings already translated or with suggestions <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/201749>16:04
adiroibandanilos: ok I will move the logic to browser code16:04
danilosadiroiban, and now, on to the resetCurrentTranslation method :)16:04
danilosadiroiban, 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 page16:07
danilosadiroiban, you need to get only the matching message if it exists16:07
adiroibanok. but the logic is ok ?16:08
danilosadiroiban, and the thing you are doing there is not what you should be doing16:08
danilosadiroiban, no, sorry16:09
adiroibanok16:09
adiroibanI still don't know how diverged messages shoudl be handled16:09
danilosadiroiban, basically, your code does this: "current, diverged translation 'blah' needs review, let's find all other diverged translations 'blah' and make them 'shared'16:10
adiroibanand what it should do?16:12
danilosadiroiban, 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 one16:12
danilosadiroiban, let me prepare an example16:12
danilosadiroiban, http://paste.ubuntu.com/394099/16:15
danilosadiroiban, fwiw, removing a message is impossible from web UI anyway, so you really can't fix this bug properly16:15
danilosadiroiban, another example to be wary of: http://paste.ubuntu.com/394100/16:17
adiroibanhow we should proceed with the removal of a message?16:18
danilosadiroiban, well, the best way would be to somehow tag it an then remove it later16:30
adiroibandanilos: and when tag it... we should tag it in a wait it will not be selected by any other existing query16:31
danilosadiroiban, what do you mean?16:31
danilosadiroiban, perhaps we'll have to allow removal from the web UI as well, though that sounds very, very bad16:32
danilosadiroiban, there's also the transfer of all the flags, like is_imported, from the deleted message to the preserved one16:32
adiroibandanilos: i guess there are many SELECT queries that will need to be modified to take into acount the tag16:32
danilosadiroiban, yeah, it'd end up being a pretty big change, with many performance implications16:33
danilosadiroiban, as such, I wouldn't tackle this before many other things are cleaned up first16:34
adiroibandanilos: but this is a big issues if someone is using Launchpad translations for reviewing or doing QA related tasks16:35
danilosadiroiban, 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 first16:36
danilosadiroiban, 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 that16:37
adiroibandanilos: for the duplicate suggestions16:38
danilosadiroiban, we'll get some weird behaviour occasionally, but let's say they are of lower priority16:38
adiroibanmaybe we can have a cron job that will do the cleaning16:38
danilosadiroiban, now, there's still some things to worry about16:38
danilosadiroiban, maybe, but that'll be very complex as well16:38
adiroibandanilos: what are are the other issues?16:39
danilosadiroiban, and it won't stop any weirdness16:39
danilosadiroiban, 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 one16:40
danilosadiroiban, i.e. http://paste.ubuntu.com/394130/16:41
danilosadiroiban, also, note that you can't unset potemplate if is_imported is true either16:41
adiroibandanilos: for lates example, I think result 1 is fine16:43
adiroibanand then another reviewer can decide if the shared suggestion is better16:44
adiroibanor it will need a new diverged translation16:44
danilosadiroiban, right16:45
danilosadiroiban, ok, so, then it won't be that complicated16:46
adiroibandanilos: what are the consequences of setting potemplate = None for a msgset with is_imported = True ?16:46
danilosadiroiban, 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
danilosadiroiban, it won't be diverged is_imported message, and if there's another shared is_imported message, you'd be in for a ride16:47
danilosadiroiban, basically, many places where only one is expected would assert/traceback16:47
danilosadiroiban, provided you don't hit a DB constraint first (I am not sure if we have one like that)16:48
adiroibandanilos: I see16:48
adiroibandanilos: ok. thanks. one more question regarding potmsgset.updateTranslation()16:50
danilosadiroiban, sure (though, you still can't touch it: nobody can, functionality needs to be slowly factored out of it :)16:50
adiroibanshould I add a new bug for the case when you tick „needs” for a already reviewd translation?16:50
adiroibanor we already have a bug for that?16:51
danilosadiroiban, what do you mean?16:51
adiroibanif you add a new suggestion16:51
danilosadiroiban, ok?16:51
adiroibanand you tick „needs review”16:51
adiroibanit will be listed as a new suggestion16:51
danilosadiroiban, right, that's how it should work16:51
adiroibanbut if in fact it is not a new suggesion16:52
jtvsalgado-lunch: I've fixed up the weirdness in my branch.  I'm going to reboot now, hoping that that'll fix it.16:52
adiroibanand _findTranslationMessage16:52
adiroibanfinds that this new suggestion16:52
danilosadiroiban, ah, right, it's an already discarded suggestion16:52
adiroibanis in fact an already existing suggestion16:52
adiroibanit will not mark it as needs review16:52
adiroibanthis is why I touched the updateTranslation code16:52
danilosadiroiban, I am pretty sure we have a bug about that as well16:53
adiroibanok16:53
danilosadiroiban, 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
danilosadiroiban, and again, moved the logic to decide what to do out into browser code16:54
adiroibandanilos: yes. I will move that logic.16:54
daniloss/moved/move16:54
adiroibanthat was the initial implimentation16:55
adiroibanbut then I didn't know where to put the tests16:55
adiroibanfor that code16:55
danilosadiroiban, right, I am talking about the other bug where discared suggestions don't show up as suggestions afterwards16:55
adiroibandanilos: ah. ok... do you have any suggestion for where should I put the integration tests for resetCurrentTranslation ?16:55
adiroibanlib/lp/translations/browser/tests/translationmessage-views.txt ?16:56
adiroibanI am very puzzled by the way unit tests and integration tests are structured16:56
danilosadiroiban, integration test? that would probably be very tricky, I'd look for existing needs review tests16:56
danilosadiroiban, good unit tests are most important here; single integration test is good enough16:57
adiroibandanilos: lib/lp/translations/tests/potmsgset-update-translation.txt16:57
danilosadiroiban, the problem is that all these translationmessage and pofile views are a mess, just like updateTranslation is: there is no clear separation of anything16:57
adiroibandanilos: ok16:58
adiroibanthanks!16:58
danilosadiroiban, 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:58
adiroibanI will try to fix the current problems and come back with more questions16:59
danilosadiroiban, sure, I am likely to be out, but I can probably find some time over the weekend (email me :)16:59
adiroibandanilos: no emails for the weekend... and I don't know if I will have time over the weekend17:00
danilosadiroiban, heh, ok :)17:00
danilosadiroiban, anyway, to be honest, this should probably be a new checkbox closer to the current translation; hopefully redesign of +translate views will fix all this17:01
adiroibandanilos: I agree, as this is not „needs review” , but reset reviewed state17:02
=== deryck is now known as deryck[lunch]
adiroibandanilos: did you have time to look over the POTemplate API ?17:03
=== matsubara-lunch is now known as matsubara
=== salgado-lunch is now known as salgado
salgadoadiroiban, did you get a failure email this time?17:10
adiroibansalgado: yes. thanks17:12
adiroibansalgado: are the same windmill tests that on my computer are also failing on mainline17:14
salgadoadiroiban, are you sure your mainline branch is pristine?17:14
salgadobecause these tests don't fail in other branches I've submitted to ec217:15
salgadoe.g. the one I submitted for wgrant which is on PQM right now17:15
adiroibansalgado: yes , the mainline is pristine... but my computer is slow and all waits.forElemets are exceded17:16
adiroibanbut most probably, my changes are introducing more delays17:16
adiroibanand this is why those tests are failing on ec217:17
salgadocould be17:17
adiroibanand they are not failing all the time17:17
adiroibanon my computer, there are 50% chances of a windmill test to pass17:18
adiroibanand this was since the beginning of my hacking on launchpad17:18
danilosadiroiban, sorry, I don't remember seeing anything about the potemplate API, where was that?17:28
adiroibandanilos: hm... I have added them as comment to the bug17:29
adiroibanand I remember sending an notification email to you, henning and jtv17:29
adiroibanthe subject was POTemplate Details API17:30
adiroibanhttps://bugs.edge.launchpad.net/rosetta/+bug/52537117:30
mupBug #525371: API for reading POTemplates details <api> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/525371>17:30
=== deryck[lunch] is now known as deryck
=== gary_poster is now known as gary-lunch
rockstarsalgado, care to look at a branch really quick?18:47
salgadorockstar, sure, where's it?18:47
rockstarhttps://code.edge.launchpad.net/~rockstar/launchpad/recipe-canonical_url/+merge/2127118:47
rockstarIt's really little.18:47
salgadorockstar, I wonder if isn't there a better place for that test...18:49
salgadoalso, you don't really need to add that empty file, do you?18:49
salgadonot really empty, but with no actual code18:49
rockstarsalgado, I do because abentley and I both need it very soon, and we wanted to avoid conflicts.18:49
salgadooh, ok.  can you s/2009/2010 there, then?18:50
rockstarHuh, I thought I did...18:50
salgadorockstar, how about moving the test to ./lib/lp/code/tests/test_sourcepackagerecipe.py ?18:51
rockstarsalgado, according to https://dev.launchpad.net/Web/URLTraversal that's where the test should go.  I originally had a unit test there.18:51
salgadooh, didn't know about that18:52
salgador=me18:53
=== kfogel is now known as jimb_intestines
=== jimb_intestines is now known as kfogel
rockstarsalgado, cheers!18:53
jtvsalgado: 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.18:58
salgadojtv, ok, I've just sent you a second round of feedback19:05
jtvthanks19:05
=== gary-lunch is now known as gary_poster
salgadojtv, will you let me know once you're done with that branch?19:59
jtvsalgado: yes19:59
=== jamalta is now known as jamalta-afk
=== jamalta-afk is now known as jamalta
jtvsalgado: over to you.  It's deep night here, so I won't be around for much longer20:24
jtvwhoops, maybe I should push those changes first :)20:24
salgadojtv, that's a good idea. :)20:29
jtvsalgado: it's taking a while for the diff to update...20:29
jtvsalgado: updated now.20:30
salgadocool20:31
=== 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
jtvsalgado: thanks!21:25
=== salgado is now known as salgado-afk
wgrantAnybody around for a review of https://code.edge.launchpad.net/~wgrant/launchpad/bprdc/+merge/21213?21:50
=== 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

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!