/srv/irclogs.ubuntu.com/2010/11/26/#launchpad-reviews.txt

=== 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
jtvAny volunteers for my branch?  It's tiny!  https://code.launchpad.net/~jtv/launchpad/recife-gettranslationrows/+merge/4192505:45
StevenKjtv: My only concern is:05:50
StevenK+ # The message is included, but is still marked as obsolete.05:50
StevenK84+ self.assertEqual(0, vpoexport.sequence)05:50
StevenKI dun geddit. Zero is a magic number?05:50
jtvYes.05:52
StevenKjtv: Can you explain how?05:52
jtvI 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
jtvEvery POTMsgSet occurring in a template has a sequence number in that template.05:52
jtv(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
jtvWhen a later update of that template no longer includes the message, the message's sequence number in the template becomes 0.05:53
StevenKAhhh, it's a refcount of sorts?05:54
jtvNo, 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:54
StevenKRight05:55
jtvAbsolutely ought to be null AFAIC—if only to make the ordering and the unique indexes sensible.05:55
StevenKjtv: I've +1'd it, but you'll need one other05:56
jtvRight ho.  Thanks!05:56
=== 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
henningejtv: approved06:00
jtvhenninge: molt dankon or whatever the esperanto is :)06:00
* StevenK high fives henninge 06:00
henninge;-)06:01
jtvthanks guys, it's landed.06:04
jtv(on recife)06:04
=== 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
adeuringtopic On call: adeuring || Reviewing: jtv || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews09:40
jtvherzlich wilkommen abel :)09:40
adeuringjtv: hey jtv!09:51
jtv:)09:51
adeuringjtv: 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 loop09:52
jtvadeuring: true… it's a cachedproperty, but with this short a loop I guess that makes sense.09:53
adeuringjtv: yeah, that's a real micro-optimisation...09:53
* jtv looks again09:53
jtvHeh, I had missed how completely nonsensical the loop becomes in one of the two cases.  :)09:53
adeuring;)09:54
jtvAnd now it becomes clear that this is just one big list comprehension.09:54
adeuringjtv: 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:55
jtvadeuring: 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
adeuringjtv: ah, thanks! r=me, then.09:58
jtvadeuring: thank you.  I'm pushing an update to that loop that also removes a nesting level.10:00
adeuringjtv: cool10:00
jtvadeuring: diff has updated.10:04
* adeuring is looking10:04
adeuringjtv: again, r=me. I like it!10:06
jtvthanks :)10:06
=== 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
henningejtv: still around or already a square?12:04
jtvhenninge: not asquare yet, no12:04
henningejtv: branch pushing12:05
jtvSee it.12:05
henningejtv: here is the diff of my changes becaue the mp diff will just show added files. http://paste.ubuntu.com/536664/12:07
jtvVery helpful, thanks.12:07
jtvhenninge: 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
henningejtv: I fully expect these three files to be removed from the codebase soon ... ;)12:09
jtvEven so, form the habit!12:10
henningejtv: I have that habit and have been telling it to others in review. "Document the state, not the change"12:10
jtvGood, good12:11
henningebut this script *is* about change.12:11
jtvWhich is why I'm not complaining about the repeated mis-spelling of "in" as "int."  :-)12:11
henningethe reason for it existence is that we have an "old model" and a "new model" ...12:11
henningeoh12:11
henningeHennign Fast-Finger Eggers12:12
henninge?12:12
jtvThen there's "loose," which is an adjective where you mean the verb "lose."12:12
jtvHennign?12:12
jtvAs in hennigne?12:12
henningekind of ;)12:12
jtv«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:13
henninge"lose has lost an 'o'", how could I forget that ... ;)12:14
henningeoh12:14
henningeof course, that is a very dumb error12:15
jtvAlso, 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:16
henningeI find that more confusing, actually ...12:18
henningebut where did I leave "current" in ?12:18
jtvWell it is more confusing to read, but it's a lot less work to review!12:18
henningeoh, sorry12:19
henningeok, I found the "CurrentTranslation" class12:19
jtvLooks to me like you let all uses of "Current" (with the capital C) in place.12:20
jtve.g. test_getCurrentNonimportedTranslations_current_upstream12:20
jtvWhich 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
jtvGiven 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
jtvWe still have plenty of code like that in the Recife branch anyway.12:21
henningeok, ok12:22
henningejtv: http://paste.ubuntu.com/536667/12:37
henningeRenames for Current12:37
henningejtv: still reviewing or sobbing in a corner? ;-)12:39
jtvhenninge: just a sec12:40
jtvhenninge: thanks, that's better—a ¾ reduction12:42
jtvOh, no it's not12:42
jtvit's incremental over the last one.12:42
jtvThat only makes things harder.12:42
jtvhenninge: I really don't see the point in having to review 500 lines of diff for renaming two flags in one script.12:44
jtvThis was meant to be a near-trivial change.12:44
henningejtv: but it is just renaming - that reads fast, does it not?12:45
jtvIt makes it a lot harder to see whether any of the renamings are wrong.12:45
henningeYou dign't expect you to check if I renamed every item correctly12:45
henningejtv: I can reduce, though, if you like.12:45
jtvWell 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
henningejtv: ok, then stop and let me redo it.12:46
jtvThanks!12:46
henningejtv: here you go, the "minimal impact" edition. ;-)13:12
henningehttp://paste.ubuntu.com/536678/13:12
jtvHow very environmentally conscious.  :)13:13
jtvhenninge: you may find makeCurrentTranslationMessage's current_other parameter helpful.13:18
henningeoh, I forgot about that13:18
jtv(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
henningeyes, I am missing that13:20
henningeWe should include that in the factory13:20
jtvMaybe, yes.13:21
jtvhenninge: 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:23
henningeHm, maybe you need to think "old model"?13:24
jtvI can't promise that makeCurrentTranslationMessage will do that—or that makeDivergedTranslationMessage will accept it.13:24
henningeIt's current, imported and diverged.13:24
jtvAh, of course, in the old model it's possible!13:24
jtvHave you checked that makeCurrentTranslationMessage really produces such a message?  It may quietly ignore one of the parameters in this strange case.13:25
henningejtv: the test does, does it not?13:25
* jtv unscrews eyes13:25
henningeThis is in test_updateTranslationMessages_diverged, right?13:26
jtvah, the test goes directly into the tm to set the flag anyway.13:26
jtvYes.13:26
jtvAnd by the way, thanks for also cleaning up that misplaced "variants" docstring!13:26
henningeI thought that had been done, actually. Aaron had complained about it, too.13:27
jtvI 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:27
jtvThe fixes were purely cosmetic, which is a bit pointless in a one-off script, so we never made the effort to backport them.13:28
henningeright and I landed danilo's branch directly13:29
jtvhenninge: you're reviewed.  Good night!13:30
henningenow I remembe I had considered doing Aaron's bidding but decided against it.13:30
jtvAnd God speed.13:30
henningejtv: thank you very much! ;-)13:30
jtvnp13:30
henningeYes, willneed that ...13:30
jtvHope to see our branch landed Monday morning!13:30
henningeSo do I13:31
* henninge eyes buildbot13:31
* henninge goes to lunch13:32
=== matsubara is now known as matsubara-lunch
gmbHi adeuring, could you review https://code.edge.launchpad.net/~gmb/launchpad/bug-276723/+merge/41952 for me?14:45
adeuringgmb: yure14:46
adeuringsure, even14:46
gmbThanks14:46
=== 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
adeuringgmb: r=me..   thanks for taking the boring task of fixing all these trivial test failure14:55
gmbadeuring: 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
adeuringgmb: yeah, completely understandable ;)14:56
=== 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

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