/srv/irclogs.ubuntu.com/2009/12/22/#launchpad-reviews.txt

=== mwhudson_ is now known as mwhudson
mwhudsonanyone here?00:36
mwhudsonjml: maybe you?00:36
mwhudsonrockstar even?00:42
rockstarmwhudson, hello sir.00:42
mwhudsonrockstar: can you do a trivial review?00:42
mwhudsoni know you're not really here...00:42
rockstarmwhudson, sure, send it on over.00:44
mwhudsonheh, it's https://bugs.edge.launchpad.net/bugs/392155 reappearing 100k bugs later00:44
mupBug #392155: make run_all is broken <Launchpad Foundations:Fix Released by jml> <https://launchpad.net/bugs/392155>00:44
mwhudsonrockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/unshag-make-run_all/+merge/1646100:46
mwhudsonman there's a lot of typing to do for a trivial fix :(00:46
rockstarmwhudson, r=me00:49
mwhudsonrockstar: ta00:49
jmlmwhudson, I'm back.00:53
mwhudsonjml: hello00:53
mwhudsonjml: could you maybe look at https://code.edge.launchpad.net/~mwhudson/launchpad/log-noop-puller-runs/+merge/16460 ?00:53
mwhudsonjml: it involves gathering data, you'll like it 00:54
mwhudson:-p00:54
* mwhudson .oO(maybe i need to graph it too for full jml baiting...)00:54
jmlhahaha00:55
jmlmwhudson, any ideas for a graph?00:58
mwhudsonjml: dunno00:59
mwhudsonjml: puller runs per day by type?00:59
mwhudsonbranch type i mean00:59
mwhudsonwould be easier after i finish off that branch that schedules puller runs with the job table00:59
mwhudsonsadly the last time i looked at that branch i started throwing things around the room00:59
jmlmwhudson, well, you're moving out soon right?01:01
mwhudson:)01:01
mwhudsonjml: i think i need to abandon my attempt to be minimal and actually think about it again01:04
mwhudsonjml: could i ask you to ec2 land https://code.edge.launchpad.net/~mwhudson/launchpad/unshag-make-run_all/+merge/16461 ?01:05
mwhudsoni would, but i need to pack up for a bit here01:05
jmlmwhudson, sure.01:05
mwhudsonta01:06
mwhudsonmind you01:06
mwhudsonmaybe ec2 land is excessive for that branch01:06
jmlmwhudson, do you want to just pqm-submit it?01:07
mwhudsonjml: yeah ok01:07
* mwhudson uses ec2 land to figure out the commit message01:07
jmlmwhudson, that's good. because I am late for a thing.01:07
mwhudsonjml: :)01:07
mwhudsonjml: enjoy your thing01:07
noodles775mwhudson: can you r or rs a testfix reversion of r10066 (which enabled inclusion of windmill tests by default) http://pastebin.ubuntu.com/344576/08:26
noodles775^^^ or anyone else who is keen.08:27
mwhudsonnoodles775: did you consider disabling the memcached layer tests?08:27
mwhudsonas it seems to be some interaction there?08:28
mwhudsonotoh, this is easier i guess08:28
noodles775mwhudson: I mentioned in my email,...08:28
mwhudsonnoodles775: ah okay08:28
mwhudsonnoodles775: r=me then08:28
noodles775safer... (we can disable the memcached tests and reenable these in another branch that goes through ec2)08:28
noodles775thanks08:28
henningeadiroiban: the final diff looks good. Let's land this!09:16
henningeadiroiban: Your last devel merge has been a few days. Can you please do that once more? I will land the branch then.09:16
henningeadiroiban: Thanks for your work.09:16
adiroibanhenninge: sure09:17
adiroibanhenninge: do I need any special setup for running soyuz or packagearchiver tests?09:17
henningeadiroiban: not that I know of. They are part of the normal test suite AFAIK09:18
adiroibani have started a full test for LP and some tests get stuck09:18
henningeshouldn't. Hm...09:19
henningeadiroiban: some tests take a bit longer, I think.09:20
adiroibanwell for ex this one was running for 8 hours lp.archiveuploader.tests.test_ppauploadprocessor.TestPPAUploadProcessor.testPPAUploadResultingInNoBuilds09:20
adiroibanhenninge: sorry for the delay. got some problem with the new css stuff. The merged branch should be pushed10:05
henningeadiroiban: I just realised: the dot "." in "and <a href=\"%s\">%d other templates</a>." should probably be placed in the template so that it appears at the end of the sentence even if this part is empty.10:05
henningeadiroiban: cool10:06
henningenp10:06
adiroibanhenninge: that was my first implementation... but this will brake some tests10:06
henninge;)10:06
henningenever mind, then.10:06
adiroibanin the previous implementation there was no dot10:06
henningewhich I'd consider a bug since this is a full English sentence. But just a very small one :-)10:07
adiroibanhenninge: let me fix it10:08
adiroibanand change the tests10:08
henningeadiroiban: if you wish. The branch is approved for landing as it is ... ;)10:09
henningethanks a lot!10:09
adiroibanwell, I will fix it...as it's now or never10:12
jtv1oh, hi henninge, hi adiroiban!10:15
henningeHi jtv1, 2, and 3 ... ;-)10:16
=== jtv1 is now known as jtv
henningejtv: had enough of cloning?10:16
jtvhenninge: yes, what's the fun in reproducing if you have to do it this way?10:16
henninge:)10:17
jtvNobody on call today?10:17
jtvI just fixed a regression to my own fix... the translations branch approver was still missing a permission.10:17
adiroibanjtv: hi10:17
adiroiban:)10:17
henningejtv: are you saying it's a small branch?10:17
jtvadiroiban: not gone for the holidays yet?  Or will you be here even more during the holidays?  :-)10:17
jtvhenninge: yes10:18
jtvhenninge: https://code.edge.launchpad.net/~jtv/launchpad/son-of-bug-487447/+merge/1648010:18
jtvalmost all test change10:18
jtvand most of the diff is adding 1 line of code to each test in the test case.10:19
jtv(lots of context lines)10:19
henningejtv: r=me10:24
jtvhenninge: kuhl, danke!10:24
henningelol10:24
stubPretty trivial review at https://code.edge.launchpad.net/~stub/launchpad/librarian-report/+merge/16463, maybe for jtv.11:14
jtvstub: having a look...11:15
jtvstub: why use Popen instead of our nice run_script wrapper?11:16
stubWe have a nice run_script wrapper? This time I'm just cargo culting what I landed last week. Last week I didn't know about run_script nor if it is suitable in this case.11:16
stub(this test is cut & paste the preceding test, with a few different arguments)11:17
jtvstub: and shame, shame, SHAME on you (or whoever is responsible) for using slashes in dates...11:17
stubjtv: Anything PG will parse unambiguously will work - you might notice the lack of validation...11:17
stubI think - works too11:18
stubFor the slash deficient amongst us11:18
jtv<slash?  I'll give you a slash!  hah!>11:18
stubI think we only hire losas in locales which use / though so it is fine.11:19
adiroibanjtv: yep. still here to annoy Henning  with my branches11:19
* jtv bares teeth at stub11:19
jtvadiroiban: annoy?  not very likely :)11:19
adiroiban:)11:19
jtvstub: slashes in dates are a symptom of inbreeding11:20
* stub goes to take a slash11:21
henningeadiroiban: cool dot! ;-)11:23
henningeadiroiban: I'll initiate the landing ...11:24
henningeforward thrusters ... check ... landing gear ... check ... parachutes .... ch...check!11:24
henningeadiroiban: we're in descent11:34
jtvstub: I'm also slightly curious what happened to the line of test output right above your real change... did you just pick a different line of the output to compare against?11:36
stubjtv: Yes. I wanted the tests to demonstrate the output changes with the different command line arguments.11:37
jtvstub: makes sense11:42
jtvstub: afaict you could get rid of all existing imports and most of the code in this test if you use run_script11:45
stubk11:50
stubOne import - I still have to assemble the patch etc.11:53
adiroibanhenninge: successfully landed. Thanks!12:55
adiroibando you think you can land this branch ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-497438/+merge/1633512:55
adiroibanit fixed the lucid translation not found problem12:55
=== ursula is now known as Ursinha
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
bacgood morning13:11
noodles775hi bac, I'm just heading out for an hour, but I've got a lazr-js branch ready for review if you've time later:13:13
noodles775https://code.edge.launchpad.net/~michael.nelson/lazr-js/491337-error-style-try-2/+merge/1642313:13
bacok, noodles775.  i'll have a look13:13
noodles775ta 13:13
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [jelmer, noodlesXXX] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningeadiroiban: the branch did not land yet, there must be some misunderstanding.13:37
adiroibanhenninge: ah. I got an email from buildbot 13:37
adiroibanmaybe is for another branch...13:37
henningeadiroiban: yes, must be.13:38
adiroibanas I can not access the link from that email13:38
henningeadiroiban: what's the link13:38
henninge?13:38
adiroibanhttps://lpbuildbot.canonical.com/builders/lp/builds/44513:38
henningeadiroiban: http://paste.ubuntu.com/344718/13:39
henningethat's what's landed13:39
adiroibanthanks13:41
jtvstub: any news on the branch?13:42
stubNews?13:42
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jelmer || queue [noodlesXXX] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningeadiroiban: can you set a commit message on bug-497438, please?13:42
stubjtv: What am I doing to what branch?13:42
jtvstub: I thought you were trying out run_script, hence my silence13:43
stubjtv: No, I was waiting on your review. I can change it to run_script easily enough - no real gain though on clarity or line count that I can see but whatever.13:44
adiroibanhenninge: done13:44
jtvstub: oic... you meant "I still have to assemble the path" but wrote "I still have to assemble the patch."  What a difference a letter makes!13:45
stuboops13:46
jtvwhat I was getting at was that you don't have to compose the path any more13:46
jtvwe do seem to have a bunch of run_script definitions though13:49
jtvstub: you'd do (retval, out, err) = run_script('scripts/', ['--from=2005-01-01', '--until=2005-12-31']) and that's all it takes to run the script13:53
stubOk - so don't need the absolute path.13:54
jtvahem, 'scripts/librarian-report.py' that is13:54
jtvright13:54
stubsounds like good change then.13:54
jtv's what I thought too :)13:55
bachi jelmer13:57
bachi jelmer_14:00
* bac tries all permutations14:00
jelmerhi bac14:07
henningeadiroiban: it's landing14:08
bachi jelmer -- was going to talk to you about interactive testing of your branch with launchpadlib but i just included the comments in the review.  let me know if you have questions.14:10
jelmerbac: will do - thanks for the review!14:10
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: noodles || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado changed the topic of #launchpad-reviews to: on call: bac || reviewing: noodles || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
salgadohi bac.  can you do a trivial review for me?14:19
bacsalgado: sure14:20
* salgado suspects his m-p submission failed. it's been almost 20 minutes since he sent it and lp doesn't seem to have done anything with it14:31
salgadobac: https://code.edge.launchpad.net/~salgado/launchpad/bug-422552/+merge/1648314:39
=== salgado is now known as salgado-lunch
adiroibanal-maisan: Hi! Do you have time for landing this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375/+merge/16279 ?  Thanks! I've done a new full test and it was ok. 15:16
=== salgado-lunch is now known as salgado
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: salgado || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
bachi salgado16:02
salgadohi bac16:02
bacsalgado: what if you just did request.split() instead of request.split(' ')?  the former seems to handle mutliple space characters16:03
salgadodoes it?16:03
bacsalgado: and doesn't require normalizing16:03
bac>>> 'a b   c'.split()16:04
bac['a', 'b', 'c']16:04
bac>>> 'a b   c'.split(' ')16:04
bac['a', 'b', '', '', 'c']16:04
salgadoIf sep16:04
salgado    is not specified or is None, any whitespace string is a separator.16:04
salgadoyeah, looks good to me16:04
bacsalgado: but i'm glad we have a test now for the case16:05
salgadoindeed16:06
bacsalgado: r=bac16:06
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
salgadobac, do you think a comment would be desirable explaining why we use split() instead of split(' ')?  (I don't think it would)16:07
bacsalgado: nah, since it DTRT i don't think it adds anything16:07
salgadocool16:08
salgadothanks bac!16:08
noodles775Thanks for the review bac (I dropped off earlier, so sorry if I missed any questions)16:18
bacnoodles775: nope.  just a fleeting moment of confusion when i realized i hadn't run 'make build' in the branch and that's why it did nothing.16:18
noodles775:)16:18
=== deryck is now known as deryck[lunch]
EdwinGrubbsbeuno: do you have any thoughts on how I should resolve the issue I commented on in the mp? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/1632518:35
beunoEdwinGrubbs, hey18:36
beunolet me actually read your comment18:36
beunohrm18:39
beunoI'm torn here18:39
EdwinGrubbsyep, me too18:40
beunowe need a good mechanism for this type of behavior18:40
beunothis branch is probably not the time to do it18:41
beunoEdwinGrubbs, how about this18:41
beunolets land this branch18:43
beunoand you brainstorm a bit on the list how to solve this problem in the future, in a more general way18:43
EdwinGrubbsbeuno: sounds good to me18:45
beunoEdwinGrubbs, cool, I'll make it official18:46
EdwinGrubbsthanks18:47
=== deryck[lunch] is now known as deryck
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
allenapbac: Thank you for the review. You saved my bacon :) I'll go and fix it now.20:30
bacummm, bacon20:31
bacallenap: i think gmb gets credit for all of those great XXX comments20:31
allenapbac: Yes :) And I get idiot points for not using grep!20:32
allenapbac: I'm still working on fixing those other XXXs, but it's getting late here, so I'll finish it tomorrow.21:58

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