#launchpad-reviews 2009-09-28
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> Hi intellectronica, here's one when you've time: https://code.launchpad.net/~michael.nelson/launchpad/437037-queue-timeout/+merge/12513
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: noodles || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<noodles775> intellectronica: I forgot to say on the MP, but the reason doc/build.txt is the relevant test is because it's testing the value of the estimated_buid_duration attribute of the resulting builds.
<intellectronica> noodles775: nice optimization. r=me
<noodles775> intellectronica: thanks
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] (pqm is release-critical) || This channel is logged: http://irclogs.ubuntu.com/ ||
<jtv> intellectronica: hi, I've got some stuff on the queue... you might enjoy the one about sprites.
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> PQM has been opened after Friday's re-roll.
<jtv> Yup.  I landed something over the weekend.
<henninge> jtv: you workaholic!
<jtv> I just wanted one more, one more!
<jtv> henninge: btw maybe you could review my fix for bug 358404?
<mup> Bug #358404: Set TranslationImportQueueEntry.date_status_changed in setStatus <Launchpad Translations:In Progress by jtv> <https://launchpad.net/bugs/358404>
<intellectronica> jtv: cool, starting on it
<jtv> intellectronica: cheers
<intellectronica> jtv: r=me for the sprites branch
<jtv> intellectronica: ta
<jtv> intellectronica: the other one is also a small UI fixup, not quite urgent enough to push last week
<jtv> maybe you'd be able to take a look at that one as well?
<intellectronica> yes, looking at it now
<jtv> this is just wonderful.
<henninge> jtv: does setStatus only get called if the status actually changed? Or should the mehod check for that and not update if unchanged?=
<jtv> henninge: I don't think there's anything to gain by making it that smart...  The UI already checks for non-changes.
<jtv> henninge: otoh the date gets updated explicitly when a re-upload refreshes a Needs Review entry without changing its state, so there's no particular reason not to check for non-changes either.
<henninge> jtv: but you mentioned API ..
<intellectronica> jtv: r=me for that too
<jtv> intellectronica: getting things done!  Thanks.  Does that include UI?
<henninge> jtv: ok, if you say so ... ;-)
<intellectronica> jtv: yes
<jtv> henninge: you've got a good point in that we might be doing non-updates that we don't realize.  Not sure if we want to notice those or just be able to ignore them.  :)
<jtv> intellectronica: spasiba
<jtv> intellectronica: ah, you reviewed the one that henninge was looking at!
 * jtv should have been clearer
 * henninge thought it was clear ..
<henninge> jtv, intellectronica: never mind, I still have a nasty bug in bzr imports to look at ...
<intellectronica> jtv: great, your work will benefit from the scrutiny of not one but two reviewers!
 * jtv trembles in joyful anticipation
<jtv> "If your code is good, you have nothing to fear.  Do you feel any fear, citizen?"
<intellectronica> jtv: so shall i look at bug-433989 now?
<jtv> intellectronica: that's what I was hoping for, yes, since it's UI.
<intellectronica> jtv: cool, grabbing it now
<intellectronica> jtv: r=me ui=me
<jtv> intellectronica: splended, thanks!
<jtv> *splendid
<gmb> intellectronica: Could you do a code and ui review of https://code.edge.launchpad.net/~gmb/launchpad/ajaxify-branch-linking/+merge/12514 please? It's the first part of the branch linking ajax work.
<intellectronica> gmb: sure
<gmb> Ta
<gmb> intellectronica: Argh, it's proposed for db-devel, not devel. Bugger... I'll generate one for you now.
<intellectronica> oh right
<gmb> intellectronica: Diff pasted.
<gmb> (into the mp)
<intellectronica> danke
<adeuring> intellectronica, henninge: could one of you review this small branch: https:/ ?/code.edge.launchpad.net/~adeuring/launchpad/bug-438024-parse-dmi-data/+merge/12518
<adeuring> https://code.edge.launchpad.net/~adeuring/launchpad/bug-438024-parse-dmi-data/+merge/12518
<jtatum> I have a small wording correction in a bzr branch and was wondering if someone could review it. This is my first merge proposal to launchpad and I don't know the process.
<intellectronica> adeuring: starting on it now
<intellectronica> jtatum: hi!
<jtatum> intellectronica, hi :)
<intellectronica> jtatum: so what's your fix about? and thanks for joining the effort :)
<intellectronica> jtatum: you can find some info on how to contribute at https://dev.launchpad.net/PatchSubmission (and on the dev wiki in general)
<jtatum> intellectronica, it's an embarrassingly trivial wording change for bug 435320
<mup> Bug #435320: +branding page text needs improvement <post-3-ui-cleanup> <story-logos> <trivial> <ui> <Launchpad Registry:In Progress by jtatum> <https://launchpad.net/bugs/435320>
<intellectronica> jtatum: also, the on-call reviewers, which change from time to time (henninge and myself are manning the current shift) can always help
<jtatum> intellectronica, great :) i read this and saw the various templates for proposals, but couldn't divine how to run lint or any other prerequisites I'd need to take care of for the merge
<intellectronica> jtatum: to run lint just type make lint in the branch
<intellectronica> jtatum: once you're happy with your change, propose it for merge into the launchpad/devel branch and ask for it to be reviewed
<intellectronica> that means me :) i will review your branch after you create the merge proposal and ping me
<intellectronica> jtatum: if it's all good, i'll land the branch on your behalf, if there are things to fix, we'll talk about them
<jtatum> intellectronica, fantastic :) proposing it now. thanks :)
<intellectronica> adeuring: s/ndes/nodes other than that everything looks great. r=me
<jtatum> intellectronica, submitted and diff generated: https://code.edge.launchpad.net/~jtatum/launchpad/bug-435320/+merge/12519
<intellectronica> jtatum: nice fix, merge-approved :)
<jtatum> intellectronica, neat! thanks. hopefully my next merge will be more substantial ;)
<intellectronica> jtatum: since you can't merge branches into the launchpad tree i will land it on your behalf (after testing it, so it will take ~3 hours before it lands)
<jtatum> intellectronica, no worries. thanks again.
<intellectronica> jtatum: great. if you want ideas on interesting things to work on, or if you're starting to work on something and want to chat about how to go about it, this is the place to come to
<jtatum> intellectronica, I believe I will :) incidentally I have submitted the canonical contributors agreement for LP, in case that comes up on a checklist somewhere
<adeuring> intellectronica: thanks!
<intellectronica> jtatum: excellent. i was just about to forget to ask you about that ;)
<intellectronica> whoa, so it looks like the whole ec2test manoeuvre changed
<noodles775> intellectronica or henninge : test fix for current buildbot breakage? http://pastebin.ubuntu.com/280368/
<henninge> noodles775: looking
 * intellectronica reads
<intellectronica> henninge: i'll leave it to you then and go eat
<henninge> intellectronica: enjoy it!
<noodles775> henninge: I've got two questions - first, is there a standard way to match pretty quotes in pagetests - I've used ... here  but have also seen '?' - happy to change them.
<noodles775> henninge: the second, I'll chat with Edwin afterwards, but I don't really agree with the way the labels are being used here (effectively using the leaf breadcrum 'page_title' as the label), but I'll send an email to Edwin about that.
<henninge> noodles775: I don't know. I have also seen backslashreplace used or actual pretty quotes in the test.
<henninge> *in tests*
<noodles775> henninge: I've seen the actual pretty quotes in view tests, but hadn't noticed in in pagetests, but yeah, lots of options.
<henninge> noodles775: would be too much for a testfix. this looks good. r=me
<noodles775> henninge: great, thanks.
<henninge> Get us up and running again!
<henninge> ;-)
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge, abentley || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> noodles775: have you not landed your testfix branch yet?
<henninge> Hi abentley !
<noodles775> henninge: I have indeed (r9581), but it seems buildbot hasn't started?
 * noodles775 checks with mthaddon
<henninge> noodles775: it was just restarted it looks
<noodles775> henninge: really? where are you seeing that? I'm looking at https://lpbuildbot.canonical.com/waterfall
<henninge> noodles775: me, too. master shutdown - master started ...
<noodles775> yep, that was a while ago though...
<henninge> oh, me not looking at time properly, then
<henninge> yeah, 3 hrs ago ...
<abentley> henninge: Hi
<henninge> abentley: what do you make of this? http://paste.ubuntu.com/280287/
<henninge> abentley: should I file a bug or could there be some pattern, I don't recognise?
<henninge> and s/toople/tuple/
<henninge> ;)
<abentley> henninge: These are the same type of tree?  They're not one RevisionTree and one WorkingTree?
<henninge> abentley: hm
<henninge> abentley: they should be the same.
<henninge> abentley: it happens when running the same script (rosetta-branches) on different branches.
<abentley> henninge: It's a bug.  The behaviour of get_file_text on directories should be consistent.
<henninge> abentley: ok, I'll file it.
<henninge> abentley: thanks
<abentley> henninge: np
<abentley> henninge: Please specify which tree type is causing this.
 * henninge looks that up
<henninge> abentley: it's a RevisionTree. I'll put that in the bug.
<abentley> henninge: great.
<abentley> henninge: A possible cause would be that one branch is in 2a format and the other is in a different format.
<abentley> henninge: Since 2a introduces a different way of representing tree contents.
<EdwinGrubbs> intellectronica, henninge, abentley: Can you review my branch to get pqm out of testfix mode?
<EdwinGrubbs> http://pastebin.ubuntu.com/280424/
<henninge> EdwinGrubbs: noodles775_ already did that.
<henninge> EdwinGrubbs: He claims to have landed it ... ;-)
<abentley> EdwinGrubbs: Sorry, debugging the puller.
<EdwinGrubbs> noodles775_: thanks for fixing my errors
<noodles775_> EdwinGrubbs: np! I sent you an email with some related thoughts.
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
<leonardr> abentley, i request a review for https://code.edge.launchpad.net/~leonardr/lazr.restful/basic-auth/+merge/12538
<abentley> leonardr: Sorry, I'm a bit distracted by problems with the branch puller.
<leonardr> abentley, np
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: leonardr || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
<abentley> leonardr: check_foo doesn't suggest to me that it returns a value.  What would you think about renaming it authenticate_user?
<leonardr> ok, sure
<abentley> leonardr: is the environ parameter to self.application allowed to contain arbitrary objects?
<abentley> leonardr: In the check_credentials docstring, I believe ReST requires an indent on the second line of ":return:".
<abentley> leonardr: There is no documentation of the __init__ parameters or the ivars.  Could you please document one of them?
<leonardr> abentley: yes, wsgi lets you put anything you want in environ
<abentley> leonardr: The name "self.regex" doesn't communicate what the regex is for.  What about secure_paths or secure_path_pattern or include_paths or something?
<leonardr> abentley, wilco
<abentley> leonardr: Aside from that, looks good to me.
<abentley> leonardr: random thought: would it be more WSGIish to pass check_credentials in as a callback?
<leonardr> abentley: i don't think so, but it would probably be nicer code
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
<EdwinGrubbs> abentley: can you review a super simple branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-421898-cross-site-timeline-graph/+merge/12551
<abentley> EdwinGrubbs: r=me
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ ||
<EdwinGrubbs> abentley: thanks
#launchpad-reviews 2009-09-29
<mwhudson> anyone around?
<henninge> jtv: Hi! Can you please review this patch of mine, that adds a test to yesterday's branch?
<henninge> ;-)
<henninge> jtv: http://paste.ubuntu.com/281070/
<henninge> jtv: I was in fact able to test it by going a bit deeper into the inner structure of the job.
<jtv> henninge: that's what I figured yesterday, but the question was whether it's worth the trouble.
<jtv> Doesn't look like too much work though
<henninge> jtv: once I thought of it, it was not too hard to write, no.
<henninge> jtv: the test case already has a lot of useful helper methods.
<jtv> henninge: but do you know that this test exercises the problem case?  I don't see a guarantee in there that there will be a directory in the set of changes.
<henninge> jtv: the method _makeBranch... creates the directory in the branch, so it does come up as changed.
<jtv> ah
<henninge> jtv: I also tried it out when my fix is removed and the test does fail, with the first assert finding three elements in the list.
<jtv> Is there an easy way you can check that in the test?
<henninge> jtv: I am passing NULL_REVISON to the job, so it asks bzrlib for all changes since the creation of the branch, so the directories *will* always be included.
<jtv> henninge: very well.  Another point: why all the trouble asserting the contents of those two lists?  You know what's in them, just assertEquals.
<jtv> ah, you extract the paths
<henninge> jtv: well, it could be that just a directory got extracted, instead of the file ...
<henninge> jtv: but what I could do, is check that all in one assert
<henninge> jtv: ah, no, not easily.
<jtv> you could self.assertEqual([pot_path], [x[0] for x in job.template_files_changed])
<henninge> jtv: yes. Do you find that easier to read?
<henninge> I was thinking along the lines of
<jtv> henninge: I find it easier to read with a little helper for the repetitive parts
<jtv> self.assertEqual([pot_path], get_paths(job.template_files_changed))
<henninge>  self.assertEqual([(pot_path, pot_content], job.template_files_changed)
<henninge> but I'd have to procide pot_content, with is a UniqueString atm.
<henninge> provide
<henninge> oh, could do that ...
<henninge> jtv: incremental diff: http://paste.ubuntu.com/281080/
<jtv> henninge: why bother with unique strings for the content?  Just pass "# empty" or something
<jtv> The code does get a lot shorter though, which shows you're on to something.
<henninge> jtv: I have been told that if a string is arbitrary, just say so be using UniqueString.
<henninge> which makes sense to me
<henninge> jtv: full difff: http://paste.ubuntu.com/281081/
<jtv> henninge: in this case, it isn't _entirely_ arbitrary: it's supposed to be in gettext.  And you're not trying to catch bzr passing the wrong file contents, and there's no risk of accidentally picking up data from elsewhere.
<henninge> jtv: right, it is not about the content at all
<henninge> jtv: but this is needed to make the assert easy to write because the tuple contains the content.
<henninge> tuples
<jtv> Sure.  r=jtv.
<henninge> jtv: thank you!
<henninge> jtv: btw, this is how a failure looks, which I think comes out nicely now:
<henninge> AssertionError: not equal:
<henninge> a = [(u'subdir/foo.pot', 'generic-string1')]
<henninge> b = [(u'subdir/foo.pot', 'generic-string1'), (u'subdir', ''), ('', '')]
<jtv> henninge: not surprising.  :)  Yes, looks good.
<allenap> Hello gmb, fancy a very very small branch to kick the day off?
<gmb> allenap: Sure.
<allenap> https://code.edge.launchpad.net/~allenap/launchpad/itch/+merge/12576
<allenap> gmb: Thank you :)
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: []  || This channel is logged: http://irclogs.ubuntu.com/
<gmb> allenap: Land it :)
<allenap> gmb: Thanks :)
<adeuring> gmb: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-438169-parse-sysfs-attr/+merge/12577 ?
<gmb> adeuring: Certainly.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com/
<adeuring> gmb: thanks!
<gmb> adeuring: Looks like you've committed at least part of  a conflict in lib/canonical/launchpad/scripts/hwdbsubmissions.py (the <<<<<<< TREE line is there in the diff)
<adeuring> gmb: Ah, right; there are conflicts with a brnach I submitted yesterday.
<gmb> adeuring: In fact it looks like the entire conflict has been committed.
<gmb> adeuring: Okay. Please fix that and paste a diff; I'll review everything that's not conflicted.
<adeuring> gmb: let me see... I did not notice any conflict yersterday...
<gmb> adeuring: Argh, according to the MP test_hwdb_submission_parser.py has the same problem - unresolved conflicts have been committed.
<gmb> adeuring: I'll reject this mp and you can resubmit (otherwise the diff will never get updated)
<adeuring> gmb: ok
<adeuring> gmb: conflict resolved.
<gmb> adeuring: Cool. Please submit a fresh mp so that I get a clean diff.
<adeuring> gmb: I already resubmiitted the the MP
<gmb> adeuring: Oh, cool.
<gmb> Looking now, then :)
<adeuring> gmb: scrap that -- i did mot wait long enough after pushing the latest version of the brnach...
<gmb> Ha.
<gmb> adeuring: So, what's the situation... should I just merge your branch into a clean branch of devel and do this the old fashioned way or do you want to delete the old MP and create a new one?
<adeuring> gmb: I_m waiting thatthe latest version of the brnach appears on LP...
<gmb> adeuring: Ok.
<adeuring> I should perhaps simply delete the brnach and and push it again..
<gmb> adeuring: That might work, yes.
<gmb> adeuring: Or push --overwrite, but I'd favour the first option myself.
<adeuring> gmb: https://code.edge.launchpad.net/~adeuring/launchpad/bug-438169-parse-sysfs-attr/+merge/12581
 * gmb looks
<gmb> adeuring: Ah, that looks much saner.
<gmb> adeuring: To make that test nicer in test_hwdb_submission_parser.py you could use dedent() rather than removing all the indentation from the sysfs-attributes string.
<gmb> At the moment it looks quite ugly.
<adeuring> gmb: right; will do
<gmb> adeuring: Also in that test, the opening braces of dicts shouldn't be on a line of their own. Better to put them on the end of the line above.
<gmb> (I'm talking about the assertEqual() call here)
<gmb> adeuring: Other than that, r=me.
<adeuring> gmb: thanks!
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/
<cprov> gmb: accepting reviews ?
<gmb> cprov: Just about to grab some lunch, but I'll put it on the queue for when I return.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [cprov]  || This channel is logged: http://irclogs.ubuntu.com/
<cprov> gmb: https://code.edge.launchpad.net/~cprov/launchpad/bug-408528-ensurePerson-again/+merge/12583
 * gmb lunches
<cprov> gmb: thank you.
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [cprov, noodles]  || This channel is logged: http://irclogs.ubuntu.com/
<noodles775> Hi gmb or reviewer - can I pls get a js review for this one when you've time! https://code.edge.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: - || queue: [cprov, noodles]  || This channel is logged: http://irclogs.ubuntu.com/
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, noodles || queue: [cprov]  || This channel is logged: http://irclogs.ubuntu.com/
<bac> hi noodles775
<noodles775> hi bac!
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: cprov, noodles || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> hi noodles775.  i was confused by the MP you submitted.  it looks like it has already been reviewed?  which are the new parts?
<noodles775> bac: it has, but salgado requested a javascript review (as he didn't feel confident enough). In the mean-time, I added js unittests and a windmill test (in addition to small changes suggested by salgado)
<gmb> cprov: Do you want me to leave that review to sinzui?
<gmb> Or am I looking at the wrong branch?
<cprov> gmb: no, please review it yourself, I will just pass it through curtis if he has time
<gmb> cprov: Okay, looking now - we are talking about bug-408528-ensurePerson-again, right?
<cprov> gmb: yup
<gmb> Cool.
<cprov> gmb: prepare your brain to be melted ...
<gmb> Oh, fun...
<bac> noodles775: so is this review a JS review specifically?
<noodles775_> bac: yep. The rest is already reviewed.
<noodles775_> bac: but of course, is you see something you think should be improved...
<noodles775_> s/is/if
<bac> noodles775_:  i think i'm going to follow salgado's lead and defer to someone with more JS experience
<noodles775_> bac: ok, pop it back in the queue... perhaps gmb can have a look (or you guys could swap?)
<gmb> noodles775_ I'll try when I've finished with cprov's branch.
<gmb> If I have a brain left.
<noodles775_> heh, ok! Thanks.
<gmb> cprov: r=me, pending sinzui's approval, too.
<cprov> gmb: super! I will address Curtis comment. Thanks
<noodles775_> BjornT: I just wrote our first windmill test using the new layer - you've made writing windmill tests about 100x more fun :)
* noodles775_ changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: cprov, - || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: noodles, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<gmb> noodles775_: Argh, I'm only just getting to your branch, sorry. Taking a look now.
<gmb> noodles775_: |Er... where's the MP; I don't see it on activereviews.
<noodles775_> gmb: https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035
<noodles775_> thanks.
 * gmb looks
<gmb> noodles775_: Wouldn't it be better to use "if (! Y.Lang.isValue())" than checking for isNull?
<gmb> Or are you expecting undefineds?
<noodles775_> gmb: just on a call, I'll be there in a tick.
<gmb> noodles775_: Okay, no rush. I'll grab a drink whilst I wait.
<noodles775_> gmb: hmm, so Y.get returns null if the thing you're trying to get doesn't exist, so Y.Lang.isNull seems more straight forward to me?
<noodles775_> gmb: in fact, I'd only use !Y.Lang.isValue() if I was expecting undefineds (which I'm not here) right?
<gmb> noodles775_: Well, you'd generally use it as a belt-and-braces. If you're not worried about it then fair enough.
<gmb> Just checking.
<adeuring> gmb, bac: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-438734-parse-kernel-release-nnode/+merge/12589 ?
<bac> adeuring, gmb: i will
<adeuring> bac: thanks!
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: noodles, adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<gmb> noodles775_: The comments at the top of the tests should describe the behaviour that the test expects. At the moment they seem to be descriptive of the line the immediately precede.
<gmb> Also, some of the tests lack comments.
<noodles775_> gmb: Yes, I initially had comments there, but then realised that I was duplicating the same string used in the assert?
<noodles775_> Is there a reason for having that initial comment describing the behaviour the test expects when it's also in the assert output? I wasn't sure.
<gmb> noodles775_: Well, I'd change the Assert errors to read more like "X should happen (when Y)" and have the leading comments as "X happens when Y". The comment is for the reader, whereas the Assert message is for the developer who sees a failure (e.g. "X should occur when Y, but it didn't.")
<gmb> noodles775_: The advantage of the comments is that you don't have to read the code.
<gmb> (Unless you want to)
<noodles775_> gmb: ok - it seems like duplicating (virtually) the same info, but I can do that :)
<gmb> noodles775_: Two different sets of stakeholders. The person reading the test failures isn't neccessarily the same person who's looking at the code. And the person who's looking at the code doesn't want to parse it when he doesn't have to.
<gmb> Especially if that person is me.
<noodles775_> gmb: yep, np. I personally just don't see it as any easier to parse the comment than the assertion string in such small tests, but consider it done :)
<gmb> noodles775_: Cool.
<gmb> noodles775_: Other than that, r=me.
<noodles775_> Great, thanks!
 * gmb goes off review so that he can catch up on other work.
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> adeuring: done
<bac> bye gmb
<adeuring> bac: thanks!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> bac: Got time to review a tiny change? https://code.edge.launchpad.net/~allenap/launchpad/ec2-niceties/+merge/12593
<bac> allenap: yep
<allenap> bac: Thanks.
<bac> allenap: no, thank you!  i look forward to a quieter ec2 experience
<bac> allenap: hey, while i have you held hostage, did you see my bug about buildbot and UTC?
<bac> allenap: r=bac, btw
<allenap> bac: Thanks :)
<bac> allenap: gah, does this mean you're ignoring me?
<allenap> bac: I think I did, but I can't remember it now.
<allenap> bac: Of course I'm not ignoring you :) I'm just taking a long time.
<bac> ok, well when you have a chance to look into it i'd be curious to know if it is doable/easy.
<bac> my head  almost exploded last week in a call trying to juggle UTC, BST, and EST
<allenap> bac: BST is easy for me ;)
<bac> allenap: cool, i'll nominate you for release manager
<allenap> bac: Actually, I'll start work on this bug soon. It seems like a good place for me to get into buildbot.
<allenap> bac: On second thoughts....
<BjornT> bac: can you do a quick review of this? https://code.edge.launchpad.net/~bjornt/launchpad/windmill-1.3beta/+merge/12601
<bac> sure BjornT
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: bjornt || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<BjornT> bac: i did this change just now to clarify things further: http://pastebin.ubuntu.com/281491/
<bac> BjornT: i'm a little confused.  where will it find that version of windmill?
<BjornT> bac: in download-cache. i've already added it there, so you can test it if you want
<bac> BjornT: ah, right.  (lightbulb goes off)
<bac> BjornT: this looks good to me.
<BjornT> thanks
<al-maisan> hello bac, how are things?
<bac> hi al-maisan
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<al-maisan> Could you please review a branch of mine?
<bac> al-maisan: sure.
<al-maisan> It's here: https://code.edge.launchpad.net/~al-maisan/launchpad/unembargo-email-316488/+merge/12603
<al-maisan> bac: thanks
<bac> al-maisan: i'll get on it shortly when the diff arrives
<al-maisan> bac: thanks, that's fine :)
<al-maisan> hmm .. the diff calculation for merge proposals seems not to be working correctly ..
<al-maisan> bac: the actual diff is here: https://pastebin.canonical.com/22758/
<bac> thanks muharem
<bac> al-maisan: i wonder why the diff is different.  did you target the wrong branch?
<al-maisan> bac: maybe
<al-maisan> I branched from devel
<al-maisan> should I be branching off db-devel ?
<bac> al-maisan: no, devel is right, AFAIK
<bac> al-maisan: please use paste.ubuntu.com so everyone has access.
<bac> al-maisan the code changes look great.  thanks for the nice branch.
<al-maisan> bac: thanks :)
<al-maisan> will use paste.ubuntu.com in future
<leonardr> bac, can you review this lazr.restfulclient branch?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/basic-auth/+merge/12612
<bac> leonardr: sure
<bac> leonardr: why is that branch private?
<leonardr> not intentionally
<bac> i figured not.  odd.
<leonardr> you should be able to see it now
<bac> leonardr: i can see it but was curious about the hash marks on the left, the privacy indicator
<bac> leonardr: on the MP i mean
<leonardr> maybe that project's branches are private by default
<bac> leonardr: the default policy is public but branches by ~launchpad are private.  i think that should be changed unless there's a good reason
<bac> nice job leonardr.  r=bac
<leonardr> great
<bac> hi barry, are you really here?
<barry> bac: kinda sorta
<barry> bac: what's up?
<bac> barry: not much.  just seeing if you are alive
<barry> bac: ibuprofen is a wonderdrug
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
#launchpad-reviews 2009-09-30
<al-maisan> Good morning
<noodles775> gmb: I'm just updating those comments in the js-test - you're right, I can see how it's now much easier to read as a dev. :)
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> allenap: I'll have one ready for you in 5 :)
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: Jolly good :)
<noodles775> allenap: thanks! https://code.launchpad.net/~michael.nelson/launchpad/432152-orig-file-counts/+merge/12645
<allenap> noodles775: Cool.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: noodles || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: I don't understand the query really. It reads like you're selecting distinct LFAs.filename where the corresponding content is the same size.
<allenap> noodles775: I don't understand how that can be a valid thing to ask for, so maybe that's explained by the other clauses in the query.
<noodles775> allenap: I'm just wanting distinct filenames to then sum the size - it just so happens that when the lfas with the same filename will also have the same size.
<noodles775> allenap: the unit test might help to demo what it's doing?
<allenap> noodles775: I should probably read it :)
<allenap> noodles775: If I apply http://pastebin.ubuntu.com/282003/ to your branch, the tests still pass (note the differing filecontent). Should that happen?
 * allenap will be back in ~5-10 minutes.
<noodles775> allenap: yes, that's correct.
<noodles775> allenap: it should fail if you change the name.
<noodles775> allenap: ah, I see... the filecontent is the actual content, not the length, so if you use filecontent='222' (ie. 3 bytes) it should also fail.
<allenap> noodles775: When does it not matter if the content is different but the size the same?
<noodles775> allenap: the query is only checking the filesize - not the filecontent.
<noodles775> For publishing tarballs/dsc files, (according to cprov), the fact that the filename is the same is enough to ensure that they are the same file (in that archive).
<allenap> noodles775: Ah, okay, that's what I was looking for :)
<noodles775> But it might make it clearer if we also include the LFC.sha1 in the query, so it's obvious that that's what's happening (I tried to clarify that in the comment, but it's still confusing).
<allenap> noodles775: Yeah, including the sha1 will probably document this better, and will also mean that future developers don't suspect a smell when there isn't one because of the data model.
<noodles775> allenap: great, I'll do that then.
<allenap> noodles775: Thanks, r=me, with one other suggestion, which you are welcome to ignore if you don't like it.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Thanks allenap !
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<stub> allenap, jtv: https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/12650 , https://code.edge.launchpad.net/~stub/meta-lp-deps/updates/+merge/12322 , https://code.edge.launchpad.net/~stub/launchpad/sanitize-db/+merge/12520
<stub> (in order both smallest to largest and in priority too)
<jtv> just when the lunch gong goes...
<jtv> allenap: shall I take that last one then?
<allenap> stub: Is the first the highest or lowest priority?
<stub> first is highest since it might need to be cherry picked.
<allenap> jtv: Perhaps you should do the first then.
<jtv> allenap: okay
<allenap> jtv: I am about to go to lunch too :) But I'll pick up as soon as I'm back.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue: [stub, stub, stub]  || This channel is logged: http://irclogs.ubuntu.com
<allenap> Sorry stub, looks like both of us are afk now. Nothing to do with you, well, on my part at least :)
<stub> k
<thumper> sinzui: https://code.edge.launchpad.net/~thumper/launchpad/rehash-branch-index/+merge/12651
<jtv> stub: I'm surprised to hear that the librarian-gc branch fixes anything (unless content_id is None as well), since AFAIK None < anything
<stub> None < some_integer == True
<stub> We don't want that test to pass if next_wanted_content_id is None
<jtv> stub: oic
<stub> Or we would be reporting 'LibraryFileContent None exists in the database', or we would if it didn't crash when trying to format None as an integer.
<jtv> stub: I can't help feeling that that loop would be a lot prettier with the config check outside it, but if this is for CP...
<jtv> It would be good to have a test though.
<stub> jtv: Hmm.... I don't see that working as I still need to lookup the next wanted id even if there is an upstream librarian
<stub> I was hoping nobody noticed the lack of test ;)
<stub> Now I have to work out what actually triggers this case :)
<stub> I think it occurs if the highest id isn't wanted
<jtv> it can avoid disappointments...
<stub> (Hmm.... that should be a generator really)
<jtv> stub: to get me into the right context: is this a CP candidate?
<stub> jtv: Yes
<stub> I can try adding a test in a minute
<jtv> stub: I'd appreciate that
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue: [stub, stub, stub, noodles]  || This channel is logged: http://irclogs.ubuntu.com
<stub> Oh sod off. I know there is another mp for this branch.
<stub> facist review system :-P
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: â, stub || queue: [stub, stub, noodles]  || This channel is logged: http://irclogs.ubuntu.com
<stub> Can http://paste.ubuntu.com/282095/ be reviewed without a mp ?
<jtv> stub: you could stick a tinyurl on the queue
<stub> np. worked out how to do the resubmit.
<stub> https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/12653
<jtv> stub: that second branch has a review request for Francis... do I take it you don't want to wait for that?
<stub> If you know anything about debian packaging, sure.
<jtv> stub: rusty, but this looks pretty trivial.  Why don't you tell me what could go wrong?
<stub> I just assigned francis as he suggested this workflow and it sat there for a few days ignored ;)
<stub> jtv: I have no idea what could go wrong. I don't even know what happens after it gets merged in to build the package ;)
<jtv> stub: well my apt has no objections to installing memcached, so you've got my imprimatur
<jtv> stub: any progress on test?
<stub> jtv: Nah - reject it and I'll resubmit when I have something
<jtv> stub: as you wish
<noodles775> allenap, jtv: just in case you get to it before I'm back, my MP is here: https://code.launchpad.net/~michael.nelson/launchpad/436182-newlines-in-sources-bz2-indices/+merge/12655
<allenap> noodles775: Cool.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub, stub || queue: [stub, noodles]  || This channel is logged: http://irclogs.ubuntu.com
<jtv> noodles775: in call
<jtv> will get back later
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub, â || queue: [stub, noodles]  || This channel is logged: http://irclogs.ubuntu.com
<deryck> I want a â
<deryck> allenap, hi
<allenap> deryck: Hi
<deryck> allenap, I have a small branch, but can I leave it on the queue for offline review.  About to return to tl sprint stuff.
<allenap> deryck: Okay, cool.
<deryck> allenap, thanks!
<deryck> allenap, never mind.  gpg fail on new laptop setup.  will catch up later about it.
<allenap> stub: Does ALTER TABLE not implicitly commit a transaction?
<allenap> deryck: Heh :)
<stub> allenap: No. PG is entirely transactional that way.
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub, â || queue: [stub, noodles]  || This channel is logged: http://irclogs.ubuntu.com
<jtv> allenap: which one for stub is still on the queue?
<allenap> jtv: https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/12653 I think
<jtv> allenap: ok
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stubÂ² || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
<jtv> stub, about the memcached test: why not self.assertEqual(self.want_memcached, is_live)?
<stub> jtv: Better error messages
<jtv> I guess...
<jtv> Something that bugs me about layers: can't we have split-transaction startup & teardown?
<stub> What do you mean?
<jtv> So e.g. memcached can start gearing up before Librarian has completed its startup, or vice versa.
<jtv> Put those extra cores to work.
<stub> jtv: Not out of the box.
<jtv> Shame.
<jtv> On a different note: in Memcachedlayer.setUp, you assign test_key twice, sort of defeating the purpose.
<stub> jtv: We could do it ourselves if we wanted - launch stuff in separate threads and block until they are all ready, but nobody has tried that yet.
<stub> I did?
<jtv> Well somebody did
<stub> fixed
<jtv> stub: http://paste.ubuntu.com/282095/ lines 150 & 168
<jtv> I'm not a huge fan of firing off threads for this sort of thing, since it gets hard to manage.  But why sit around looping for your service to become available when you could just return and let the next service start?  Managing dependencies is a bitch, of course.
<stub> I wouldn't want to bother until we migrate to something superior to layers anyway - no point making the change harder than it already is.
<jtv> something I don't understand about your atexit handler: how does its default argument get handled?  How does it avoid dereferencing MemcachedLayer._memcached_process if that's already been torn down?
<jtv> review meeting in 10...
<stub> jtv: The default value for the pid argument is evaluated when the function is defined.
<stub> (Not when the function is invoked)
<jtv> stub: Ah.  Also, since the atexit handler is one of those things you won't want to test, can you share a bit more of the killing code between the two places where it's written?
<stub> What would you share?
<stub> I'd like the tearDown to raise exceptions when things screw up. The atexit is just a second chance of not leaving random processes running if things go wrong.
<jtv> hmmm... yeah, not much to share
<jtv> only "if pid is not None: os.kill(pid, signal.SIGKILL)"
<jtv> It would make tearDown a bit nicer to read though, since you'd be rid of the line break in the "if" condition.
<stub> I personally prefer that than the extra indirection
<jtv> ok, no particularly strong feelings.
<jtv> stub: oh, it's not in the pastebin but above Memcachedlayer.launch you had a double blank line.
<jtv> Whereas one is missing above MemcacheClientTestCase
<jtv> stub: are you in the reviewers meeting?
<stub> Which file is that?
<stub> nah
<jtv> lib/lp/services/memcache/tests/test_memcache_client.py
<stub> fixed
<jtv> stub: ok, then r=me
<stub> Ta. Now if I can work out how to get lp-dev-deps rebuilt I might one day be able to land it ;)
<jtv> I _would_ say "let the Foundations people worry about that," but...
<allenap> stub: In line 194 of http://paste.ubuntu.com/282095/ it should be is *not* None I think.
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: noodles, stub || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: noodles, stub || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<stub> allenap: yup. fixing.
<allenap> stub: Cool. I commented on the bug too, just in case you'd gone, not because I like to harass you ;)
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: noodles, â || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, â || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<jtv> allenap: I've got a pretty urgent MP myself... shall I take it?  Or chuck it on the queue?  :-)
<jtv> oh, EdwinGrubbs maybe?
<allenap> jtv: I'm doing noodles' review now, but I can push yours on the stack.
<EdwinGrubbs> jtv: which review is it?
<jtv> allenap: stack?  Not queue?
<jtv> EdwinGrubbs: redo-uploads
<allenap> jtv: I'm a stack based machine.
<jtv> uh-oh
<allenap> jtv: Don't worry. I rarely overflow.
<jtv> tmi dude, tmi
<jtv> EdwinGrubbs: this one: https://code.edge.launchpad.net/~jtv/launchpad/redo-uploads/+merge/12659
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, deryck || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<EdwinGrubbs> jtv: ok, I'll do it.
<jtv> EdwinGrubbs: thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, â, jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, deryck, jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<jtv> topic serialization failure
<jtv> deryck: what if a bug summary is a full sentence?
<jtv> deryck: I sometimes paste an entire error message into a bug search, for instance.
<allenap> noodles775: In bug 436182, it says "there's a superfluous newline after libuno-cli-cppuhelper1.0-cil", but it's actually after the comma that follows libuno-cli-cppuhelper1.0-cil. Could that mean that it's really a leading newline in libuno-cli-ure1.0-cil?
<mup> Bug #436182: Newlines in Sources.bz2 indices <oem-services> <soyuz-upload> <Soyuz:Triaged by michael.nelson> <https://launchpad.net/bugs/436182>
<jtv> deryck: Is "ValueError: The date you gave was in a timezone that is not divisible by 12 minutes, which somehow matters to some hypohetical piece of code." one of these?
<noodles775> allenap: that seems to be just the cause of the issue - what happens then is that we (soyuz) incorrectly parse it and add a newline to the end of the field (dsc_binary in this case), as demo'd in the test.
<noodles775> allenap: cprov did a separate query showing that the relevant SourcePackageReleases have a '\n' appended to the end of the dsc_binaries field. We should add that to the bug.
<deryck> jtv, I think that's ok.  That's why I used quotes.  It's hard to echo on the page nicely, and beuno did sign off on the layout, maybe he has ideas.
<beuno> deryck, maybe "Is this the same as: $summary"
<beuno> and no quotes?
<deryck> beuno, but what is "this"?
<allenap> noodles775: This change will affect every field in the index stanza. That sounds like a good bit of belt-and-braces engineering. Might even be worth .strip()ing, not just .rstrip()ing.
<beuno> deryck, "Is this bug the same as: $summary?"
<noodles775> allenap: I initially had it just on the dsc_binaries, but then chatted with cprov and we decided to apply it generally to all the fields.
<deryck> beuno, but it would be more accurately, "Is one of these bugs the same as:  $summary"  which is long, I think.  but I'm not picky.
<noodles775> allenap: we've only encountered the problem on dsc_binaries, but it's the same parsing function that parses all the fields.
<deryck> beuno, your needed in our room at 4.
 * deryck has to have coke before the next meeting, sorry
<cprov> allenap, noodles775: and debian policy states it clearly ... http://www.debian.org/doc/debian-policy/ch-controlfields.html
<noodles775> allenap: and it's important that it's only rstrip() (imo), as there can be other single line-breaks within the fields values, just not at the end.
<cprov> Many fields' values may span several lines;''
<beuno> deryck, I will be there in 5ish
<jtv> deryck: you're good to go
<allenap> cprov, noodles775: Cool, thanks. r=me
* allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: -, deryck, jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* jtv changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: â, jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<jtv> allenap, EdwinGrubbs: sneaking out
<jtv> I'll still be here for a while, but not looking for new reviews to do.
<noodles775> Thanks allenap !
<deryck> jtv, thanks!  Sorry I couldn't be more attentive to review.
<jtv> deryck: you're sprinting, no worries.  FWIW I did check that the text gets HTML-escaped, and it does.  Just in case.  :)
<deryck> jtv, thanks! :)
<EdwinGrubbs> jtv: I don't understand where I go to 'pick a source package in "main" for some Ubuntu release'.
<jtv> EdwinGrubbs: https://translations.launchpad.dev/ubuntu/hoary/+source/evolution should do it
<jtv> for a dev system
<EdwinGrubbs> jtv: When I run reupload-translations.py -s hoary -p evolution
<EdwinGrubbs> jtv: I get this: WARNING Found no translations upload for evolution in Ubuntu Hoary.
<jtv> EdwinGrubbs: that's normal
<jtv> it seems none of these packages have translations uploads in the sample data.  :/
<allenap> leonardr: Is this proposal - https://code.edge.launchpad.net/~leonardr/launchpadlib/uses-restfulclient/+merge/5348 - still in need of review?
<leonardr> allenap, no, it's long merged
<allenap> leonardr: Okay, I'll change the status. Thanks.
<EdwinGrubbs> jtv: what does this mean in TestReuploadScript? "INFO\s*Processing [^\s]+ in .*\n"
<allenap> gmb: You want someone to review ajaxify-branch-linking?
<jtv> EdwinGrubbs: a log message that looks like INFO    Processing <something>
<jtv> EdwinGrubbs: the package names are arbitrary thanks to the factory, and I don't want to rely on the ordering even if they weren't.
<gmb> allenap: No, Tom's already reviewed that; waiting for a child branch to be ready for reviewing, but I've been sidetracked again.
<gmb> this is going to be one of those I-only-land-one-branch weeks, I think.
<allenap> gmb: Okay, cool.
<gmb> allenap: Thanks for reviewing that trac-launchpad-migrator branch. I'd forgotten it even existed.
<allenap> intellectronica: Could you mark https://code.edge.launchpad.net/~gmb/launchpad/ajaxify-branch-linking/+merge/12514 as reviewed by you, so it disappears off the "Requested reviews I can do" bit on +activereviews please? :)
<gmb> allenap: We need an nag email for merge proposals, I think...
<intellectronica> allenap: sure thing
<allenap> gmb: Yeah, I noticed it when I started using launchpad-project/+activereviews. Got a bit of a shock!
<allenap> gmb: Are you getting sidetracked by checkwatches?
<intellectronica> maaan, the edit button still evades me whenever i go to this page :)
<gmb> intellectronica: You're not the only one.
<gmb> I think it should still be next to the status myself, but maybe that should only happen when status editing is ajaxified.
<allenap> intellectronica: Thanks!
<adeuring> allenap: EdwinGrubbs: could one of you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice/+merge/12669 ?
<allenap> adeuring: I don't think I'll have time today.
<adeuring> allenap: np
<allenap> sinzui: Are you able to land https://code.edge.launchpad.net/~sinzui/launchpad/sprint-is-physical/+merge/12026 now?
<allenap> sinzui: I'm just doing some late late spring cleaning in +activereviews.
<allenap> EdwinGrubbs: Are you reviewing https://code.edge.launchpad.net/~jtv/launchpad/bug-436426-2/+merge/12610?
<EdwinGrubbs> jtv: Why are only uploads in the ROSETTA_TRANSLATIONS format re-uploaded? What other formats are there?
<jtv> allenap: he's doing my other one
<EdwinGrubbs> allenap: no, I'm reviewing the redo-uploads branch
<jtv> EdwinGrubbs: the other formats are Soyuz stuff unrelated to Translations.  "Format" looks to be a bit of a misnomer IMHO
<EdwinGrubbs> jtv: It also seems that there is no test which exercises the for-loop which calls addOrUpdateEntriesFromTarball().
<jtv> EdwinGrubbs: it has to do with the way a single source upload spawns various work items making their own respective ways through the Soyuz work queue.
<allenap> EdwinGrubbs, jtv: Okay, thanks. I don't actually have time to review it so I'm not sure why I was asking now!
<jtv> EdwinGrubbs: you mean, before I exercised it in this branch?
<jtv> allenap: that'll teach you  :-P
<jtv> EdwinGrubbs: nm, I misread
<jtv> EdwinGrubbs: you're talking about _processPackage, right?  That's called from main(), and is exercised in test_processPackage
<sinzui> allenap: The branch bounced many times. mthhadon fixed db-devel a few hours ago. (It was stuck in release-critical)
<allenap> sinzui: Okay. I'll mark it as approved then so it drops out of the reviews list, if that's okay.
<sinzui> oh. thanks
<EdwinGrubbs> jtv: ok, so that test would not have output like "WARNING\s*Found no translations upload for" if you were testing the output?
<EdwinGrubbs> jtv: so, is this script just going to be run once manually for karmic and probably never run again unless there is another bug?
<jtv> EdwinGrubbs: right on both counts
<jtv> want me to inject a mock logger and test the output?
<jtv> (actually there's easier ways to check: uploadless_packages)
<jtv> EdwinGrubbs: oh, I already check that uploadless_packages is empty there.
<jtv> Packages are appended to that in tandem with producing that log message.
<jtv> But the test needs a comment!
<jtv> Done.
<EdwinGrubbs> jtv: and I assume that summarize_translations_queue() ensures that the main() actually processed something, so an empty uploadless_packages couldn't be because nothing was processed?
<EdwinGrubbs> jtv: merge-conditional
<jtv> EdwinGrubbs: it summarizes what's on the translations queue for that package.  Since the package was just created, it was previously empty.
<jtv> The test verifies that _processPackages did upload files to the queue.
<jtv> So a pretty thorough test; not just the script's own opinion.
<jtv> EdwinGrubbs: thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, â, -|| queue: []  || This channel is logged: http://irclogs.ubuntu.com
<EdwinGrubbs> jtv: did you want me to review your other branch when I get back from lunch? bug-436426-2?
<jtv> EdwinGrubbs: if you have time, yes please; but not as urgent.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: jtv || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> EdwinGrubbs: ping
#launchpad-reviews 2009-10-01
<stub> EdwinGrubbs: https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/12695 if you are still there.
* stub changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<al-maisan> Good morning!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: stub || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> stub: hi! Just looking at your MP... would it be worth in addition creating a separate condition to log a warning for this situation where the file exists on disk but not in the db?
<stub> That is actually normal - client.addFile(foo,...); transaction.abort() leaves a file on disk
<noodles775> OK - I assumed that the transaction is only aborted in abnormal circumstances.
<stub> Nah - anytime a web request needs to be retried after uploading a file for instance.
<noodles775> k. great.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<stub> noodles775: test_deleteUnwantedFiles_bug444 or test_delete_unwanted_files_bug444 ?
<stub> noodles775: The former matches the existing test method names more closely, the latter is correct as the function being tested is called delete_unwanted_files()
<noodles775> stub: according the the test style guide, I thought the method name should *match* the real method name...
<stub> its a function being tested, not a method, and its name is delete_unwanted_files()
<stub> I'd probably go for the latter option myself, which I think is correct per the guide, and stuff the existing naming in that file (circa 2005 IIRC)
<noodles775> yep, so I'd use the latter too.
<stub> k
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: Fancy a simple branch? https://code.edge.launchpad.net/~allenap/launchpad/more-retest/+merge/12709
<noodles775> allenap: sure thing!
<allenap> noodles775: Thanks :)
<thumper> deryck: https://code.edge.launchpad.net/~thumper/launchpad/inline-lifecycle-status-edit/+merge/12707
<noodles775> allenap: nice - I hadn't seen takewhile before...
<noodles775> allenap: on line 97, why are you converting to a set rather than a list?
<noodles775> I mean, test output will not have duplicates...?
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: allenap || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: If multiple test logs are passed in (one of the new features), this removes duplicates.
<noodles775> allenap: then why not convert it back to a sorted list then and there?
<noodles775> ie., so extract_tests returns a sorted list of unique tests?
<noodles775> That's great that multiple logs can be used - and that it's now using a buildout template!
<allenap> noodles775: I only want them sorted to display them prettily, otherwise a set is what I want. Not much in it really.
<allenap> noodles775: As in, could be either a set or a list, but it happens to be a set because of de-duplication.
<noodles775> oh, I didn't see the set features being used anywhere... looking again.
<noodles775> Ah, gotcha. ok.
<noodles775> allenap: great, r=me... just note, there's one reference to the old utilities/retest in the usage instructions.
<noodles775> er, actually, two :)
<allenap> noodles775: Ah, good spot, thanks :)
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<adeuring>  noodles775: could you please review a sequel of my last MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-2/+merge/12713 ?
<noodles775> adeuring: ah great... I get to find out how the new HW character is introduced :)
<mrevell> noodles775: I have a branch fixing the problems we discussed: https://code.launchpad.net/~matthew.revell/launchpad/bugs-439909-439267/+merge/12710
<noodles775> mrevell: great, thanks for that!
<noodles775> mrevell: doing yours first as it's tiny :), I was just checking to see if there were any tests that might check the url for the contact-us link...
<noodles775> mrevell: and there aren't, but: http://pastebin.ubuntu.com/282928/
<noodles775> there's a few other places where we say 'Contact us' but send people to feedback@launchpad.net?
<noodles775> mrevell: I'll leave it up to you whether you think it's worthwhile making them consistent.
<noodles775> mrevell: r=me
<mrevell> noodles775: Yeah. The first of those, main-template, I believe is an over hang from the old design. I'll fix the other links. Thanks.
<noodles775> Great.
<noodles775> mrevell: and, of course, run it through ec2 first as there could be a test somewhere looking for the contact us url/href etc.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> mrevell: out of interest, did you find out what's behind the new version.txt file (ie. should it be updated automatically or at least as part of the rollout process?)
<mrevell> noodles775: sinzui didn't know where it had come from, but I'm going to ask beuno. I'm assuming it'll need a manual update.
<noodles775> hrm... well it was added with r9425 fwiw - check the commit msg there for more details.
<noodles775> adeuring: gee, that's an unfortunate diff...
<adeuring> noodles775: why?
<noodles775> adeuring: Nothing wrong with your code, I jut meant the huge segment of HALDevice that diff has assumed has been deleted and added.
<adeuring> noodles775: Ah, OK.
<noodles775> adeuring: r=me
<adeuring> noodles775: thanks!
<noodles775> adeuring: did you try using the revision spec '-r x..y' with bzr send so that the colour-coded diff on the mp is just the revisions you want? If not, might be good for the third installment :)
<adeuring> noodles775: I wasn't aware of that option. Thanks for the hint!
<noodles775> great, np!
<maxb> Is that bzr send -r ---> MP thing documented anywhere?
<noodles775> maxb: bzr help send
<beuno> maxb, https://help.launchpad.net/Code/ReviewEmail%20interface
<noodles775> at least, that documents that the option exists :)
<noodles775> ah, even better.
<henninge> Hi! I want to use a TAL formatter in a view and I found out I can do it this way: https://pastebin.canonical.com/22831/
<henninge> Is that the right way to do it? Or should I go through the registration somehow? I remember seeing an example once that was different ...
<henninge> jtv: ^
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
 * noodles775 finishes reviewing for the day.
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> noodles775: dang!
<barry> is anybody available to review a 279 line diff?
<noodles775> barry: there should be some others around soon... :)
<barry> noodles775: i guess not cprov-afk though :(
 * barry waits for rockstar and leonardr 
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<rockstar> barry, leonardr doesn't have a OCR day.
<leonardr> but, barry, if you want me to review something specific, i can
<barry> leonardr: no? you're listed on https://dev.launchpad.net/ReviewerSchedule
<barry> rockstar, leonardr it's xmlrpc and mailman.  don't fight over it :)
 * rockstar yells "Not it!"
<rockstar> :)
<barry> :-D
<rockstar> barry, I'll take it.
<barry> rockstar: thanks!  sending the mp now
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: barry || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<leonardr> barry: that wiki page is probably from when i was being mentored
<barry> leonardr: right.  you're not doing ocr anymore?  do you want to remove your name from that list?  or ic an do it.
<leonardr> barry: i'm doing it now
<leonardr> (removing my name--i'm not doing ocr, i went through mentoring because it was ridiculous to review lazr.* branches and then have to get someone else to do a 'real' review)
<barry> leonardr: sounds good.  rockstar sorry for the delay.  was otp.  finishing up the mp now
<rockstar> barry, yeah, I just looked for your proposal and couldn't find it.  Was about to bug you.
<barry> rockstar: sent
<bac> rockstar: can i add a MP to your queue?
<rockstar> bac, sure.
<bac> rockstar: cool, still waiting for it to show up
<rockstar> barry, did you request me as reviewer?
<barry> rockstar: oops
<barry> rockstar: done now
<rockstar> barry, I don't see it on lp reviews either.
<barry> rockstar: https://code.edge.launchpad.net/~barry/launchpad/403606-expat/+merge/12734
<bac> rockstar: https://code.edge.launchpad.net/~bac/launchpad/bug-422128/+merge/12735
* bac changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: barry || queue: [bac]  || This channel is logged: http://irclogs.ubuntu.com
<rockstar> bac, cool, thanks.
<rockstar> barry, I really only have one comment, and it's a nitpick really.
<bac> rockstar: i need to go get a samwich.  you ok with me being awol a bit?
<rockstar> barry, you use "Binarys" on line 59 of the diff.  I realize Binary is a class name, but it still looks funny.
<barry> rockstar: how about Binary's ? (kind of still icky)
<rockstar> bac, if you're not here when I get to it, I'll note my questions in the MP and you can address them when you get back.
<bac> rockstar: that is a sensible plan of action.  see you in a bit.
<rockstar> barry, even worse.  Bad apostrophe.  Maybe "Binary instances" or something.
<barry> rockstar: i like that!
<rockstar> I mean, it could probably stay as it...
<barry> easy to change
<rockstar> barry, other than that, it seems sane to me.
<barry> rockstar: thanks!
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/bzr-2.0.0-lp-1/+merge/12738 ?
<rockstar> abentley, yessiree
<abentley> rockstar: Thanks.
<rockstar> abentley, I'm assuming the actual source has landed in download-cache?
<abentley> rockstar: Yes.  There's not much involved in that.  Commit, push, wait.
<rockstar> abentley, alright, r=me
<abentley> rockstar: Thanks.
<abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/diffstat-count/+merge/12720 ?
<bac> thanks rockstar.
<rockstar> bac, you are welcome.
<rockstar> abentley, looks good.  r=me
<abentley> rockstar: Thanks.
<abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/move-linked-bugs/+merge/12736 ?
<rockstar> abentley, it doesn't look like you need the yui-u div around source revisions now.
<abentley> rockstar: quite possible.  It's all cargo-culted.
<rockstar> abentley, yeah, I figured.
<rockstar> abentley, so, source-revisions should be wrapped in a yui-g, but that's it.
<rockstar> abentley, other than that, r=me.
<abentley> rockstar: I'm confused.  Change the u to a g?
<rockstar> abentley, yea, and remove the first, or that may result in weirdness.
<rockstar> abentley, yui-g is a row, yui-u is a column
<abentley> rockstar: I miss tables.
<abentley> So do this and I can land it? http://pastebin.ubuntu.com/283191/
<rockstar> abentley, yeah.  Take a look at what the page looks like, but basically, yes, do that.
<abentley> rockstar: Oh great, now I have to push some branches to launchpad.dev
<rockstar> abentley, I just use the factory to make fake revisions.
<rockstar> Oh wait, that won't work for BMPs though, I don't think.
<rockstar> My computer hard locked a bit ago, so if someone was asking for a review, I haven't seen it.  Please repost.
<rockstar> Okay, so, I have to say this again just in case I missed something when my system locked again.  Did anyone request a review from me?
<rockstar> Also, desktop effects suck.
<leonardr> rockstar, can you do a quick review of https://code.edge.launchpad.net/~leonardr/launchpadlib/release/+merge/12746 ?
<rockstar> leonardr, sure.
* rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
#launchpad-reviews 2009-10-02
<al-maisan> Good morning!
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<jtv> henninge: care to review?  
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-440445/+merge/12777
<henninge> jtv: so you are assuming that it will also need access to language and translator?
<jtv> henninge: yes; it's looking at TranslationGroup to check for membership, and that definitely involves Translator.
<henninge> jtv: valid assumption, I guess.
<henninge> jtatum: r=me
<jtv> Not sure about Language, but it's one of those things that are likely to be fetched by the ORM as a side product
<jtv> henninge: no, the other person.  :)  Thanks.
<henninge> jtv: r=me
<jtv> :)
<henninge> jtatum: whatever branch you want reviewed, I have *not* approved it! ;-)
<henninge> jtatum: sorry to bother you ...
<henninge> jtv: please remember to set the mp to "approved". For some reason I cannot do it.
<jtv> henninge: sure, thanks
<al-maisan> hello adeuring -- could you please have a look at a small/simple branch?
<al-maisan> it is here: https://code.launchpad.net/~al-maisan/launchpad/cmsg-425800/+merge/12774
<adeuring> al-maisan: I'm already doing this :)
<al-maisan> adeuring: ah, wow!
<al-maisan> adeuring: I am duly impressed :)
<adeuring> al-maisan: there are two new tests: test_requestMirror_doesnt_demote_branch() and test_requestMirror_can_promote_branch(). Perhaps I'm missing something obvoius there -- but isn't future_time in the latter test a timestanp in the past: future_time = datetime.now(pytz.UTC) - timedelta(days=1) ?
 * al-maisan looks
<al-maisan> adeuring: argghh
<al-maisan> the diff generated for the MP is wrong
<al-maisan> just a minute
<al-maisan> adeuring: http://pastebin.com/m4c94727b
<al-maisan> sorry about that
<al-maisan> this is a *really* simple branch
<al-maisan> bbiab
<adeuring> al-maisan: Ah, indeed quite much shorter ;)
<adeuring> al-maisan: but the other test is there nevertheless ;)
<al-maisan> adeuring: aha
<al-maisan> adeuring: I don't see any of that in the diff I provided you with
 * al-maisan rubs his eyes
<adeuring> al-maisan: what I menat: I am simply curious about this test, even it is is not part of the patch.
<al-maisan> adeuring: yep, 'future_time' would have to lie in the past .. provided the time on the test machine is set correctly :)
 * adeuring is quite confused... future time in the past?
<adeuring> al-maisan: but let's put that aside.
<adeuring> al-maisan: al-maisan: just one question about the short diff: You removed the text "Did you mean to upload to a PPA?". Isn't this sometimes a useful hint for users?
<al-maisan> adeuring: not really .. it would rather confuse people
<adeuring> al-maisan: OK, so r=me
<al-maisan> adeuring: thanks
<allenap> adeuring: Fancy a devscripts.ec2test branch to review?
<allenap> https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-race-bug-422433/+merge/12778
<adeuring> allenap: sure
<allenap> adeuring: Thanks :)
<bigjools> al-maisan: don't remove that error text
<al-maisan> bigjools: alright; what do you suggest instead?
<bigjools> al-maisan: I don't have any context - what are you doing?
<al-maisan> bigjools: https://bugs.edge.launchpad.net/soyuz/+bug/425800
<mup> Bug #425800: confusing message if signer can upload packages from some package sets but not others <packagesets> <soyuz-upload> <Soyuz:In Progress by al-maisan> <https://launchpad.net/bugs/425800>
<bigjools> al-maisan: that error text is still correct, but only if the uploader has no packagesets
<bigjools> he can upload to
<al-maisan> bigjools: that's not quite right.
<al-maisan> this error message indicates that the uploader has no upload permissions whatsoever
<bigjools> al-maisan: yes, that's exactly what I mean
<bigjools> so the existing check for that error text needs to also check packagesets
<al-maisan> however, he may end up in this situation if he has *some* package set based permissions but is lacking one
<bigjools> no, that is wrong
<al-maisan> hmm .. I looked at the code .. the source package based permissions (including package sets) are checked before this error is raised
<bigjools> the error should be raised if the upload has no component-based *and* no packageset-based permissions
<al-maisan> is that error (formulated that way) useful in first place?
<bigjools> yes - it distinguishes between NO permissions and SOME permissions but the wrong ones
<al-maisan> aha
<bigjools> PPA packages fall foul of it a lot, hence the extra text
 * al-maisan takes another look at verify_upload()
<bigjools> al-maisan: basically if the uploader is trying to upload to say, main, but only has universe permissions, this error is not used
<bigjools> the same needs to apply with packagesets
<al-maisan> bigjools: I see what you mean.
<bigjools> great
<adeuring> allenap: r=me
<allenap> adeuring: Thanks :)
<adeuring> allenap: what I forgot to say iin the review: thanks for splitting the diff into two parts!
<allenap> adeuring: I'm glad it helped :)
<allenap> adeuring: I replied to your review of ec2-test-race-bug-422433 with an incremental diff. Could you sanity check it, then I'll send it all off to ec2? Thanks.
* barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> allenap: looks good. Thanks for the change!
<allenap> adeuring: Thanks.
<noodles775> barry, have you checked some bug pages also? The "Bug #440220 reported by Barry Warsaw 16 hours ago" is also using the registration slot iirc...
<mup> Bug #440220: registering slot interferes with watermark <post-ui-3-cleanup> <Launchpad Registry:In Progress by barry> <https://launchpad.net/bugs/440220>
<noodles775> which means that it will *always* wrap (unless you've got a 2m screen ;) ).
<barry> noodles775: right.  let me paste a bug page s/s
<barry> noodles775: attached two more s/s.  they do always wrap but it doesn't look too bad to me
<noodles775> barry, I think the thing I don't like is that it's wrapping when it doesn't need to.
<noodles775> Have you tried leaving it where it was after adding the no_wrap on the app-tabs?
<barry> noodles775: it will run into the tabs if the registering contents are long (long person name).  maybe that's rare enough to not worry about it though?
<noodles775> barry, if you give the registration slot a large left-margin (maybe 2em) it shouldn't get too close? (and will wrap since the app tabs can't, iirc?)
<barry> noodles775: e.g. this does not look as good to me: http://launchpadlibrarian.net/32862698/ff-nowrap.png
<barry> noodles775: that's a possibility too.  i kind of like that the registering information sits neatly above the portlet
<noodles775> barry, hmm.... to me it often (ie. with longer slots like on bugs pages) looks like it's been squashed into a space that's too small.
<noodles775> barry, I personally agree with rockstar's comment - if it was just below the app tabs, it wouldn't need to wrap at all, and would not interfere... but maybe there's a reason for not doing that?
<barry> noodles775: it is a pretty small space (21%).  
<barry> noodles775: i definitely want to explore that, but iirc beuno really wants that registering slot on the right side
<barry> noodles775: so i kind of wanted to run that to ground to see if we could make it work
<noodles775> barry, it would be on the right side wouldn't it? I just mean a little bit down from where it is in the last link above.
<barry> noodles775: hmm... possibly so.  give me a few minutes and i'll try that
<noodles775> k
<barry> btw, thanks!
<noodles775> np!
<barry> noodles775: take a look at the s/s in comments 13-16
<noodles775> barry, what do you think? My first thought is that the only thing that looks ugly about it is the super long username?
<barry> noodles775: agreed.  i just did that to see worst case.  this works for me
<noodles775> barry, great. Thanks for fixing it!
<barry> noodles775: thanks!  can i request you as one of the two ui reviewers for the branch?
<noodles775> barry, sure!
<barry> awesome
<barry> adeuring: ping
<adeuring> barry: yes?
<barry> adeuring: have time for a 39 line CSS review?
<adeuring> barry: sure
<barry> cool.  sending mp now
<adeuring> barry: I have also an MP (a bit larger than yours, but just trivial refactoring): https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 could you review it?
<barry> adeuring: sure!  this feels so incestuous though :)
<adeuring> barry: ;)
<adeuring> barry: r=me
<barry> adeuring: thanks!
* barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> adeuring: did you request me as a reviewer on your branch?
<adeuring> barry: no
<barry> adeuring: url?
<adeuring> barry: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733
<al-maisan> hello adeuring, can you please have a look at the fix for bug #425800 (2nd try) ?
<mup> Bug #425800: confusing message if signer can upload packages from some package sets but not others <packagesets> <soyuz-upload> <Soyuz:In Progress by al-maisan> <https://launchpad.net/bugs/425800>
<al-maisan> https://code.launchpad.net/~al-maisan/launchpad/cmsg-2-425800/+merge/12791
<adeuring> al-maisan: sure
<al-maisan> adeuring: thanking you :)
<barry> adeuring: when you raise NotImplementedError in your properties, you don't need to instantiate them if they have no arguments.  Python will DTRT for you here, and it's also somewhat more efficient
<adeuring> barry: Ah, thanks! didn't know that
<adeuring> (Though I hope the excpetion will never be raised ;)
<barry> adeuring: if the exception has arguments, instantiating it in place is the right thing to do though.  
<barry> adeuring: right! :)
<adeuring> al-maisan: r=me
<al-maisan> adeuring: thank you very much!
<adeuring> al-maisan: immer gerne :)
<al-maisan> adeuring: :)
<barry> adeuring: with that fix, r=me
<adeuring> barry: thanks!
* barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> rockstar: ping
<rockstar> barry, yo
<barry> rockstar: hiya!  could you do a quick second ui review on https://code.edge.launchpad.net/~barry/launchpad/440220-slot/+merge/12786
<rockstar> barry, sure.
<barry> rockstar: thanks!
<mrevell> barry: Can you take a look at small help branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-215798/+merge/12792
<barry> mrevell: sure thing
<mrevell> thanks barry
<barry> mrevell: did you forget to bzr add register-branch.html perhaps?  it's not in the diff
 * mrevell hangs head in shame
<mrevell> barry: Yup. Sorry. Pushing new version now
<mrevell> barry: done
<barry> mrevell: can you add the file as a comment.  i don't think the mp will update the diff
<rockstar> mrevell, I have bzr ci aliased to `bzr commit --strict` and I use that all the time.  It makes sure you don't have any rogue files laying around.
<barry> rockstar: +1
<rockstar> barry, it will update the diff.
<mrevell> barry: I thought we had moving diffs now
<mrevell> rockstar: thanks for the top!
<rockstar> barry, it updates the diff on push of the source branch now.
<barry> rockstar: \o/
<mrevell> s/top/tip
<rockstar> barry, abentley is pretty amazazing.
<barry> rockstar: he's amazing, too!
<mrevell> :)
<rockstar> barry, I was using a word from a movie, so the apparent typo was actually purposeful.
<barry> rockstar: oh!  what movie?
<rockstar> barry, it's from "The Benchwarmers"  The dialogue goes something like "You guys think you're ath-a-letes now?"  "I didn't know athletes had three syllables.  That's amazazing!"
<barry> mrevell: r=me
<mrevell> thanks barry
<barry> rockstar: guess i have to put that on my netflix now :)
<rockstar> barry, it's appropriate for Max too.
<barry> rockstar: great review quotes: "filled with sprays of vomit, fountains of spit and enough hot body air to launch a flotilla of passenger balloons" and "a movie packed with so many idiot characters that rob schneider is cast as the cool guy -- and sort of pulls it off"
 * barry clicks add to queue :)
<rockstar> barry, :)
* adeuring changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> EdwinGrubbs, rockstar ping
<EdwinGrubbs> barry: pong
<barry> hi EdwinGrubbs can you take a quick look at a testfix diff for me?
<barry> EdwinGrubbs: http://pastebin.ubuntu.com/284096/
<EdwinGrubbs> barry: is the facetmenu class have any stylesheets, or is it just a marker?
<barry> EdwinGrubbs: it has some css, but base-layout.txt doesn't test it.  it is a horrible horrible test, so this just repairs it in the easiest possible way
<EdwinGrubbs> barry: r=me
<barry> EdwinGrubbs: thanks!  testfix here i come
#launchpad-reviews 2010-10-04
* wgrant changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [wgrant(bug-653382-domination-series-restriction)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> henninge: Thanks. Can you ec2 that?
<henninge> wgrant: sure ;)
<wgrant> Thanks.
<bac> hi henninge, can you do a review for me?
<henninge> HI bac, sure.
<bac> henninge: cool.  it's in the usual place.  :)
<henninge> bac: you can help me with one, too. I was just looking at our style guide.
* bac changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [wgrant,wgrant,bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> henninge: i'll try
<henninge> bac: https://code.edge.launchpad.net/~wgrant/launchpad/bug-629921-packages-empty-filter/+merge/37339
<wgrant> I was wondering about that myself.
<wgrant> The style guide wasn't exactly clear.
<bac> henninge, wgrant: the style guide seems clear to me, though i'm not defending it in this case.
<henninge> bac, wgrant: Sorry, got distracted. Yes, that's exactly what I was about to ask ... ;)
<henninge> bac: I see the use of "any" a similar border case.
<henninge> e.g. I have been writing code lately like this "if translations is not None and any(translations):" to check for empty strings or none in translations.
<bac> wgrant: why do you care if requested_name_filter[0] is None or not?  why not just return requested_name_filter[0], after checking that it exists?
<bac> i mean:  if len(requested_name_filter) > 0: return requested_name_filter[0]
<bac> you return None when the test fails anyway
<wgrant> bac: I want to return None if it's "".
<wgrant> That's the whole point of the bug.
<bac> wgrant: well, yeah, there is that...  :)
<wgrant> Heh.
<bac> wgrant: then i think lifeless' suggestion is the cleanest, PSG be damned
<wgrant> Great, thanks.
<wgrant> Can someone please bless and land that, then?
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> wgrant: approved. I'll land both your branches.
<wgrant> Thanks henninge, bac.
<allenap> henninge: Would you be able to review my branch? I can't be here unfortunately, so please say no if that's a problem.
* allenap changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bac || queue: [allenap?] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> allenap: put it in the queue and I'll try. ;)
<bac> mrevell: can you do a text review of https://code.edge.launchpad.net/~bac/launchpad/bug-639703-pg-bugs/+merge/37463 ?
<mrevell> sure bac
<bac> thanks
<henninge> bac: You are creating a view purely to have it aggregate an "official_malone" property?
<henninge> bac: I think this is something the ProjectGroup class should do - and thus the tests should be for that class, too.
<henninge> bac: I mean, thre should be a ProjectGroup.official_malone property that loops through the product, as does the view now.
<henninge> s/product/products/
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: allenap, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> abentley: Hi!
<abentley> henninge: hi!
<henninge> abentley: I ran late and cannot start on Gavin's branch as I had intended. Can you take it, although he's not here?
<abentley> henninge: okay.
<henninge> He said it's ok if we don't do it because he's not here.
<henninge> abentley: thanks!
* henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> abentley: good bye ;)
<abentley> henninge: bye!
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> abentley: do you have time to look at an MP with literally one line change? https://code.edge.launchpad.net/~jcsackett/launchpad/messages-translated-652264/+merge/37508
<abentley> jcsackett: r=me
<jcsackett> thanks, abentley.
#launchpad-reviews 2010-10-05
<wallyworld> lifeless: ping
<lifeless> hey
<wallyworld> lifeless: i have put up for review an enhancement to allow logging of sql bind parameters, to make performance tuning easier
<wallyworld> https://code.edge.launchpad.net/~wallyworld/launchpad/log-sql-bind-variables/+merge/37552
<wallyworld> can you take a look and see if you agree with it?
<wallyworld> thnks :-)
<lifeless> I reviewed it already
<wallyworld> wow, that quick
<lifeless> no messing :)
<wallyworld> let me look and see if you approved :-)
<wallyworld> cool. thanks.
<bac> hi henninge
<henninge> Oh hi bac! You are ahead of me time-zone-wise, right ... ;-)
<bac> henninge: thanks for the review yesterday.  i know you think i'm on crack, but the hoo-ha with the involvement portlet was following a pattern already employed.
<henninge> bac: is crack cheap in vietman?
<bac> henninge: however, in light of your questions i'm having a hard time defending it.  :(
<henninge> bac: What would be the advantage of doing it that way?
<bac> henninge: well, the original thought was to not repeat the code.  but i cannot rationalize why it should be in a portlet rather than the model
<henninge> exactly ... ;-)
<bac> henninge: you'll see in answers and maybe translations the same thing is done
<henninge> well, it's never too late to mend our old ways. ;)
<henninge> I mean, there is a lot of old code in LP that neither you nor I would have written that way. Non need to continue the errors of others.
<bac> henninge: please don't joke about me + crack + vietnam.
<bac> :)
<bac> henninge: i completely agree
<henninge> ;)
<henninge> stub: Hi! Are you up for a database review?
<stub> henninge: I see a merge proposal containing nothing but sampledata rebuilt with PG 8.3 instead of PG 8.4
<henninge> stub: yes, I just realized that myself.
<henninge> the 8.3 vs. 8.4 diffference
<henninge> How come I am not running 8.4 locally?
<henninge> stub: also, I just added a diff for the other files that were changed.
* 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
<bac> henninge: after installing PG8.4 you still need to change your conf files so that 8.3 doesn't start and 8.4 does
<henninge> oh
 * henninge checks that 8.4 is installed ... should be
<bac> start.conf needs to have 'auto'
<jtv> henninge: even if they're both installed, you may still be running 8.3 on the expected port (5432).  Try running "psql"
<henninge> jtv: I understand and psql reports 8.3
<jtv> henninge: that's the version of psql itself, so that tells us you have 8.3 still installed.
<jtv> I'm trying to remember how to ask for the backend version.
<jtv> ah yes, "show server_version;"
<henninge> jtv: it is obviously 8.3 because the sampledata I generated reports 8.3
 * henninge tries that
<henninge> 8.3.9
<jtv> henninge: in /etc/postgresql/8.4/main/postgresql.conf, make sure that "port" is set to 5432.
<jtv> henninge: the package installation scripts will set it to 5433 if there's already another postgres version set to run on 5432.
<henninge> bac: start.conf in both 8.3. and 8.4 contain only one directive: "auto"
<henninge> jtv: will look at that
<bac> henninge: yes, you must remove the one from 8.3
<bac> henninge: unless you need 8.3 for some reason you can just blow away the 8.3 config directory
<jtv> Probably best to un-install 8.3 entirely, with --purge, yes.
<bac> henninge: and possibly run launchpad-database-config, or whatever it is called
<jtv> It's a bit of a trap that doing this, by default, will leave you with 8.4 running on 5433 and all connection attempts on the default port failing as if you have no backend running.
<bac> jtv: the script will take care of that
<jtv> Oh good
<StevenK> launchpad-database-setup
<henninge> jtv, bac: I am a bit unsure what to do: http://paste.ubuntu.com/506314/
<bac> henninge: would you mind having a look at PillarView?
<henninge> bac: sure
<jtv> henninge: try removing all of those that have "8.3" in them, but also installing their 8.4 equivalents in the same run.
<jtv> henninge: also, there's no problem with temporarily dropping the lp packages if that makes things easier.  The 8.3 stuff should go in any case.
<henninge> jtv: that's what I was more worried about
<henninge> I think I already have all the 8.4 stuff installed.
<jtv> henninge: then just uninstall this stuff and then re-install the lp packages.
<jtv> it'll probably come up with conflicts and two resolution scenarios, one using pg 8.3 and one using pg 8.4.
<jtv> (not that I'm too familiar with aptitude)
<henninge> jtv: I don't usually use it either. I just did not find the purge option on apt-get.
<jtv> apt-get remove --purge
 * henninge tries that
<henninge> jtv: much better! http://paste.ubuntu.com/506320/
<bac> henninge: i'm hesitant to unroll all of the PillarView logic and move it to the model without discussing it with sinzui.
<bac> henninge: i think i'd like to land what i have, repeating the existing pattern, and then fix them all together if that is what we decide to do
<henninge> bac: please do that. I did not think that it is that much.
<henninge> bac: yes, please file a bug about it.
<bac> rt
<henninge> rt?
<henninge> right then
<henninge> ?
<bac> "roger that"
<henninge> :-)
<bac> my friend the former helicopter pilot infected everyone with saying it
<jtv> bac: careful with thatâ¦ you may trigger an allergic psychotic reaction in losas
<henninge> stub: please look at the mp again. I'll request a db review from you on it if that's ok.
<henninge> stub: oh, nm. You already did ... ;-)
<stub> I already approved it didn't I?
<henninge> yes, I just noticed. Thanks!
<lifeless> I need a testfix review: https://code.edge.launchpad.net/~lifeless/launchpad/test/+merge/37571
 * henninge waits for diff to become available
<henninge> lifeless: simple enough fix, thank you, but can you please rename _setup to _is_setup or _is_set_up?
<henninge> lifeless: r=me
* noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> noodles775: I can trade you a review in a few minutes
<jtv> (well, it'll take more than "a few minutes" before you reap the benefits, but still :)
<noodles775> jtv: great!
<henninge> jtv: can you have a quick look at this code-wise first, please?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/db-devel-translationmessage-pofile/+merge/37563
<jtv> I think I can promise a quick lookâ¦
<jtv> 2749 lines of diffâ¦ which of those need my quick look?  :)
<henninge> jtv: 2456-2478
<henninge> :-P
<jtv> henninge: grrr truncated diff
<henninge> jtv: look in the comments!
<henninge> the rest is just removing lots of 'pofile' and 'NULL' from sampledata
<jtv> henninge: I have to _get_ at the comments firstâ¦
<henninge> ? They are at the top, above the diff, are they not?
<henninge> oh, you are going through the mails ...
<jtv> henninge: oh, the MP comments.  I thought you meant code comments.
<henninge> jtv: there is this https://launchpadlibrarian.net/57120537/review.diff and this https://launchpadlibrarian.net/57122440/inc.diff
<henninge> jtv: but you can see it all on the mp page.
<jtv> henninge: yes, I see
<jtv> henninge: wow, passing vars() to a % is pretty nastyâ¦ didn't know we did that.  For a doctest I guess it doesn't matter that much.
<henninge> jtv: Not using the factory here seems pretty out of place, too.
<henninge> But fixing that is out of scope for this db-oriented branch.
<jtv> henninge: ultimately most or all of our doctests should just go away and leave us in peace.
<jtv> anyway, approved.
<henninge> jtv: thanks
<jtv> noodles775: getting to your branch now
<noodles775> jtv: great, thanks. I've just reviewed yours.
<jtv> thanks
<jtv> noodles775: the "dynamic name for the action" you refer to is the text in the button at the bottom, right?
<noodles775> jtv: yeah, so that it displays "Sync selected Warty versions into Hoary" (with appropriate substitutions).
<jtv> gotcha
<jtv> I never thought about that; didn't think it'd have to come out of a vocabulary.
<noodles775> huh? the dynamic vocabulary issue is a separate one... what did you mean exactly?
<jtv> noodles775: ah, that makes more sense
<jtv> Just trying to parse your MP with a bit of a headache.  :)
<noodles775> urgh... hope it's not too painful.
<bac> thanks for the rewording suggestions, mrevell
<jtv> noodles775: no, I'll just take it out on some poor engineer in a review.
<noodles775> :P
<jtv> noodles775: it goes like thisâ¦ I thought point 2) in the MP detailed your approach to solving point 1), though there's nothing in particular to indicate that.
<bac> mrevell: should we really say "Sorry" though?
* bigjools changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, noodles775,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> noodles775: I'm not much at home in zope but istm what you want is an alternative to custom_widget that you can invoke from, say, __init__
<noodles775> jtv: well, the vocabularies are actually specified on the field, not the widget (the widget then grabs them from the field).
<noodles775> jtv: IMO, if zope had something like the IContextSourceBinder, IViewSourceBinder, that would work.
<noodles775> AFAICS, zope does allow a vocab factory to implement ISource instead, which I had thought in the past was a more general way that would allow what I needed, but I've never managed to get it to initialise from the view.
<noodles775> Hence, the solution I used in this branch.
<jtv> noodles775: this sounds familiar
<noodles775> I might ask one of the zope guys to take a look later when they are around, incase there is something I've missed.
<jtv> noodles775: smart move
<bac> mrevell: ping
<mrevell> hey there bac
<bac> mrevell: i have a counter proposal
<bac> howzabout:
<bac>  Sorry, you can't report a bug for the $ProjectGroup project group. None of the
<bac>  projects within the group use Launchpad for bug tracking. Please check the
<bac>  $ProjectGroup website for details of where they track bug reports.
<bac> argh, no, that's yours
<jtv> noodles775: it's really galling that you have to go into class attributes to get at the labelâ¦  I don't suppose you could somehow _easily_ set self.actions to a copy of self.__class__.actions and manipulate that instead?
<mrevell> haha, I thought it looked good bac
<bac> You cannot report a bug for <tal:name replace="name"/> as none of
<bac>             the projects within the group use Launchpad for bug tracking.
<bac>             Please check the individual projects for details
<bac>             of where bugs are reported.
<bac> a) we don't know if the project group has a web site
<bac> b) i didn't like starting off with "Sorry"
<bac> it is a shame but it isn't my fault so why should i apologize?
 * mrevell is British
<mrevell> that's all the explanation you need
<mrevell> bac: +1 on your version.
<mrevell> I like it
<bac> yes, and when i read that i heard it in your voice
<noodles775> jtv: agreed, but no, I didn't see an obvious easier way, hence my question on the MP. I'll take another look though.
<jtv> as a Dutch poet ones wrote about theBritish: "mylord's final word is: please excuse my rigor mortis"
<mrevell> :)
<jtv> s/ones/once/
<bac> thanks mrevell
<mrevell> You're welcome.
<mrevell> Or, as Americans seem to prefer, "Mmm hmm".
<jtv> noodles775: I wonder if it's possible to use a factory instead of a class for the view.
<bac> mrevell: also, i'm concerned about using the project display name in that sentence
<bac> mrevell: sometimes it will be "Mozilla" and others "Bazaar Project"
<mrevell> bac: Oh, because people put hard-to-parse things as their display name.
<jtv> (little Robbie Tables as we call him)
<mrevell> heh
<bac> mrevell: not hard to parse but sometimes it would need a 'the' and others not
<bac> wft do we call "the"?
<mrevell> article?
<bac> ah, yes, an article
<jtv> lovely language, English: it has "the" definite article but your choice of "an" indefinite article.
<bac> jtv:shush, we're working here
<jtv> noodles775: completely different thing meanwhileâ¦ ll. 74â79 doesn't format the list comprehension quite right.
 * jtv shushes
<noodles775> jtv: line 79 should be indented?
<bac> mrevell: in the project registration we do encourage them to end with "Project", so we should word for that?
<mrevell> bac: It's a little awkward but how about this? "You cannot report a bug for the project group '<tal:name replace="name"/>'"?
<jtv> noodles775: I think this one will become easier if you keep the list comprehension in a variable.
<jtv> noodles775: we're supposed to break the line after the opening bracket, not before.
<noodles775> Right, I missed that.
<bac> mrevell: yeah, that is awkward
<mrevell> bac: I suppose we don't have to explicitly state the project group name. *Presumably* the person knows which project group they're trying to report a bug on ... but I'm always a fan of being explicit because I know it's easy to confuse people when you're not.
<jtv> noodles775: in line 159 of the diff, s/syncs/sync/ ?
<noodles775> jtv: yep.
<jtv> (given how little I know about this, either way could have been valid)
<jtv> noodles775: I didn't know you could construct a BeautifulSoup out of a view.  That's neat.
<jtv> brb
<noodles775> Yeah, I forget where I saw it first, but it's handy for unit-tesing special case things in the template.
<henninge> bac: please push the latest revision of your branch. ;)
<bac> henninge: ok, i was just about to
<bac> henninge: why so pushy?  :)
<henninge> GTD ;-P
<bac> mrevell: i looked at how it was used in context other places in that template and just stuck with what i wrote originally
<mrevell> Okay.
<bac> henninge, mrevell: could  you update the MP, please?
<bac> henninge: what about the UI?  did you consider it as a UI reviewer or should i just run it past sinzui?
<henninge> bac: no, I have not but let me have a look.
<bac> henninge: for the MP you'll have to state "code/UI*" or something like that
<bac> which will no doubt confuse our tools
<henninge> bac: oh, I cannot do two different reviews?
<henninge> never tried it before
<bac> henninge: nope, it takes the latest type
<henninge> :(
<henninge> bac: I'll just comment on the ui, then.
<bac> henninge: i think a combined type would be better
<bac> fwiw
<henninge> even if it confuses our tools? ;)
<henninge> bac: what's the url for seeing and adding projectgroups?
<henninge> I forgot
<bac> henninge: we hid it at lp.net/projectgroups
<henninge> ah, I tried +projectgroups
<henninge> I sense a bit of inconsistency here ... ;)
<jtv> there's also /builders.  I keep typing /+builders instead.
<jtv> I wonder if I could call my project "builders"
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [jtv, noodles775,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> henninge: lp.net/people, lp.net/projects, lp.net/distros ...
<henninge> bac: then maybe t.lp.net/+translationgroups is the inconsistency?
<henninge> and +languages?
<henninge> I know, they are not pillars but do the users see the difference?
<henninge> bac: have a look at this, please:
<henninge> https://bugs.staging.launchpad.net/mailclipper
<henninge> nm, I'll post a screenshot
<bac> henninge: what is your question about mailclipper?
<henninge> bac: that's how uncofigured bugtracker looks on a project
<henninge> http://people.canonical.com/~henninge/screenshots/unconfigured_bugtracker.png
<henninge> and here is how an unconfigured blueprints looks on a project group:
<henninge> http://people.canonical.com/~henninge/screenshots/unconfigured_blueprints_pg.png
<henninge> bac: so your screenshots is at least missing a page heading and the breadcrumbs.
<henninge> but I also like how noodles (I think it was him) added those help links on the project page.
<jtv> noodles775: I approved your code.  Tried some more to find alternatives for the dynamic label, but no luck.  I do hope you find something.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [noodles775,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> hi gmbâI think noodles775 just reviewed me.
<jtv> gmb: â¦and I just reviewed noodles775.
<gmb> Heh.
<gmb> nm; I'll start on bigjools instead
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> it's trivial
<gmb> bigjools: Indeed. It's approved.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: fast work :)
<gmb> Small branch :)
<gmb> And lots of people doing work for me.
<gmb> Always fun to be fashionably late...
<noodles775> Thanks jtv
<jtv> np, thank you too
<noodles775> gary_poster: Hi! I'm wondering if you'd be able to look at two zope questions at https://code.edge.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572
<gary_poster> noodles775: on call, will look, formlib may in fact suck at this
<noodles775> Thanks gary_poster, no rush :)
<bac> hi gmb, i'm working on the bugs index page for project groups that do not use launchpad.  henninge has pointed out that page doesn't
<bac> have a title or breadcrumbs
<bac> i see that the project group page never had either.  do you know why?
<bac> deryck: perhaps you know about ^^^
<gmb> bac: No, I don't;
<gmb> My first guess would be "we didn't have time before 3.0 and then other things got in the way"
<bac> gmb: probably.
<bac> gmb: so you wouldn't be hurt if i added such things?
<bac> if i can figure it out, that is...
<deryck> bac, yeah, no good reason.  Just oversight.
<gmb> bac: I don't think it would hurt the page. It may hurt your brain; I can't remember how knotty the +index pages are...
<bac> gmb:  very convoluted
<henninge> bac: you should have an example in the blueprint page for project groups, or not?
<bac> henninge: you'd think.  this bug page is complicated
<henninge> :(
<gmb> Yeah, don't make the mistake of thinking that just because we're not Soyuz we can't write curiously intricate* code.
<gmb> *replace with "spaghetti" as appropriate
<leonardr> gmb, can you do ui reviews?
<gmb> leonardr: 'fraid not.
<leonardr> gmb, np. is it okay to get the non-ui code reviewed before the ui is approved? (i have actually never done a ui review before)
<gmb> leonardr: Sure, that's fine.
<noodles775> leonardr: also, you can request a review from launchpad-ui-reviewers and then ping someone (henninge and salgado are currently mentats, so may want some more).
<leonardr> noodles775: edwin already reviewd it, so i need a non-mentat reviewer to check his review
<noodles775> leonardr: ah, who's his mentor, or does he not have one?
<leonardr> noodles775: i got edwin's review about 2 weeks ago and then lifeless put the brakes on the branch, so i don't really remember anything. i may just start over
<leonardr> gmb, the branch is https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/36363
<leonardr> edwin, when you get in, let me know who your mentor is
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> hmm, that diff looks like there's a branch i haven't merged in or something
<leonardr> gmb, hold off a bit
 * gmb holds
<leonardr> ok, the problem was i'd proposed a merge against db-devel instead of devel. but now i see a two-week-old XXX in the diff, so i may be a while longer
<leonardr> gmb, if you're almost at eod, don't wait around for me
<gmb> leonardr: It's just coming up to 1pm for me, so I'll go and grab lunch. If you're still going on this at about 5pm my time I'll let you know that I'm EODing :)
<leonardr> ok
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> leonardr: I've taken your branch out of the queue for now; just re-add it when it's ready.
<leonardr> gmb. thanks
<noodles775> henninge or salgado: do either of you have time for a UI review? https://code.edge.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572
<noodles775> I don't think there's much to review here, it's just standard selection/form submission.
<salgado> noodles775, I'll do it in a couple hours if henninge doesn't beat me to it
<noodles775> Thanks salgado.
 * henninge is not the beating type ...
<noodles775> l
<noodles775> heh, another great use of the posture sensing ubuntu interface would be to optionally ensure the window you're looking at has focus (if you haven't explicitely given focus to a window in the last n seconds).
* leonardr changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> henninge, can you do a ui review for me?
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/37590
<leonardr> edwin did a review about two weeks ago, as recorded in the mp linked to from that me
<leonardr> or salgado, you can do my ui review after noodles755's
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/37590
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> leonardr: Is your branch ready for code review now, too?
<leonardr> gmb: yes it is
<gmb> leonardr: Okay, I'll look shortly.
<leonardr> great
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> henninge, can you take leonard's?
<leonardr> gmb, pushing a tiny test refactoring (r11591)
<gmb> k
<henninge> salgado, leonardr: yup, in 15 min.
<gmb> leonardr: code is r=me.
<gmb> leonardr: Nice work, BTW :)
<leonardr> gmb, thanks
<henninge> leonardr: looking now
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> leonardr, ping
<leonardr> sinzui, hi
<sinzui> I am reading your doctest
<leonardr> ok
<sinzui> leonardr, "The Ubuntu desktop called mycomputer" <- how do you know I am not running Mint or Fedora
<leonardr> sinzui: it's the client's responsibility to put the right string in there
<leonardr> that said, i have no idea how the client is supposed to know
<sinzui> okay
<leonardr> i'm kind of thinking uname -o but that gives "GNU/Linux" (which might be good enough?)
<sinzui> leonardr, "all applications running on mycomputer" <- I think this is an exaggeration. All applications running when I am logged in is closer to the truth
<sinzui> This is hard because most users do not distinguish between sessions and that procs run as under multiple users
<leonardr> sinzui: yes, i would say that knowing the truth is a prerequisite to knowing that that's an exaggeration
<sinzui> :)
<sinzui> use "choose" instead of "click". I can use a touch screen or keyboard
<sinzui> leonardr, if we used the exaggerated language, how will we respond to the bug reports from the knowledgeable?
<leonardr> sinzui: we tell them the whole truth and say that the wording is geared towards the casual user
<leonardr> sinzui: platform.linux_distribution() will give me 'Ubuntu'
<leonardr> but it is a 2.6ism
<sinzui> fab
<leonardr> so i guess i will use platform.dist() instead
<sinzui> leonardr, Other that the removal of the word "click" I have no suggested changes.
<leonardr> sinzui: ok. is this an official ui review?
<sinzui> yes
<sinzui> I will approve it now
<henninge> leonardr: I was about to tell you that I cannot reach the page ...
<henninge> leonardr: I get an openid login page and then "you're not allowed here".
<henninge> I don't see a warning
<leonardr> henninge: this is not a page you can just visit in your web browser
<leonardr> a client such as launchpadlib needs to do some setup work first
<leonardr> it's not a normal part of launchpad
<henninge> leonardr: yes, that is what I get when I do that.
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> browser pops up with a login page.
<noodles775> salgado: thanks. And yes, the all/none checkbox is included on the mockup, I hope to add it in a later branch, but it's not a priority right now.
<mars> morning gmb
<noodles775> salgado: who's your ui mentor again?
<leonardr> henninge: walk me through it step by step. what do you do?
<salgado> noodles775, sinzui
<gmb> Hi mars
<henninge> leonardr: 1. "make run" in your branch
<noodles775> sinzui: do you have time? https://code.edge.launchpad.net/~michael.nelson/launchpad/652838-select-diffs-for-syncing/+merge/37572
<henninge> leonardr: 2. "python" in another terminal
<henninge> leonardr: that's a system python
<sinzui> noodles775, I am just finishing leonardr's review. I will start yours in a few minutes
<noodles775> Thanks sinzui.
<henninge> leonardr: 3. "from launchpadlib.launchpad import Launchpad"
* allenap changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> leonardr: 4. "l = Launchpad.login_with("Ubuntu desktop (Bob's Computer)", service_root="dev", allow_access_levels=["DESKTOP_INTEGRATION"])"
<allenap> gmb, mars: Either of you fancy a short branch?
<gmb> mars: Could you take allenap's branch?
<mars> sure
<gmb> I'm trying not to context switch.
<gmb> mars: Thanks.
<mars> allenap, what do you have?
<leonardr> henninge: you want "System-wide: Ubuntu desktop (Bob's Computer)"
<allenap> mars: https://code.edge.launchpad.net/~allenap/launchpad/structural-subscriptions-with-filters-5/+merge/37608
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> allenap, I assume this is the branch listed in the topic?
<henninge> leonardr: ah, I took that from the original MP but I admit I did not read through the comments.
<leonardr> henninge: yeah, it changed so that you could say "System-wide: Android phone (...)" instead of always having to say "desktop"
<allenap> mars: Yep.
<allenap> Thank you :)
<henninge> leonardr: ok, I see it now ;)
<leonardr> great
<leonardr> henninge, fyi, sinzui just did the ui review
<mars> allenap, done, r=mars.
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> mars: Cheers!
<henninge> leonardr: I saw that but I have two more comments. I just put that in there.
<leonardr> ok
<leonardr> thanks
<bdmurray> mars: could you review https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-permissions/+merge/37514 ?
<mars> bdmurray, sure
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> leonardr: btw, a screenshot would have sped this up a lot. ;-)
<leonardr> henninge: sorry, i almost never do anything that has any ui component, so i'm new to this
<henninge> leonardr: np
<sinzui> noodles775, you have my approve with a remark about button case
<leonardr> mrevell, can i ask you to look at the wording in the branch https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/37590 ?
<mrevell> I'd be very happy to leonardr.
<noodles775> Thanks sinzui
<mars> bdmurray, done, r=mars
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> mars: great, thanks!
<henninge> mrevell, leonardr: screenshot for your convenience: http://people.canonical.com/~henninge/screenshots/desktop-integration-warning.png
<henninge> :-)
<leonardr> mrevell: i'd also like your opinion on a good heading for the page
<mrevell> thanks henninge, have you added me to the review?
<mrevell> I'd be glad to.
<henninge> mrevell: done
* gmb changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> henninge, is there any special technique for showing a page's heading, or shall i just scrape the h1 tag?
<henninge> leonardr: no, I think it's just an h1.
<henninge> the style sheet will take care of the rest
<henninge> leonardr: but put it inside the top-portlet container.
<leonardr> henninge: i meant for testing that it has a certain value
<henninge> leonardr: I don't understand
<leonardr> henninge: if i put <h1>Integrating $computer into your Launchpad account</h1> in the template, i need to add a test that "Integrating Bob's Computer into your launchpad account" shows up
<henninge> leonardr: I'd just get the top-portlet and do "print extract_text"
<henninge> extract_text(tag)
<leonardr> ok
<leonardr> henninge: r11592 of my branch has the change you asked for
<leonardr> mrevell: the title of the page i just added is "Integrating [computer/application] into your Launchpad account"
<gary_poster> mars, hopefully easy one when you get a chance: https://code.edge.launchpad.net/~gary/launchpad/bug650343/+merge/37633
<mars> gary_poster, sure
<gary_poster> thanks
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: gary_poster || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> leonardr: thank you very much. This looks good.
<henninge> now we are just waiting for mrevell to give his blessing ;-)
<mars> gary_poster, did you link the correct bug?
<mrevell> henninge: I'm clear now :)
<mars> gary_poster, bug 65043
<_mup_> Bug #65043: latest updates to intel-agp and/or i915-drm  in 2.6.17-10.29 slows down x generates mammoth syslog files <linux-source-2.6.17> <linux (Ubuntu):Incomplete> <linux-source-2.6.17 (Ubuntu):Won't Fix> <https://launchpad.net/bugs/65043>
<gary_poster> mars, sorry, yes and no.  See the programmatic bug link underneath my text
<gary_poster> bug 650343
<_mup_> Bug #650343: Add X-Launchpad-Original-To to recipient lists <Launchpad Foundations:Triaged by gary> <https://launchpad.net/bugs/650343>
<mars> ah, ok
<gary_poster> corrected
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> gary_poster, approved, r=mars
<gary_poster> mars, thank you.
<gary_poster> you ask good questions.  my answer to #1: I have no idea, but I think that the log warnings are intentional.  this was largely a mechanical change for me.  How far do you think I should chase this?  TBH, if you do not push me, I will not chase it at all.  I'm not proud of that, but I want to get it off my queue. :-)  So feel free to push me.
<gary_poster> #2: No idea, but that sounded like long-term thinking AFAICT
<mars> gary_poster, just a #comment will do - something to say "Yes, I thought about these, here is what I think you should know about them"
<gary_poster> OK, I'll see if I can crib an answer from someone, mars.  thanks
<mars> gary_poster, #2 is long-term thinking, yes.  Part of our review initiative to move tests out of c.l.
<gary_poster> gotcha
<gary-lunch> mars, jml, I'm not clear on the requirements: I need to move c/l/mail/incoming.py and c/l/doc/incomingmail.txt to lp/services/mail in order to land?  Or just the .txt?
<gary-lunch> bac, ping?
 * gary_poster forgot to change his nick
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gary_poster> mars, did you see my question above?  I tried to find instructions on what needs to move where but didn't find anything.
<gary_poster> and it looks like jml is telling me to move at least one file
<mars> gary_poster, yes, I don't know - I think the .py files would be easiest to move
<gary_poster> well, except then I have to fix all imports
<gary_poster> it just makes this a much bigger branch
<mars> yes, unfortunately.  If you had a pipes setup, I would say you could do that, or a follow-up
<mars> changing the imports should not be too difficult
<mars> grep + ec2 will find everything
<gary_poster> yeah, a follow-up branch is what I was thinking
<gary_poster> yeah, slowly ;-)
<gary_poster> okthanks
<mars> np
<allenap> mars: Have you got time for another small branch? https://code.edge.launchpad.net/~allenap/launchpad/structural-subscriptions-with-filters-6/+merge/37653
<mars> I do
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: allnap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> mars: Tip top, thanks :)
<mars> allenap, done, r=mars
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> mars: Thank you!
* mars 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
#launchpad-reviews 2010-10-06
<noodles775> r or rs for a oneline js testfix? http://pastebin.ubuntu.com/507097/
 * noodles775 lands it.
* gmb changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [gmb(http://is.gd/fNK3P)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap, deryck, adeuring: Do any of you gents have time to review my branch? I'd like to try and get it into EC2 early this afternoon so that I can deal with any fallout before my EoD
<adeuring> gmb: I'll do it
<gmb> adeuring: Excellent, thank you.
<gmb> adeuring: FTR, it's mostly code moves, so the contents of BugSubscriptionSubscribeSelfView are already-reviewed code from BugTaskView (if that helps at all).
<adeuring> gmb: thanks
<adeuring> gmb: r=me
<gmb> adeuring: Thanks.
* gmb 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
<henninge> stub: ping ;)
<henninge> stub: I need a new patch number because 21 is already in use for lp_OpenIdentifier.
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/db-devel-translationmessage-pofile/+merge/37563
<stub> henninge: 2208-23-0
<stub> henninge: Sorry about that. I must have gotten distracted before marking it down.
<henninge> stub: np, thanks.
* noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Speaking of reviews, jelmer, are you up for one?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/638090-base_version-property-for-differences/+merge/37742
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> noodles775: I can review your branch.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Great, thanks EdwinGrubbs :)
<EdwinGrubbs> noodles775:  I think test_base_version_common could be fleshed out so that it verifies that it chooses the right version when there are multiple common versions and that it ignores versions that aren't in common.
<noodles775> EdwinGrubbs: yep, adding now.
<EdwinGrubbs> noodles775: should a series with versions [1.0, 3.0] and a parent series with versions [1.0, 2.0, 3.0] return 3.0? I ask because 3.0 seems is a common version, but the term "base version" makes me think of  bzr, in which case it would be 1.0.
<noodles775> EdwinGrubbs: yes, it's the most recent version that both the parent series and the derived series have in common.
<noodles775> EdwinGrubbs: http://pastebin.ubuntu.com/507317/ <-- pushing as r9851
<EdwinGrubbs> noodles775: that looks good, but the status parameter shouldn't have  spaces around "=". Also, you could make it shorter with a for loop to create versions 1.0 and 1.1.
<noodles775> Yep.
<EdwinGrubbs> noodles775: r=me
* sinzui changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: noodles775 || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Thanks EdwinGrubbs
<sinzui> EdwinI have a branch that fixes the oops caused by large messages
<noodles775> EdwinGrubbs: jfyi, http://pastebin.ubuntu.com/507322/. Thanks.
<EdwinGrubbs> looks good
<bdmurray> Could I get https://code.edge.launchpad.net/~brian-murray/launchpad/modify-official-bug-tags-permissions/+merge/37758 reviewed?
<jcsackett> bdmurray: if you're asking for oncall, it helps to add yourself to the queue as well, in the channel info.
* bdmurray changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: noodles775 || queue: [sinzui, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: noodles775 || queue: [sinzui, bdmurray, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> EdwinGrubbs: there is clearly a long line ahead of me, but when/if you get to it, here's my MP: https://code.edge.launchpad.net/~jcsackett/launchpad/project-involvement-translations-contradiction-652287/+merge/37763
<jcsackett> if you don't think you'll have time today, i'll run someone else down.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: bdmurray || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> jcsackett: The review has been sent. I have to merge your branch into my upcoming branch to resolve some merge conflicts, so please land this as soon as possible so I don't have to delay landing my branch when it is ready.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs,  I reviewed your branch: https://launchpad.dev/apache and https://launchpad.dev/gnome both oops
<sinzui> I like your code changes. I do not see the cause of the oops
* benji changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> EdwinGrubbs: comin' atcha: https://code.edge.launchpad.net/~benji/launchpad/bug-612754-2/+merge/37797
<EdwinGrubbs> sinzui: thanks for the review. I've pushed up my fixes.
<EdwinGrubbs> benji: I'm starting on your mp now.
<benji> EdwinGrubbs: cool; I've got to head out but I'll check back later
#launchpad-reviews 2010-10-07
<sinzui> Edwin: I approved your branch
<EdwinGrubbs> benji: r=me
<EdwinGrubbs> sinzui: thanks
* StevenK changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [benji,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> StevenK: are you a reviewer?
<StevenK> EdwinGrubbs: I am, but my mentor is on holidays
 * mwhudson can mentor something if it's easy
<mwhudson> (not because i'm lazy but because i need to disappear fairly soon)
<EdwinGrubbs> StevenK: well, I want you to look at the branch since it is a soyuz testfix for db-devel.
<StevenK> EdwinGrubbs: Sure, point me to the MP?
<EdwinGrubbs> StevenK: here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/broken_test_do_not_copy_disabled_dases/+merge/37815
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [benji,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> Oh, that's my fault :-/
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> EdwinGrubbs: The code + test that was on db-devel pre-dates the devel code, and I was meaning to fix it, but sleep got in the way.
<StevenK> EdwinGrubbs: So, I can tell you my ideas on the MP, or I can just fix it since it's my fault. Which do you think?
<EdwinGrubbs> StevenK: you can just fix it.
<StevenK> EdwinGrubbs: Okay, doing so, thanks.
<wgrant> StevenK: Quick Soyuz one, if you're interested: https://code.edge.launchpad.net/~wgrant/launchpad/bug-655648-a-f-maverick/+merge/37820
<StevenK> wgrant: Added my code* to it
<wgrant> StevenK: Thanks.
<noodles775> Hi stub, would you have time for a tiny db-review? https://code.launchpad.net/~michael.nelson/launchpad/638090-base_version-property-for-differences/+merge/37742
 * stub has a look
<stub> noodles775: Might we need to join or lookup rows using base_version ? I'm wondering if it needs an index or not.
<noodles775> stub: Hrm... I can't think of an example. Currently the use-case is, here's a (loaded) difference, do you want to request a PackageDiff to be generated between the base version and the derived version etc. But right, it might save pain in the future when we add a query and assume an index?
<stub> noodles775: Sounds like we don't need it.
<stub> noodles775: r=stub patch-2208-24-0.sql
<noodles775> Great, thanks stub!
<bac> hi henninge
<henninge> Hi bac!
 * henninge looks at activereviews
<bac> hi henninge, could you have another look at my MP?  sinzui did the UI mentor
<bac> henninge: i'm also having some trouble with a translation test and need some domain-specific geniusity
<henninge> oh, is there a planned downtime for code hosting atm?
<henninge> bac: I imaging the help link looks just like what it looks on the project page, right?
<henninge> codehosting is back
<bac> yes indeedy
<henninge> bac: r=me
<henninge> bac: what's the failing test?
<bac> thanks
<bac> henninge: i'll paste it in a sec
<bac> henninge: when using factory.makePOTemplate it looks like sourcepackagename is non-optional, right?
<bac> if so that is, uh, dumb
<bac> henninge: http://pastebin.ubuntu.com/507871/
<bac> henninge: the failure is at line 116
<bac> and it is barfing on this part of the page template:
<bac>               <a tal:attributes="href context/menu:navigation/templates/url">
<bac>                 full list of templates</a>.
<bac> templates do not exist
<bac> henninge: so i think i have not done something that is required in the setup
<bac> henninge: you still around?
<henninge> bac: sorry, got called awy. back now
<bac> np
<bac> henninge: you can see the failure in devel with
<bac> bin/test -vvm lp.translations -t TestRobotsDistroSeries
<bac> if you download that test
<henninge> the setup looks ok
<henninge> bac: I wonder if the distribution needs official_rosetta set
<bac> hmm
<henninge> but I am still preparing to run the test
<henninge> bac: can you try ubuntu (but a new distroseries)?
<bac> henninge: in that test (look at set_usage in the base class) i do set the translation_usage property, which sets official_rosetta
<henninge> oh, that's what the test is about ... ;-)
 * henninge is catching on
<henninge> bac: which branch is this on?
 * henninge downloaded the wrong branch
<bac> henninge: lp:~bac/launchpad/bug-652280-pg-trans
<bac> give it a second as i'm pushing
<bac> and done
<henninge> branching
<henninge> bac: ok, I see the failure but am not any wiser yet.
<bac> henninge: i see the menu item is enabled_with_permission(launchpad.Edit)
<henninge> bac: ah yes, that is true ;)
<bac> henninge: the template does not check permissions.  but if that is the case i'm not sure how this works at all
<henninge> bac: can you point me to the template, please
<bac> distroseries-translations.pt (i think)
<henninge> sounds sensible
<henninge> bac: hm, I can see ubuntu/maverick/+templates, even when logged out.
<bac> yeah, i just saw that
<henninge> bac: btw, you can do without the test_suite at the end if you turn your "Base" into a "Mixin"
<bac> henninge: you mean make it not be a TestCase?
<henninge> yes and make the actual testcases inherit from (TestCase, Mixin)
<bac> i'm +0 on that.  maybe -0
<henninge> ;-)
<henninge> Why is that?
<henninge> bac: the test has no problems rendering +templates
<bac> henninge: i don't think actually rendering +templates is an issue, it is using the menu to simply construct the URL
 * bac baffled
<henninge> bac: removing the permissions check from the menu item does not help either.
<henninge> bac: so clearly it is accessing the wrong menu.
<bac> henninge: and removing the section from the page template results in another KeyError for language_packs...similar construct
<henninge> bac: do you know how menu lookup works?
<henninge> I mean, how "context/menu:..." works?
<bac> yeah, i think so
<bac> henninge: it is found through an adaption of the 'usedfor' interface on the given facet
<bac> henninge: was that sufficiently hand-wavy?
<henninge> bac: yes, thank you ;)
<henninge> It just explained why I don't see a connection made in zcml
<bac> henninge: odd.  i did just put a breakpoint in the 'templates' menu item...and it was not hit
<henninge> bac: makes sense. if the tempalte does not see it it will not call it.
<bac> duh
<henninge> bac: I'd ust like to get a "repr" of the menu that is actually used in the template
<henninge> s/ust/just/
<henninge> bac: TranslationUnavailable: Translations for this release series are not available yet.
 * henninge greps for TranslationUnavailable
<bac> huh?
<bac> where'd that come from?
<henninge> bac: I commented out both problem sites in the template.
<henninge> lib/canonical/launchpad/webapp/error.py:class TranslationUnavailableView(SystemErrorView):
<bac> henninge: so do you see something i'm missing in my setup that would cause translations to not be available?
<henninge> bac: check_distroseries_translations_viewable
<henninge> distroseries.hide_all_translations
<henninge> bac: I'll try setting that to false
<bac> yes, it is true by default
<henninge> hmph
<henninge> bac: no joy :(
<bac> i made that change to set hide_all_translations=False.  now i get a ComponentLookupError on +hierarchy on all tests
<henninge> bac: oh, really? I just get the old error. But I also reverted all my other changes.
<henninge> danilos: !
<danilos> henninge, hey
<henninge> danilos: bac created a new distroseseries on a test and tries to render +translations on it but it fails. What could be missing? ;)
<danilos> henninge, sorry, otp with bigjools
<henninge> :(
<henninge> danilos: np
<bac> henninge: thank you for your help.  i must EOD now in bitter defeat.
<henninge> bac: sorry but tomorrow is s a new day ... ;)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> StevenK: There's no branch for you on launchpad/+activereviews, so is your queue position stale data?
 * allenap assumes it's stale.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bryceh || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: Can I get a review for https://code.edge.launchpad.net/~gmb/launchpad/launchpadformify-subscribe-view-bug-651240/+merge/37850 please?
<allenap> gmb: Certainly :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Ta.]
<adeuring> allenap: the changes in your branch look good. But what is the reason for the changes? Do you assume that the SQL query for the old method would be too slow?
<allenap> adeuring: It's easier to select the targets elsewhere, and it's makes the resulting code closer to the existing structural subs code, so it's easier to reuse.
<allenap> adeuring: My follow-on branch is now simpler to implement.
<adeuring> allenap: ok. But now you may get a subscription more than once, if somebody is subscribed to two or more bug targets having bug tasks for a given bug
<allenap> adeuring: That turns out to be a problem. So far :)
<allenap> adeuring: That turns out to *not* be a problem. So far :)
<adeuring> allenap: interesting. But anyway, r=me; the possible duplicate results can be treated later
<bigjools> wgrant: moving here....
<bigjools> wgrant: test_ftparchive.py doesn't seem to test anything?
<allenap> adeuring: Thanks :)
<wgrant> bigjools: It tests that a disabled arch doesn't make it into the a-f config. It's from before I had the other tests -- it's redundant now, but more direct. I'm not sure if I should remove it or not.
<bigjools> wgrant: I can't see any new assertions in it, just setup
<wgrant> bigjools: The assertion is a-f config file comparison.
<bigjools> wgrant: I can't see any comparisons
<wgrant> It's probably a bad place to put it, but the rest of the tests are so black-box that it seems bad to rely on solely on them.
<bigjools> lines 81-89 in the diff
<bigjools> sets up the disabled das and re-generates config
<wgrant> bigjools: I'm adding a new disabled distroarchseries. The point of the test is to show that it doesn't show up in the a-f config file. So the a-f config comparison does not change from the original test.
<bigjools> wgrant: that's not a good test, since it works with or without that setup
<wgrant> Right, exactly.
<wgrant> But I don't see how else to do it.
<wgrant> Except for the even blacker-box approach of the other test.
<bigjools> add a direct assertion that it's not in the config
<wgrant> Eh. I suppose.
<bigjools> well, that's a bit shit too
<wgrant> It is.
<wgrant> This part of the publisher is, sadly, shit.
<wgrant> And the tests are worse.
<bigjools> so, we need a test that it does appear when enabled, and not there when disabled
<wgrant> Ideally. I could just drop this test, since it's covered by the other one now.
<bigjools> true
<bigjools> they are better tests
<bigjools> since you have negative and positive
<wgrant> They are. But it feels a little awkward to be testing this behaviour only at that very high level.
<wgrant> But I guess the code leaves us little choice.
<bigjools> my only comment about them is that they do a lot of asserting and if one fails you won't see the results of the others
<bigjools> so if you can think of a way to assert all at once that would be better
<wgrant> That would certainly be a good thing. But I don't know of any easy way.
<wgrant> So, I'll drop the test_ftparchive test.
<bigjools> one way is to make two lists and compare them
<wgrant> I would love to improve the test, but we sort of need this code soon, and I need to leave for uni in a little over 6 hours. So I might not.
<bigjools> that's ok
<bigjools> ditch the a-f test then
<bigjools> and I'll land the branch
<bigjools> set the MP to "needs review" though :)
<wgrant> Pushing...
<bigjools> stab stab stab dogfood
<wgrant> It's still going?
<bigjools> yes.  perhaps it needs more coal.
<wgrant> It seriously cannot take that long to dump 100000 rows from a view.
<wgrant> Or are there other dirty pockets?
<bigjools> well, it's chugging on lucid.  Perhaps I should restart with -s maverick
<bigjools> :/
<wgrant> That may be a reasonable idea.
<bigjools> le sigh
<wgrant> bigjools: Diff updated. I am happy enough with the branch now.
<bigjools> ok
<bigjools> wgrant: I'll test and land, thanks for writing it
<wgrant> Thanks.
<wgrant> A little unfortunate that I couldn't avoid that wonderful [7:]
<bigjools> shrug :/
<wgrant> But I tried to block it in a few other places, and they wouldn't work, so...
<bigjools> so it was quicker this time
<bigjools> the i386 Packages didn't get re-written though :/
<bigjools> I'll try again, the ctrl-c may have done nasty stuff
<wgrant> Ugh.
<wgrant> There'll be no dirty pockets.
<wgrant> You'll need to flip one of the pubs back to Pending.
<bigjools> yes
<bigjools> or upload a new package, which is a better test
<wgrant> But that'll take another 10 hours.
<wgrant> Anyway, I should sleep.
<bigjools> nope
<wgrant> Good luck with that...
<bigjools> 5 mins
<bigjools> thanks wgrant
<wgrant> Thanks for fixing my rather spectacular fuckup.
<bigjools> it's not quite done yet, we need to send grovelling emails to PPA owners
<bigjools> but enough of making you feel bad for one day :)
<bigjools> g'night
<wgrant> Most of them are probably already ancient, unnoticed history.
<wgrant> But we need to check that out, yeah.
<wgrant> Night.
<gmb> allenap: How's that review looking? It contains a fix for some of the current buildbot breakage so the sooner it's RTL the better.
<gmb> (I'll split that bit out if necessary, though).
<allenap> gmb: I'm about half way through it, but it's going quickly. I'll put the pedal to the floor though :)
<gmb> allenap: Ta.
<sinzui> gmb, was the test wrong? is the subscription to mozilla-firefox (Ubuntu)?
<gmb> sinzui: Well, I'm just looking now, and I'm getting the opposite results locally. It should be to Mozilla Firefox, not mozilla-firefox (Ubuntu) because the Mozila Firefox task is the primary task for the bug and we subscribe to the bug not the task.
<sinzui> ah
<gmb> sinzui: So, the test passes as it is in stable on my local machine. Change it to fix the BB failure and it fails locally.
<gmb> I think my brain may melt out through my ears shortly.
<allenap> gmb: Sorry, I fscked up your review by missing the prerequisite branch. Approved though.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: Thanks.
<gmb> allenap: So _subscribers_for_current_user is used quite a bit in the subsequent branch, so I'm happier to leave that as-is.
<gmb> allenap: And _isSubscriptionRequest() is vestigial and no longer in use.
<allenap> gmb: Yeah, those comments of mine need a lot of salt today.
<gmb> (Which I just noticed)
<gmb> allenap: Fair enough. I'll remove one and leave the other, then.
* noodles775 changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi allenap, do you have time for https://code.edge.launchpad.net/~michael.nelson/launchpad/656166-expose-dsd-diffs/+merge/37858
<allenap> noodles775: otp, but sure.
<noodles775> Thanks.
 * noodles775 fixes conflicts
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: noodles775 || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jml: I have a fix for the xx-person-subscriptions failure if you'd like to cast a weather eye over it; https://code.edge.launchpad.net/~gmb/launchpad/stable-unfark/+merge/37860
<jml> gmb: thanks. looking now.
<noodles775> allenap: ok, the MP is now updated and without conflicts.
<allenap> noodles775: Cool.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap 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
<sinzui> EdwinGrubbs, ping
<sinzui> EdwinGrubbs, do you have time to review
<sinzui> https://code.launchpad.net/~sinzui/launchpad/mlist-sync-0/+merge/37885 that fixes an oops?
<EdwinGrubbs> sinzui: sure
<EdwinGrubbs> sinzui: r=me
<sinzui> thanks EdwinGrubbs
#launchpad-reviews 2010-10-08
* bac changed the topic of #launchpad-reviews to: On call: - || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> Hi I'm on-call now.  bring out your reviews...
<StevenK> Except you set yourself as the branch being reviewed, not who is on-call
<bac> StevenK: yeah, well...
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> more happy StevenK?  :)
<StevenK> Heh, slightly. :-)
<wgrant> I feel timezone-confused.
<bac> why is that wgrant?
* bac 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
<bac> hi bigjools
<bigjools> hey bac
<bac> bigjools: did you verify your def-row db query change on the live db?
<bac> it looks good to me...
<bigjools> bac: it's already cowboyed, so yes :)
<bac> :)
<bac> alrighty
 * bigjools is doing two CP MPs at the moment :/
<jcsackett> anyone available to review a 93 line MP? https://code.edge.launchpad.net/~jcsackett/launchpad/mirrors-tell-where-they-live-652134/+merge/37925
<allenap> jcsackett: I'll do it. Can you review one of mine?
<jcsackett> allenap: i am not a reviewer yet. :-P
<allenap> jcsackett: Drat! You get this one for free then :)
<jcsackett> thanks allenap. i'll owe you one in november when i start the reviewing thing. :-)
<sinzui> allenap, I can review
<allenap> sinzui: Woohoo, thanks :)
<allenap> sinzui: You have probably found it already, but if not: https://code.edge.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823/+merge/37976
<sinzui> allenap, I see metal:use-macro="view/macro:page/main_side", but this template does not use the side. Is the intent to add navigation to see other listings such as indirect bug subscriptions or blueprints?
<allenap> sinzui: I don't know if that's the intent, but it can be changed back later. I'll switch to main_only.
<sinzui> okay
<sinzui> r=me. This branch does make the code clearer.
<allenap> sinzui: Thanks.
