=== EdwinGrubbs 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
[04:40] <wgrant> Anyone around to review a tiny regression fix?
[04:40] <wgrant> https://code.launchpad.net/~wgrant/launchpad/bug-522800-getFileByName/+merge/43159
[04:40]  * mwhudson takes a look
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-lunch
=== matsubara-lunch is now known as matsubara
=== salgado is now known as salgado-lunch
=== henninge changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:55] <henninge> Hi jcsackett!
[16:55] <henninge> I have a branch that is actually quite simple.
[16:55] <jcsackett> hello, henninge. i would be happy to look at it.
[16:55] <jcsackett> link?
[16:56] <henninge> But it contains a sample data change that makes the diff look big.
[16:56] <henninge> https://code.edge.launchpad.net/~henninge/launchpad/recife-611668-fix-tests/+merge/43236
[16:56] <henninge> jcsackett: thanks
[16:56] <jcsackett> henninge: you're welcome.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[17:02] <jcsackett> henninge: in pofile.txt did you move the bit "msgid = potmsgset.msgid_singular.msgid..." out of the loop just to speed things up, since it appears to be a value that won't change in iteration?
[17:02] <jcsackett> or was there another purpose?
[17:03] <henninge> no, just because there was no reason for it to be in the loop
[17:03] <henninge> It may even speed  up the loop, yes. ;)
[17:03] <jcsackett> henninge: cool, thanks.
[17:04] <henninge> The test was failing because of the output from the format string further down.
[17:04] <henninge> The old format produced trailing spaces but I don't understand why that worked before.
[17:05] <henninge> jcsackett: I have to run out right now but will be back in about an hour. Sorry.
[17:05] <jcsackett> henninge: no problems.
=== salgado-lunch is now known as salgado
=== deryck is now known as deryck[lunch]
[17:19] <EdwinGrubbs> jcsackett: I've added my branch to the queue: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas/+merge/43116
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: henninge || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[17:20] <jcsackett> EdwinGrubbs: okay.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== benji is now known as benji-lunch
=== deryck[lunch] is now known as deryck
[18:33] <jcsackett> EdwinGrubbs: are you planning on fixing the lint errors that you put in the MP?
[18:34] <EdwinGrubbs> jcsackett: yes, except for the one where it complains about two blank lines because of a comment.
[18:35] <jcsackett> EdwinGrubbs: yeah, i was ignoring that one. just confused b/c i generally clean up lint before submitting, but either way works as long as they'll get fixed before merging.
[18:38] <henninge> jcsackett: Hi
[18:38] <jcsackett> henninge: hey there.
[18:39] <henninge> jcsackett: There is a prerequisite branch, as mentioned in the MP
[18:39] <jcsackett> henninge: yes, i got that. i didn't have failing tests. :-/
[18:39] <henninge> hm
[18:39] <EdwinGrubbs> jcsackett: sometimes, reviewers don't like it in the mp diff, because it makes it harder to understand the real changes.
[18:39] <jcsackett> EdwinGrubbs: fair point.
[18:40] <jcsackett> henninge: i will look again after reviewing Edwin's branch. there is nothing else but the recife branch, the prereq listed, and yours to consider, correct?
[18:41] <henninge> jcsackett: actually, there are three other branches before that but I thought they'd be included in the one branch (because it depends on them)
[18:42] <henninge> jcsackett: the other problem is that the original branches were started against db-devel
[18:42] <henninge> but now the the merge is into recife.
[18:42] <henninge> because that was backed out of db-devel again.
[18:42] <jcsackett> hm.
[18:43] <jcsackett> henninge: what branch were you looking in that had the errors?
[18:43] <henninge> jcsackett: db-devel-bug-611668-filtermethods-2 merged into recife
[18:43] <jcsackett> henninge: okay, that is what i thought i was looking at, but i will rebuild it and double check.
[18:44] <jcsackett> btw, henninge: where does the name "recife" come from?
[18:45] <henninge> jcsackett: it's the place of the sprint where this branch was started ... in March :/
[18:45] <henninge> Recife, Brazil
[18:45] <jcsackett> henninge: ah. cool.
[19:06] <jcsackett> EdwinGrubbs: there are a few comments on your MP.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== benji-lunch is now known as benji
[19:08] <jcsackett> henninge: taking another look now.
[19:10] <henninge> jcsackett: I think I know what is going on here
[19:10] <jcsackett> henninge: hit me.
[19:11] <henninge> does pofile.txt fail for you?
[19:11] <henninge> That is the test that was originally failing for me.
[19:11] <jcsackett> henninge: tell you in a second, building bin/test for your branch now.
[19:11] <henninge> I fixed that by updating the sample data, as described.
[19:12] <jcsackett> rather, building bin/test for recife now.
[19:12] <henninge> After that, the other two tests failed because the ordering of the translationsmessages changed.
[19:12] <henninge> They were ordered by "date_last_touched"
[19:13] <henninge> and two had been touched by the migration script.
[19:14] <henninge> So I "fixed" that by manually setting the "date_last_touched" back to what it was before (in the sampledata) instead of changing the order in the test.
[19:15] <henninge> So it's correct, the two tests translationsperson.txt and xx-person-activity.txt will pass before and after branch.
[19:15] <henninge> So, only pofile.txt is being fixed by this branch, the others were just "not broken"...
[19:17] <EdwinGrubbs> jcsackett: thanks for the review
[19:18] <jcsackett> henninge: that seems to be the case (coupled with me possibly messing up purity of branches when checking pofile, which didn't fail on recife before).
[19:18] <jcsackett> but carefully rebuilding the state of the branch before your MP does indeed show pofile failing.
[19:20] <jcsackett> henninge: r=me; i am mentoring so sinzui will need to follow up. i have already requested a review from him.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[19:20] <henninge> jcsackett: thanks ;-)
[19:21] <jcsackett> henninge: your welcome. sorry i evidently goofed on checking the tests. :-P
[19:28] <henninge> jcsackett: and I mislead you by implying that these tests were failing.
=== EdwinGrubbs is now known as Edwin-afk2
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk
[21:25] <bac> jcsackett: time for a quick review?
[21:26] <jcsackett> bac: sure.
[21:26] <bac> https://code.edge.launchpad.net/~bac/launchpad/bug-662994/+merge/43299
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[21:27] <jcsackett> bac: bit of a delay; the diff has not yet appeared. :-P
[21:27] <bac> jcsackett: it is there now
[21:28] <jcsackett> bac: huzzah.
[21:33] <jcsackett> bac: are there no unit tests around this the review view?
[21:35] <bac> jcsackett: i didn't find any.  i held my nose and updated the story
[21:36] <jcsackett> hrm. i'm tempted to file a bug against the lack of actual unit tests in place of stories...
[21:37] <bac> jcsackett: i can defer this one and create the unit tests
[21:37] <jcsackett> bac: i would buy you a beer if you did that. :-)
[21:38] <bac> ok
[21:38] <jcsackett> i like the code though. looks like a good branch.
[21:38] <jcsackett> you want me to still claim the MP, or leave it till later?
[21:39] <jcsackett> if you want me to go ahead and claim it, i'll happily take a look at it whenever you're done.
[21:39] <jcsackett> bac ^
[21:39] <bac> i'll just retract it
[21:40] <jcsackett> bac: okay. thanks for putting tests in place: you're a gentleman and a scholar.
[21:40] <bac> bah
=== jcsackett 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
[22:46] <jcsackett> is there anyone available to review: https://code.edge.launchpad.net/~jcsackett/launchpad/membership-status-matters-676494/+merge/43303
[22:46] <jcsackett> it's a test + a small change to a sql query