=== 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 [05:45] Any volunteers for my branch? It's tiny! https://code.launchpad.net/~jtv/launchpad/recife-gettranslationrows/+merge/41925 [05:50] jtv: My only concern is: [05:50] + # The message is included, but is still marked as obsolete. [05:50] 84 + self.assertEqual(0, vpoexport.sequence) [05:50] I dun geddit. Zero is a magic number? [05:52] Yes. [05:52] jtv: Can you explain how? [05:52] I would very much like us to use null for that, but 0 is how our forefathers did it and we never quite broke away from that. [05:52] Every POTMsgSet occurring in a template has a sequence number in that template. [05:53] (This sequence number is stored, in Entity-Relation terms, as an attribute on the relationship between POTemplate and POTMsgSet. In the schema you find it in TranslationTemplateItem.) [05:53] When a later update of that template no longer includes the message, the message's sequence number in the template becomes 0. [05:54] Ahhh, it's a refcount of sorts? [05:54] No, it's a 1-based sequence number for non-obsolete messages in a template, with 0 being a special case for messages that are no longer in the template. [05:55] Right [05:55] Absolutely ought to be null AFAIC—if only to make the ordering and the unique indexes sensible. [05:56] jtv: I've +1'd it, but you'll need one other [05:56] Right ho. Thanks! === 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 [06:00] jtv: approved [06:00] henninge: molt dankon or whatever the esperanto is :) [06:00] * StevenK high fives henninge [06:01] ;-) [06:04] thanks guys, it's landed. [06:04] (on recife) === jtv 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 === 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 [09:40] topic On call: adeuring || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [09:40] herzlich wilkommen abel :) [09:51] jtv: hey jtv! [09:51] :) [09:52] jtv: IN line 37 of the diff, you run "if self.form_is_writeable:" for each loop iteration. I think you can move the "if" outside the loop [09:53] adeuring: true… it's a cachedproperty, but with this short a loop I guess that makes sense. [09:53] jtv: yeah, that's a real micro-optimisation... [09:53] * jtv looks again [09:53] Heh, I had missed how completely nonsensical the loop becomes in one of the two cases. :) [09:54] ;) [09:54] And now it becomes clear that this is just one big list comprehension. [09:55] jtv: perhaps I had not have yet enogh coffee -- but where did the "alt_current.setPOFile(alt_pofile)" from line 49 go to? Or is it simply unnecessary? [09:58] adeuring: that's what made me clean this up—alt_current goes into alt_submissions, which is really just [alt_current] + alt_external, so at the end I just have to loop over alt_submissions instead of alt_external. [09:58] jtv: ah, thanks! r=me, then. [10:00] adeuring: thank you. I'm pushing an update to that loop that also removes a nesting level. [10:00] jtv: cool [10:04] adeuring: diff has updated. [10:04] * adeuring is looking [10:06] jtv: again, r=me. I like it! [10:06] thanks :) === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara [12:04] jtv: still around or already a square? [12:04] henninge: not asquare yet, no [12:05] jtv: branch pushing [12:05] See it. [12:07] jtv: here is the diff of my changes becaue the mp diff will just show added files. http://paste.ubuntu.com/536664/ [12:07] Very helpful, thanks. [12:09] henninge: beware of expressions like "old model" and "new model" in the codebase. Many changes down the road someone may read that and not be able to figure out which "new" model you were referring to, way back when. Make sure you have at least one specific reference near the beginning to set the right expectations. [12:09] jtv: I fully expect these three files to be removed from the codebase soon ... ;) [12:10] Even so, form the habit! [12:10] jtv: I have that habit and have been telling it to others in review. "Document the state, not the change" [12:11] Good, good [12:11] but this script *is* about change. [12:11] Which is why I'm not complaining about the repeated mis-spelling of "in" as "int." :-) [12:11] the reason for it existence is that we have an "old model" and a "new model" ... [12:11] oh [12:12] Hennign Fast-Finger Eggers [12:12] ? [12:12] Then there's "loose," which is an adjective where you mean the verb "lose." [12:12] Hennign? [12:12] As in hennigne? [12:12] kind of ;) [12:13] «For this reason, all current messages in \n source packages need to get their "is_current_upstream" flag set.» → You mean the ones in upstream projects, right? [12:14] "lose has lost an 'o'", how could I forget that ... ;) [12:14] oh [12:15] of course, that is a very dumb error [12:16] Also, I find it a bit confusing that you seem to rename all references to "imported" to ones with "upstream," but do not update "current" to "ubuntu" where appropriate. Frankly I'd be more comfortable with this branch if you left all the variable names etc. intact, and only changed identifiers where needed to port this to Recife. [12:18] I find that more confusing, actually ... [12:18] but where did I leave "current" in ? [12:18] Well it is more confusing to read, but it's a lot less work to review! [12:19] oh, sorry [12:19] ok, I found the "CurrentTranslation" class [12:20] Looks to me like you let all uses of "Current" (with the capital C) in place. [12:20] e.g. test_getCurrentNonimportedTranslations_current_upstream [12:21] Which is doubly confusing because I can't tell if that means "current on the upstream side" or "current as in the old model, and current-upstream." [12:21] Given that this is a migration script, I'd have no problem with reading a script documented along the lines of the old model but with just the identifiers updated to work in the new model. [12:21] We still have plenty of code like that in the Recife branch anyway. [12:22] ok, ok [12:37] jtv: http://paste.ubuntu.com/536667/ [12:37] Renames for Current [12:39] jtv: still reviewing or sobbing in a corner? ;-) [12:40] henninge: just a sec [12:42] henninge: thanks, that's better—a ¾ reduction [12:42] Oh, no it's not [12:42] it's incremental over the last one. [12:42] That only makes things harder. [12:44] henninge: I really don't see the point in having to review 500 lines of diff for renaming two flags in one script. [12:44] This was meant to be a near-trivial change. [12:45] jtv: but it is just renaming - that reads fast, does it not? [12:45] It makes it a lot harder to see whether any of the renamings are wrong. [12:45] You dign't expect you to check if I renamed every item correctly [12:45] jtv: I can reduce, though, if you like. [12:46] Well if you replace all use of "imported" or "current" all over the place, I _do_ need to check that because it's so easy to get it wrong! [12:46] jtv: ok, then stop and let me redo it. [12:46] Thanks! [13:12] jtv: here you go, the "minimal impact" edition. ;-) [13:12] http://paste.ubuntu.com/536678/ [13:13] How very environmentally conscious. :) [13:18] henninge: you may find makeCurrentTranslationMessage's current_other parameter helpful. [13:18] oh, I forgot about that [13:20] (In the tests I am working on, I also have a helper for making a message that's current only on the other side) [13:20] yes, I am missing that [13:20] We should include that in the factory [13:21] Maybe, yes. [13:23] henninge: there's a test that creates a message that's shared in upstream and Ubuntu, but at the same time diverged. That's a bit weird. [13:24] Hm, maybe you need to think "old model"? [13:24] I can't promise that makeCurrentTranslationMessage will do that—or that makeDivergedTranslationMessage will accept it. [13:24] It's current, imported and diverged. [13:24] Ah, of course, in the old model it's possible! [13:25] Have you checked that makeCurrentTranslationMessage really produces such a message? It may quietly ignore one of the parameters in this strange case. [13:25] jtv: the test does, does it not? [13:25] * jtv unscrews eyes [13:26] This is in test_updateTranslationMessages_diverged, right? [13:26] ah, the test goes directly into the tm to set the flag anyway. [13:26] Yes. [13:26] And by the way, thanks for also cleaning up that misplaced "variants" docstring! [13:27] I thought that had been done, actually. Aaron had complained about it, too. [13:27] I did it in my branch based on Aaron's review, but then danilo went in and fixed the bug in the script in his own branch. [13:28] The fixes were purely cosmetic, which is a bit pointless in a one-off script, so we never made the effort to backport them. [13:29] right and I landed danilo's branch directly [13:30] henninge: you're reviewed. Good night! [13:30] now I remembe I had considered doing Aaron's bidding but decided against it. [13:30] And God speed. [13:30] jtv: thank you very much! ;-) [13:30] np [13:30] Yes, willneed that ... [13:30] Hope to see our branch landed Monday morning! [13:31] So do I [13:31] * henninge eyes buildbot [13:32] * henninge goes to lunch === matsubara is now known as matsubara-lunch [14:45] Hi adeuring, could you review https://code.edge.launchpad.net/~gmb/launchpad/bug-276723/+merge/41952 for me? [14:46] gmb: yure [14:46] sure, even [14:46] Thanks === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jelmer_ is now known as jelmer [14:55] gmb: r=me.. thanks for taking the boring task of fixing all these trivial test failure [14:56] adeuring: No problem. I seriously considered rewriting a bunch of those tests as unit tests, but then I decided that I'd like to actually land the branch before Christmas. [14:56] gmb: yeah, completely understandable ;) === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-lunch is now known as matsubara === salgado is now known as salgado-lunch === salgado-lunch is now known as salgado === matsubara is now known as matsubara-afk === adeuring changed the topic of #launchpad-reviews to: On call: - || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara === matsubara is now known as matsubara-afk