#launchpad-reviews 2009-10-05
<al-maisan> Good morning!
* intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: jtv || queue: [gmb]  || This channel is logged: http://irclogs.ubuntu.com
* intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: gmb || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<intellec`> gmb: i wonder if a name like forceUpdateAll or something like that would be more appropriate for the method you've added. it's a bit confusing to read that there are no watches to update, and yet when you call updateAllWatches some update
<gmb> intellectronica: Hmm. Yes, I see your point. forceUpdateAll() also lends the method an air of danger, which it posesses in spades :)
<gmb> intellectronica: I'll change the method name.
<intellectronica> cool
<intellectronica> gmb: r=me with that change. nice branch!
<gmb> intellectronica: Thanks :)
<BjornT> gmb: i have a few questions about that review
<BjornT> gmb: what happens if you 1) run checkwatches.py --all 2) Ctrl-C:s since it's taking too long 3) restart checkwatches.py without the --all option?
<gmb> BjornT: If you run checkwatches.py --all without specifying a bugtracker it'll be ignored, for a start. If you kill it in the middle of a run then the watches will be left with lastchecked=None, because checkwatches deliberately just re-raises KeyboardInterrupt and SystemExit exceptions.
<BjornT> gmb: right, that's what i thought (regarding bug watches being left with lastchecked=None)
<gmb> BjornT: So I guess what we could do is have Ctrl-C mark the watches as lastchecked=datetime.now(), which would stop them from being updated next time. That seems a bit kludgy though. Alternatively we could keep a mapping of watch id => lastchecked time and just restore the old data.
<gmb> Or, hey, we could roll back :)
 * gmb has just checked where the transaction boundaries lie.
<gmb> Hmm.
<BjornT> gmb: it's nothing i would expect from using --all (and no, you can't roll back, since you commit the transaction after resetting)
<gmb> BjornT: No, you right, we can't. Thought it was in the try: except block.
<gmb> BjornT: So, the simple solution is setting lastchecked=datetime.now() for all watches.
<BjornT> gmb: i think the behaviour is fine, just unexpected. i think renaming --all to --reset would make things clearer. then it's more explicit
<gmb> BjornT: Okay, I agree with you.
<gmb> I'll make that change now. Thanks.
<BjornT> gmb: thanks. also, don't we run checkwatches.py in some test already?
<gmb> BjornT: Yes, we do, but I wanted to demonstrate that this could be done from the command line.
<BjornT> gmb: you shouldn't do that by starting a sub process. the LaunchpadScript class allows you to pass in arguments to it for testing
<gmb> BjornT: Oh, okay. Didn't know that. I'll update the test to use that, then.
<BjornT> gmb: that way you can make the test more useful, instead of checking that passing --all has the same effect as not passing --all ;)
<gmb> :)
<gmb> Righto.
<gmb> BjornT: Hang on, though, how do I import from cronscripts/checkwatches? I thought cronscripts wasn't in the PYTHONPATH.
<BjornT> gmb: you don't. you move the CheckWatches class to somewhere you can import it.
<gmb> BjornT: Ah, right. Figured you might say that :)
<matsubara> ow
<matsubara> oops
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: gmb,  - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<abentley> intellectronica: Could you please review https://code.launchpad.net/~abentley/launchpad/suppress-resubmit/+merge/12872 ?
<intellectronica> abentley: sure
<abentley> intellectronica: Thanks.
<intellectronica> abentley: so what's the story with those lint warnings?
<abentley> intellectronica: Dunno.  Bogus lint warnings like that have been showing up for months.
<intellectronica> abentley: in wonder if it would be better to move the check into findRelatedBMP
<intellectronica> fair enough
<abentley> intellectronica: Well, it would be findRelatedAndNotSupersededBMP then.
<intellectronica> no, i meant to also parameterize it
<intellectronica> but now i see that it's not in the interface. if it's just an internal function then i guess it doesn't really matter much
<abentley> intellectronica: I'd be happy to change it to something like findRelatedBMP(include_superseded=False)
<intellectronica> abentley: yes, that's what i meant. your call
<abentley> intellectronica: I've pushed a version that does that.  Here's the diff: http://pastebin.ubuntu.com/286266/
<intellectronica> abentley: great. r=me
<abentley> intellectronica: Thanks.
<allenap> abentley: Can you review a very small trivial wording change for me? https://code.edge.launchpad.net/~allenap/launchpad/just-comment-on-question-bug-114710-wording-change/+merge/12879
<abentley> allenap: sure
<abentley> allenap: r=me.
<allenap> abentley: Thanks.
* sinzui1 changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: gmb,  - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> sinzui1: what do you need reviewed
<intellectronica> ?
<sinzui1> intellectronica: I do
<sinzui1> https://code.edge.launchpad.net/~sinzui/launchpad/delete-structural-target/+merge/12877
<intellectronica> oright, i'm reviewing it
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui,  - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<intellectronica> sinzui1: are the changes to launchpad-login.pt and style-3-0.css really part of what you want me to review?
<intellectronica> sinzui1: also, isn't destorySelf an sqlobjectism?
<intellectronica> sinzui1: also, does it really make sense to disallow the deletion of a milestone with subscribers? anyone can subscribe to the milestone, so this can make it very inconvenient for the project team. i'd expect to be able to delete it with the subscriptions deleted too
<intellectronica> sinzui1: ah, i think i understand what's going on. you're doing that, but in the view. the problem is that the behaviour is then different if you interact with the model directly (through the API, for example)
<intellectronica> sinzui1: also nm my first question. i now see why you did want me to review those changes
<intellectronica> sinzui1: so the code looks fine, but i really think the work should be done in the model and not in the view. what do you think?
<sinzui1> intellectronica: yes, the login style fix is required because .subordinate must work as beuno wants
<intellectronica> yeah got it once i read the rest of the diff
<sinzui1> intellectronica: destorySelf may be a sqlobject-ism, but that is also  the method we use for all destroys. I do not know of another
<intellectronica> sinzui1: the storm way of doing this is store.remove(obj)
<intellectronica> or something like that
<sinzui1> intellectronica: I can try that
<intellectronica> sinzui1: Store.of(obj).remove(obj)
<intellectronica> a bit clumsier, but it will be nice to slowly get rid of the sqlobject stuff in the codebase
<sinzui1> intellectronica: I am skeptical of moving the product series work into the model. 1) it is not true delete, 2, it couple the model to a dozen objects, some of which are outside the registry
<intellectronica> sinzui1: right, i understand the dilemma
<intellectronica> how would we solve that if and when we want to expose that functionality via the API?
<sinzui1> We do not intend to expose this over the API
<sinzui1> When we can delete translations, we ill consider it
<intellectronica> that's a good answer, i guess :)
<sinzui> intellectronica: danilo thinks it is possible but this kind of issue is not out focus anymore
<sinzui> :(
<intellectronica> though i can totally see how milestone and series mgmt is something project teams might want to automate
<intellectronica> yeah, i realise that :-/
<intellectronica> oh well, hopefully one day we'll have an opportunity to improve on that
<intellectronica> sinzui: for now, r=me
<sinzui> thanks. I will work on the storm change now
<intellectronica> cool
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -,  - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<sinzui> intellectronica: thanks for the storm suggestion. I was able to factor out removeSecurityProxy.
<intellectronica> excellent!
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
#launchpad-reviews 2009-10-06
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> gmb: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-submission-consistency-check-udev/+merge/12910 ?
<gmb> adeuring: Sure.
<adeuring> gmb: thanks!
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<gmb> adeuring: In your tests, you need to tidy up the dicts a bit. For single item dicts you can get away with the  "{'id': 1}," style. Otherwise, opening braces should be on the end of the preceding line, not on a line of their own.
<adeuring> gmb: right...
<gmb> adeuring: Take a look at the dicts in the devices list on line 233 of the diff, for example.
<gmb> adeuring: Otherwise, this is r=me.
<adeuring> gmb: thanks!
<bigjools> gmb: got room for another review?
<gmb> bigjools: Sure
<bigjools> gar
<bigjools> clicking on the wrong person in the chooser is seriously annoying, you don't get a chance to separately OK the review request :/
* bigjools changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adeuring || queue: [bigjools]  || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bigjools || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<gmb> Sampledata change. Nooooooooooooo.
<stub> bigjools: Feel like adding the patch-.sql file and pushing?
<bigjools> stub: ohhhhhhhhhhhh fuck
<bigjools> gmb: lint requires it
<bigjools> schema change
<gmb> bigjools: Yeah, I know. Wish we could exclude its largeness from the diff though.
<stub> bigjools: Is there any foreseeable use of storing the new data in a more structured format?
<stub> bigjools: I'm assuming it is just used raw as is by the soyuz code.
<gmb> bigjools: Code looks food, fwiw. r=me.
<bigjools> stub: no, I see this as a temporary change until OEM has all their archives in LP
<bigjools> gmb: grassyass
<gmb> bigjools: Day nardar.
<bigjools> :)
<gmb> West country accented spanish. That sounds like it should be a sitcom trope.
<bigjools> stub: it gets passed to the builders as-is, whence it is placed in the sources.list in the chroot
<stub> cool. Assuming the patch is just 'ALTER TABLE foo ADD COLUMN bar text;' it all looks fine at my end.
<bigjools> yep that's it
<bigjools> I pushed the new rev up BTW
<stub> bigjools: Is Mark aware of this yet? (Not that I see him having any issues with this one)
 * gmb really needs to remember to write an X-Chat plugin that manages the review queue. Or a bot. Or something else.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bigjools> stub: I think jml is doing that job now
<stub> cool
<bigjools> which is why he's on the MP ;)
<allenap> jml: Do you have a few minutes to look at https://code.edge.launchpad.net/~allenap/launchpad/packages-for-autoload-bug-443061/+merge/12867 -- it's a patch for the autoland problem we talked about yesterday.
<jml> allenap, sure.
<al-maisan> jml: I was wondering whether you had a chance to look at lp:~al-maisan/launchpad/dd-bug-no-1
<jml> al-maisan, no, not yet. I plan to today
<jml> (just going through a backlog of email caused by sprinting & moving to London)
<jml> al-maisan, I did skim it though. I was wondering if you looked at lp:~jml/launchpad/upload-permission-joy at all?
<al-maisan> jml: no problem
<al-maisan> jml: you mean the refactoring you did?
<jml> al-maisan, that's right.
<jml> al-maisan, well, the second work-in-progress refactoring branch that I was doing before I changed the branch permission stuff :)
<al-maisan> jml: the function added (person_may_edit_branch()) makes use of verify_upload()
<al-maisan> the function that resulted from your refactoring
<jml> al-maisan, yes, but there's a second & different refactoring branch that's in progress.
<al-maisan> jml: oh
<al-maisan> would that be 'lp:~jml/launchpad/upload-permission-joy' ?
<jml> al-maisan, yes
<al-maisan> jml: sorry, I merged lp:~jml/launchpad/upload-permission-joy into LP devel, that was my starting point
<al-maisan> or .. wait
<al-maisan> did I get the branch wrong .. just a sec..
<jml> al-maisan, I think you got the branch wrong. Your first commit says Imported jml's 'official-write-permissions' branch, and I don't see my work from u-p-j in the diff.
<al-maisan> jml: hmm .. I see.
<al-maisan> jml: I'm on CHR duty today but will take a look at lp:~jml/launchpad/upload-permission-joy tomorrow ..
<al-maisan> maybe we can have a chat then
<jml> al-maisan, cool. I look forward to it :)
<al-maisan> :)
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Hi gmb, got time for one? https://code.launchpad.net/~michael.nelson/launchpad/436182-newlines-in-sources-workaround-pt2/+merge/12918
<gmb> noodles775: Sure.
<noodles775> Thanks
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: noodles775 || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<gmb> noodles775: Nice branch. r=me
 * gmb lunches
<noodles775> Thanks gmb.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: lunch, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> good morning
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<gmb> Morning bac.
<bac> hi graham.  quiet review day?
<gmb> bac: So far. Let's not tempt fate, eh?
<bac> gmb:  have you upgraded to karmic?  are there expected test failures for the LP suite?
<gmb> bac: I've only upgraded my netbook so far; waiting on reports back from deryck about whether the Karmic upgrade affects the development process before doing the work machines.
<bac> gmb: i did a netbook last week too.  did my dev box yesterday.  ran the full suite last night and got a handful of errors
<gmb> Not unexpected, I guess.
<bac> nope
* sinzui changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<sinzui> gmb: bac: Do either of you have time for https://code.edge.launchpad.net/~sinzui/launchpad/announcement-heading/+merge/12898
<bac> sinzui: just claimed it
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, sinzui || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<bac> hi sinzui.  i'm looking at your changes to create_view and create_initialized_view
<bac> it looks good but i'm surprised you didn't use them in this branch.
<sinzui> bac: I did. I used them to test breadcrumbs. I had to create a fake request
<bac> sinzui: gah, nm i see it in use at th e last line
<bac> sinzui: nice change.  r=bac
<sinzui> thank
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> bac: Can you review a very small lpbuildbot branch please? https://code.edge.launchpad.net/~allenap/lpbuildbot/avoid-deadlock/+merge/12921
<bac> allenap: yes.  i'm about to start a 45 minute call but can look at it afterwards
<allenap> bac: Thanks.
* allenap changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [allenap]  || This channel is logged: http://irclogs.ubuntu.com
<gmb> bac, I'll take allenap's branch
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> Thanks gmb.
<gmb> allenap: So, can you explain to my addled mind why proc.wait() is safe now that you've grabbed stdout before doing it?
<gmb> Cos reading the MP, I don't get it :)
<allenap> gmb: proc.wait() waits for the process to finish. If it tried to write more than 64k to its stdout, it would fill the buffer and be blocked on writing, waiting for it to be read, which won't happen because we're waiting for it to finish.
<allenap> s/it to be read/the buffer to be read/
<gmb> allenap: Ah right, I think I get it now.
 * gmb Needs tea
<gmb> allenap: r=me, then.
<allenap> gmb: Danke :)
<gmb> allenap: You'll need to set the branch status manually as I'm not sub'd to it.
<allenap> gmb: Strange, ~launchpad is subscribed to the trunk. I wonder if that's a bug.
<allenap> gmb: Ah, I can't set it either!
<gmb> Hah.
<gmb> allenap: That might be a bug then.
<gmb> allenap: While you're here, do you ahve time for a reciprocal review?
<gmb> It's 300 lines, but it's mostly refactoring
<allenap> gmb: Or some new Krypton Factoresque puzzle from the code crew.
<allenap> gmb: Yeah, sure.
<gmb> allenap:  https://code.edge.launchpad.net/~gmb/launchpad/checkwatches-all-to-reset-bug-442993/+merge/12930
<gmb> allenap: It's a fix for bug 442993, not 422993 as the MP suggests.
<mup> Bug #442993: checkwatches --all should be --reset <bugwatch> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/442993>
<allenap> gmb: I was looking at the bug thinking "whaaa?"
<gmb> Indeed.
<gmb> What amuses me is that someone understood that bug well enough to mark it as a dupe.
<jml> allenap, references to the Krypton Factor were one of the many things in "A Bit of Fry and Laurie" that I didn't get.
<bac> gmb:  thanks for taking allenap's review.  i'm off the call now
<allenap> jml: If those two chose to be cryptic, who am I to let you in? I mean, Stephen Fry has recently been described as the "King of Norfolk" no less.
<jml> allenap, well, it's more that the Krypton Factor never existed in .au
<jml> allenap, so jokes about it being unwatchable lack punch.
<leonardr> bac, i'll have a fun review for you in a few minutes
<bac> leonardr: ok
<leonardr> bac: here you go: https://code.edge.launchpad.net/~leonardr/lazr.restful/oauth/+merge/12931
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<jml> https://code.edge.launchpad.net/~jml/launchpad/browse-source-code/+merge/12933 when you're ready :)
<jml> beuno-lunch, ^^^
<allenap> gmb: r=me
<jml> bac, can you please review that branch?
<bac> jml: sure.  after leonard's and lunch
<jml> bac, thanks.
<bac> or maybe lunch then leonard's
<gmb> allenap: Awesome, thanks.
<gmb> allenap: Can you mark the mp approved please for the sake of ec2 land?
<gmb> Not that I can't do it myself, but that still seems funky to me.
<allenap> gmb: Sure, sorry, I forgot.
<gmb> allenap: I honestly think that if there's only one review the reviewer should be presented with that option when they approve the review. But maybe that's just me.
 * gmb will file a bug.
<allenap> gmb: Yeah, it's a two-step process that I'm often doing. I think the option to default to "no change", but it would be useful if it were there.
<allenap> s/to default/should default/
<gmb> allenap: Better still, it would happen automagically. After all, if there are n reviewers (where n > 1), who gets to decide that the whole thing is ready to land? The last reviewer? What if they're just reviewing the legal aspects of the change... etc.
<jml> gmb, ec2 land also has a "--force" option to set the overall mp status.
<jml> well, not set, but ignore.
<jml> but it needs actual votes to populate the thingummy.
<gmb> jml: If anyone ever provides a --force option for a thing, I decline to use it, in case it will do something Bad. Even if I know it won't.
<allenap> gmb: How about a --go-on option?
<gmb> But maybe that's a hangover from my old job where we had a script that needed --force, --really-force and --no-seriously in order to actually do anything useful.
<gmb> allenap: --pretty-please
<jml> gmb, fair call.
<jml> gmb, that's kind of the spirit that I added it in (i.e. don't actually use it)
<gmb> Glad to hear it :)
<jml> but yes, we need a much better way of handling the overall mp status
<gmb> jml: Incidentally, `ec2 land` is made entirely out of win. Nice job :)
<jml> gmb, thanks.
<bac> leonardr: nice branch, thanks.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jml || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> jml: done.  thanks
<jml> bac, thank you.
<jml> you wouldn't happen to know the answer to my question in #launchpad-dev, perchance
<leonardr> bac: ok, i hope to have a corresponding lazr.restfulclient branch for you later today
<bac> leonardr: look forward to it
<bac> jml: for ec2 land, what does the commit message need to look like?  what parts does the script fill in for you?  will you update the message on your MP i just reviewed?
<jml> bac, Just the text. ec2 fills in the square bracket bits. no, I'll just use --commit-text.
<jml> bac, e.g. ./utilities/ec2 land https://code.edge.launchpad.net/~jml/launchpad/browse-source-code/+merge/12933 --commit-text="Change the link to browse source code to point at actual source code, not a revision log."
<bac> jml: nice.
<jml> there's a --dry-run option to let you muck around.
<jml> output of that command + dryrun is: ['./utilities/ec2', 'test', '--headless', u'--email=jml@mumak.net', '-b', 'launchpad=devel', '-s', u'[r=bac][ui=beuno][bug=369273] Change the link to browse source code to point at actual source code, not a revision log.', 'bzr+ssh://bazaar.launchpad.net/~jml/launchpad/browse-source-code']
<bac> ping rockstar
<gary_poster> allenap: I have a fix for the problem that https://code.edge.launchpad.net/~allenap/launchpad/packages-for-autoload-bug-443061/+merge/12867 addresses
<gary_poster> AFAIK
<gary_poster> So that you would no longer need to warn people about that problem
<gary_poster> maxb: fwiw, ^^^
<maxb> whoa, your commit message has broken the layout of the BMP page ! :-)
<gary_poster> allenap: I'll add a comment to the MP.  If this diff has landed, that's fine.  It won't cause any harm, and we can revert it later
<rockstar> bac, hi
<bac> rockstar: hi
<rockstar> bac, what's up?
<leonardr> bac: here's the lazr.restfulclient branch: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/oauth/+merge/12940
<bac> rockstar: i was trying to use branch.getMergeProposals() from launchpadlib and am not ever getting any results.  i wonder if you've used it.
<rockstar> bac, tarmac uses it.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> leonardr: will start now
<leonardr> great
<rockstar> bac, are you sure you have the right branch?
<bac> In [54]: br
<bac> Out[54]: <branch at https://api.edge.launchpad.net/beta/~thumper/launchpad/code-review-history>
<rockstar> bac, lemme look.
<bac> thx
<bac> nice leonardr
<bac> rockstar: i see what is happening now
<rockstar> bac, yes?
<bac> branch.getMergeProposals returns MPs proposed to merge INTO the branch not ones for where the branch is the source
<rockstar> bac, yeah, that's what my hypothesis was, but I couldn't confirm it yet, as I was on the phone.
<bac> rockstar: the good news is there is branch.landing_targets
<rockstar> bac, yeah, that's what I was going to suggest, and that getMergeProposals should return all involved proposals.
<rockstar> We seem to have some redundancy there.  Could you file a bug on that?
<bac> rockstar: is there redundancy or just poor documentation?
<rockstar> bac, redundancy, as we also have branch.landing_candidates
<bac> rockstar: oh, i hadn't looked at that one yet
<bac> rockstar:  sure, i'll open a bug
<rockstar> bac, thanks.
<bac> rockstar: though landing_candidates only returns those in a final state where getMP allows you to specify the status.  just saying there is some varied functionality
<bac> rockstar:  bug 444854
<mup> Bug #444854: API for getting a branch's merge proposals is confusing <Launchpad Bazaar Integration:New> <https://launchpad.net/bugs/444854>
<rockstar> bac, yeah, I think that bug reflects the real problem: confusion.
<rockstar> bac, got time for at least one little branch, maybe two?
<bac> rockstar:  at least one
<bac> rockstar: branches for me?
<rockstar> bac, one coming up.  Sorry, learned another difference between lightweight and heavyweight checkouts.
<bac> rockstar: np
<rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/branch-upgrade-job/+merge/12945
<mwhudson> bac: you there still?
<bac> hi  mwhudson
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<mwhudson> bac: i have a fix for a very simple bug
<mwhudson> bac: but the test is offensive
<bac> great
<mwhudson> bac: here's the diff http://pastebin.ubuntu.com/287338/
<mwhudson> (this is the bug https://bugs.edge.launchpad.net/launchpad-code/+bug/444387)
<mup> Bug #444387: productseries-codesummary.pt typo gives all series branches 31 recent revisions <post-ui-3-cleanup> <trivial> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/444387>
<mwhudson> bac: any ideas for how to make the test less horrid?
 * bac looks
<bac> mwhudson: sorry, i can't think of how to simplify that test
<mwhudson> bac: oh well, i guess i'll just try to hide it with some bland prose
<bac> mwhudson: the narrative at line 32 of your diff is a bit perplexing.  Could you expand that a bit?
<mwhudson> bac: heh, yes, that bit isn't finished yet...
<bac> ok
<bac> mwhudson: based on what you have you can rs=bac if you can't find a reviewer later.
<mwhudson> bac: thanks
<mwhudson> bac: how long are you around for?
<bac> not long at all
<bac> i may be back later, though
<bac> but i'm checking out as OCR now
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<mwhudson> ok
<barry> bac: ping
<bac> barry: this change looks great.  r=bac  http://pastebin.ubuntu.com/287348/
<barry> bac: thanks!
<mwhudson> bac: http://pastebin.ubuntu.com/287352/ diff now looks like this, have time to review it still?
<mwhudson> rockstar: hi, can you do a simple review for me?
<mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/bug-444387/+merge/12949
<rockstar> mwhudson, looking.
<mwhudson> rockstar: thanks
<rockstar> mwhudson, r=me.  Thanks for getting us some more test coverage.
<mwhudson> rockstar: thanks
<mwhudson> rockstar: can i bang another simple one past you?
<mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/rocketfuel-setup-ssh/+merge/12950
<rockstar> mwhudson, r=me
<mwhudson> rockstar: thanks
#launchpad-reviews 2009-10-07
<rockstar> mwhudson, going to find dinner, will be back to handle any more reviews in just a bit.
<mwhudson> rockstar: i think i'm done with the trivial stuff for now, thanks
<al-maisan> Good morning!
<stub> Trivial review needed to close a 4 digit bug: https://code.edge.launchpad.net/~stub/launchpad/bug-3050/+merge/12977
<jml> stub, already done.
<stub> jml: Just saw that :) ec2test already run so its just pqm submit for this one.
<jml> stub, ok, thanks.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<stub> jtv: np. Do you need me to land any of my branch or are you still going to be working with 8.3 for the time being?
<jtv> stub: No need; I left the issues that your branch resolves untouched, and that avoids any conflicts.
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Hi allenap - a really easy one for you: https://code.launchpad.net/~michael.nelson/launchpad/add-soyuz-windmill-layer/+merge/12983
<noodles775> (well, short :) ).
<allenap> noodles775: On it :)
<noodles775> Thanks!
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: noodles || queue: [EdwinGrubbs]  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: r=me
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: EdwinGrubbs || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> Thanks again :)
<allenap> noodles775: Heh, you're welcome :)
<BjornT> noodles775, allenap: i have some remarks on that last review
<noodles775> BjornT: yep?
<BjornT> noodles775: why did you change jstest.in?
<noodles775> BjornT: because I moved the old functional test to lp.soyuz.windmill, so I assumed (perhaps wrongly) that the change in jstest.in was required?
<BjornT> noodles775: or rather, why did you bother to move the old-style test?
<noodles775> BjornT: because that seems to be what all the other apps had done?
<noodles775> s/had/have
<BjornT> noodles775: well, yes. but you should also convert it, which requires it to be moved to another location again
<noodles775> BjornT: yes, but it's dependent on a registry base class to test the inlineeditor, and I didn't have time to update that now, so I just did what all the other apps had done.
<BjornT> noodles775: i still think it's easy to convert that one. might not be the best solution, but should be enough for now
<noodles775> BjornT: great, thanks.
<BjornT> noodles775: have you tested to create a test that creates a widgets.InlineEditorWidgetTest instances, and calls it?
<noodles775> BjornT: I did not look at updating that test at all - as I assumed that I'd need to update the InlineEditorWidgetTest itself (which would affect lots of other tests).
<noodles775> But your idea would be much simpler.
<BjornT> noodles775: please try it. if it works, it's not much work at all. to be extra clear, i meant something like this: http://pastebin.ubuntu.com/287744/
<noodles775> BjornT: Great - will do (after finishing my current MP) :)
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: EdwinGrubbs || queue: [noodles]  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> just off to lunch, but in case you get time allenap : https://code.launchpad.net/~michael.nelson/launchpad/436182-newlines-in-sources-fix-without-workaround/+merge/12988
<noodles775> tia!
<allenap> noodles775: Okay.
<leonardr> allenap, can you add me to the queue? https://code.edge.launchpad.net/~leonardr/lazr.authentication/initial-implementation/+merge/12989
<allenap> leonardr: Sure.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: EdwinGrubbs || queue: [noodles, leonardr]  || This channel is logged: http://irclogs.ubuntu.com
<leonardr> allenap, i have two more branches that go along with that one. a lazr.restful branch and a lazr.restfulclient branch
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/remove-authentication/+merge/12991 and https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/use-lazr-authentication/+merge/12990 respectively
<leonardr> if it gets confusing, let me know
<allenap> leonardr: Gah! Okay, I'll try to get to them :)
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: EdwinGrubbs || queue: [noodles, leonardr*3]  || This channel is logged: http://irclogs.ubuntu.com
<bac> hi jml
<jml> bac, hello
<bac> jml yesterday i fiddled with ec2 land a bit. i made it figure out where the MP is based on the public branch.  would you care to look at the changes as a "mid-implementation"?
<jml> bac, yeah, I'd love to.
<adeuring> allenap: can I add myself to your review queue?
<adeuring> https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-1/+merge/12992
<jml> bac, I'm on a call right now, and I'm going to help vds with some trial stuff later.
<allenap> adeuring: It's getting pretty busy, but I'll try.
<bac> jml:  whenever you can get to it.  no rush.
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: EdwinGrubbs || queue: [noodles, leonardr*3, adeuring]  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> allenap: thanks!
<jml> bac, cool, thanks.
<bac> jml: http://pastebin.ubuntu.com/287803/
<bac> jml: lp:~bac/launchpad/gmp
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: noodles || queue: [leonardr*3, adeuring]  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> BjornT: so http://pastebin.ubuntu.com/287819/ worked perfectly. Thanks!
<BjornT> noodles775: cool!
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: leonardr || queue: [leonardr*2, adeuring]  || This channel is logged: http://irclogs.ubuntu.com
<jml> bac, you still around?
<allenap> leonardr: In lp:~leonardr/lazr.authentication/initial-implementation, revision 47, it looks like you moved the code from src/lazr/authentication/__init__.py to wsgi.py, but didn't add wsgi.py to the branch. Is that right?
<leonardr> let's see...
<leonardr> yes, indeed
<leonardr> pushing a new version now
<allenap> leonardr: Cool :)
<leonardr> allenap: pushed
* allenap changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> adeuring: I'm really sorry, I didn't get to your branch.
<adeuring> allenap: no problem. I must admit that I found some quirks in the branch anyway. Fixing them at present...
<allenap> adeuring: Ah, just as well then :) Have a good evening.
<leonardr> deryck, maybe you want to review my forward-port of intellectronica's branch? https://code.edge.launchpad.net/~leonardr/lazr.restful/forward-port/+merge/13009
<deryck> leonardr, sure, looking in 2 minutes.
<leonardr> ok
<intellectronica> leonardr: your diff contains lots of unrelated stuff
<leonardr> intellectronica: i merged with another branch i did this morning because i wanted to have a unified NEWS file. i'll give you a clean diff
<leonardr> intellectronica, deryck: https://pastebin.canonical.com/23066/
<intellectronica> looks fine to me
<deryck> leonardr, looks good to me, too.
<leonardr> all right, i'll land it after my prerequisite branches land
<intellectronica> leonardr: thanks for taking care of this
<deryck> leonardr, indeed, thanks so much!
<leonardr> don't get excited yet :) i haven't figured out the launchpad side of things
<deryck> heh
<bac> hi jml
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: EdwinGrubbs || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<leonardr> gary, care to review https://code.edge.launchpad.net/~leonardr/lazr.restful/work-on-2.4/+merge/13015 ?
<gary_poster> leonardr: sure :-)
<leonardr> gary, confirmed that it still works with 2.6
<gary_poster> leonardr: great.  r=gary
<leonardr> gary: who do i need to talk to to get launchpad to pick up the new version?
<leonardr> gary: sorry, i typed that and then did research myself
<gary_poster> leonardr: heh, yeah, cool.  someone with commit privs.  can I answer any questions?  It should be pretty easy.
<leonardr> it looks like i need to do a launchpad branch
<gary_poster> yes
<leonardr> i'll follow the instructions and see how far i get--i'll probably finish tomorrow
<gary_poster> leonardr: cool.  I hope to leave RSN myself
<bac> hi EdwinGrubbs -- time for a 213 line MP?
<EdwinGrubbs> bac: sure
<bac> EdwinGrubbs: ok.  i just sent it.  i'll paste here when it arrives
<bac> EdwinGrubbs: https://code.edge.launchpad.net/~bac/launchpad/bug-341935-captcha/+merge/13022
<EdwinGrubbs> bac: review sent
* sinzui changed the topic of #launchpad-reviews to: on call: EdwinGrubbs || reviewing: - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<bac> thanks EdwinGrubbs
<bac> EdwinGrubbs: that was indeed a typo
#launchpad-reviews 2009-10-08
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<nhandler> I wanted to try my hands at patching a few bugs in LP. Following https://dev.launchpad.net/PatchSubmission, I am coming here first to discuss it prior to beginning work. I was thinking about trying to fix Bug #296469
<mup> Bug #296469: "to describe the type of review your doing" on merge proposal submit form <code-review> <trivial> <ui> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/296469>
<sinzui> hi nhandler
<rockstar> hi nhandler 
<nhandler> Hell sinzui and rockstar. I think the bug looks pretty straight forward. Other than the stuff outlined in the dev wiki, is there anything special that I need to do prior to beginning work on it?
<rockstar> nhandler, It's a pretty straightforward patch.  If you get stuck, just ping me, I'd be happy to help.
<sinzui> nhandler: This text change may no have a test. We do not normally test for grammar
<nhandler> Ok. I'll follow the steps on the wiki and see how far I can get. So far, I've managed to get a working copy of LP on my local machine without any difficulty
<nhandler> Who would be the project lead (for the contributor agreement) ?
<sinzui> hmm
<sinzui> nhandler: flacoste is the acting lead
<sinzui> Francis Lacoste
<nhandler> Thanks
<sinzui> nhandler: I just searched for part of the text fragment. Looks like you want to edit lp/code/interfaces/codereviewvote.py
<nhandler> Thanks sinzui. I was just about to do a grep for that string ;)
<nhandler> Does flacoste have an email address I should use? There is no email address on launchpad.net/~flacoste (but there is a gmail account listed under Jabber)
<sinzui> hmm
<mwhudson> i guess it's not a state secret that firstname.lastname@canonical.com works pretty well ?
<rockstar> mwhudson, yeah, I was just about to mention that.
<sinzui> mwhudson: yes. I used private message
<mwhudson> sinzui: ah, cool
<rockstar> Although it does reveal that people don't know where to send contributor agreements.
<nhandler> mwhudson: I wasn't sure if they wanted mail sent to their @canonical.com address. I usually try and use whatever address people specify on their LP page
<mwhudson> oh
<mwhudson> nhandler: have you seen this page yet? http://www.canonical.com/contributors
<nhandler> mwhudson: Yes. That is why I was asking about the project lead. Reading it again, it looks like I completely missed the table at the bottom (which should also be updated to reflect the new name of the Ubuntu Software store and to only list 1 irc nick for David Barth)
<nhandler> Would someone here be willing to review the merge request (I need a name for the merge proposal according to https://dev.launchpad.net/PatchSubmission)
<mwhudson> nhandler: sure
<nhandler> Thanks mwhudson 
<nhandler> I'm working on the Demo and Q/A section of the cover letter. Unless I missed something, I do not remember seeing any mention of Sample Person (test@canonical.com:test) up until now. I saw the admin@canonical.com account, but never test@. It might be a good idea to mention this account on one of the other pages
<nhandler> And is there a package I was meant to install for the restful module? I'm getting a few warnings about 'No module named restful' from make lint
<mwhudson> nhandler: not necessarily, that might just be pylint being stupid
<nhandler> mwhudson: So should I just ignore the warnings this time?
<sinzui> ignore it
<nhandler> It might also be a good idea to demonstrate enclosing the diff in triple-braces in the diff section of the DetailedCoverLetterTemplate wiki page
<nhandler> Just to make sure that people use the correct format
<sinzui> There are several lazr.* false warnings
<nhandler> Should the cover letter go to the launchpad-dev mailing list? The wiki did not mention an address to send it to
<mwhudson> nhandler: it should go into the merge proposal
<nhandler> mwhudson: Alright thanks. I just submitted the merge proposal (https://code.launchpad.net/~nhandler/launchpad/bugfix296469/+merge/13034). Hopefully, everything is correct.
<mwhudson> nhandler: unfortunately life is never easy :/
<nhandler> mwhudson: Very true. Although, preparing that patch and merge request was not too bad.
<mwhudson> nhandler: the problem is you've change the text on the page where you do a review too
<mwhudson> nhandler: review sent
<nhandler> Thanks for the review mwhudson. I'll see what I can do.
<mwhudson> nhandler: i forgot to thank you for your contribution!
<mwhudson> that's remiss of me
<mwhudson> nhandler: thank you for helping to make launchpad better!
<nhandler> You are welcome. Thank you for taking the time to help me (and other people) through the process ;)
<mwhudson> nhandler: no problem at all!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> noodles775: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-1/+merge/13051 ?
<noodles775> adeuring: sure - just in a standup, but will start it straight after.
<adeuring> noodles775: thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: - || queue: [sinzui, adeuring]  || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: adeuring || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> adeuring: I'm trying to merge your branch but get: $ bzr pull lp:~adeuring/launchpad/hwdb-class-udev-device-1
<noodles775> bzr: ERROR: These branches have diverged. Use the missing command to see how.                                         
<noodles775> Use the merge command to reconcile them.
<adeuring> noodles775: interesting...
<adeuring> noodles775: let me merge trunk
<noodles775> adeuring: in case it helps: http://pastebin.ubuntu.com/288466/
<adeuring> noodles775: odd... I can't reproduce the errors you are seeing....
<noodles775> adeuring: did you try merging your branch into a new rf branch?
<adeuring> noodles775: ah, let me try
<adeuring> noodles775: that work too for me...
<adeuring> noodles775: ah, could you try "bzr merge" instead of "bzr pull"?
<noodles775> adeuring: gar, sorry - I'd just pulled devel :/
<adeuring> noodles775: "bzr branch lp:~adeuring/launchpad/... review-branch" works also quite fast for me
<noodles775> yes, my mistake... sorry.
<noodles775> mrevell: shoot :)
<mrevell> noodles775: https://code.edge.launchpad.net/~matthew.revell/launchpad/status-link/+merge/13054
<mrevell> :)
 * noodles775 waits for the branch to be scanned...
<mrevell> tis there now noodles775
<noodles775> mrevell: r=me,ui=me.
<mrevell> thanks noodles775
<noodles775> adeuring: is it really necessary to be patching class methods in _setupUdevConsistencyCheckTests()?
<adeuring> noodles775: that is a pattern I used elsewhere too in that file. The advanatge is that you don't need to setup more or less complex or convoluted test data.
<adeuring> noodles775: the method checkConsistentUdevDeviceData() will be extended in future branches
<noodles775> adeuring: I'm all for using fakes/doubles/stubs/mocks, but not by manually patching object methods like that...
<noodles775> it looks scary ;)
<adeuring> noodles775: what would you suggest instead?
<noodles775> adeuring: ideally I'd like us to have a standard helper library for that type of thing (actually, from memory stub added one a while back?)
<adeuring> noodles775: really? any idea where I can find it?
<noodles775> adeuring: No, I'm hoping stub might remember - but if not, in this case, the sample data required doesnt' look too tedious?
 * noodles775 looks a bit closer.
<stub> mocker was added to the buildout config
<noodles775> Thanks stub.
<adeuring> stub: thanks
<adeuring> noodles775: the point is that checkConsistentUdevDeviceData() will in the future call at least two more methods, and setting up data that lets one of these clled methods fail will become tedious
<noodles775> adeuring: right. So that's exactly where mocker should be able to help right? I'm just checking for exactly what we need here.
<adeuring> noodles775: I couldn't find yet stub's mocker ...
<noodles775> "Mocking via temporary patching of existent classes and instances."
<noodles775> From http://labix.org/mocker
 * noodles775 checks the branch - I'm not sure that anyone has actually used in in LP yet - it was added after a discussion around this exact issue.
<stub> eggs/mocker-0.10.1-py2.4.egg for the source
<BjornT> noodles775, adeuring: i don't see a reason for using mocker to do that. can't you simply subclass SubmissionParser and override the methods in the new class?
<adeuring> BjornT: nice idea, thanks!
<noodles775> BjornT: yep - that sounds like a good option.
<BjornT> adeuring, noodles775: that's what we do elsewhere, for example when testing ExternalBugTracker
<noodles775> BjornT: although it's happening *lots* in adeurings file - and creating separate classes for all the different examples used in the test could be overkill? (using mocker, it would be just setting up the results you want from your proxied object, like this: http://labix.org/mocker#head-f372c49a9d1b56832e42d5a1ccc960ff75b0b096)
 * noodles775 checks the ExternalBugTracker tests to see what it's like there.
<adeuring> noodles775: http://paste.ubuntu.com/288544/
<noodles775> adeuring: great - that works well :)
<BjornT> noodles775: well, it depends. i'd like to see the code before commenting. the advantage of creating a class, is that you can give it a meaningful name and a docstring, explaining what it does. it's also easier to debug.
<adeuring> right
<noodles775> BjornT: yep - agreed.
<noodles775> adeuring: *phew* r=me. Sorry it took me so long to get through it!
<adeuring> noodles775: thanks!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: lunch || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com
* allenap changed the topic of #launchpad-reviews to: on call: noodles || reviewing: lunch || queue: [sinzui, allenap*2]  || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: allenap || queue: [sinzui, allenap]  || This channel is logged: http://irclogs.ubuntu.com
<noodles775> allenap: so the previous mp failed on buildbot because it didn't have paramiko?
<allenap> noodles775: Yes, the buildbot AMI seems to be out of date, which is another bug. However, I thought it was a good idea to put the dep into the tree anyway.
<noodles775> allenap: ok, and where is pycrypto used?
<allenap> noodles775: It's required by paramiko :)
<noodles775> allenap: r=me
<allenap> noodles775: Thanks :)
<intellectronica> noodles775: hi, i need a ui and code review. fairly simple stuff. shall i get in line after sinzui and allenap?
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: bac || queue: [sinzui, allenap]  || This channel is logged: http://irclogs.ubuntu.com
<sinzui> My branch is small
<noodles775> intellectronica: pop it in the queue - I'm doing reviews for people who are around first..
<intellectronica> i'm around
<noodles775> :)
* intellectronica changed the topic of #launchpad-reviews to: on call: noodles || reviewing: bac || queue: [sinzui, allenap, intellectronica]  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: Sorry I took a while to answer earlier. My wife had smashed her finger and I had to soothe her and put a plaster on it :) (It was tiny.)
 * sinzui note to self that intellectronica bot has been replaced by a human
<noodles775> lol on two counts.
<allenap> sinzui: Or a self-aware bot.
<jml> bac, hi
<jml> bac, I was going to talk to you about your patch yesterday, but I didn't get the chance.
<sinzui> bac wont be available for a few hours
<noodles775> oh, I assumed he was around too.
<sinzui> he is offline for about 2 hours
<noodles775> sinzui: and are you around? If so, I'll grab your branch now (your status is still away?)
<noodles775> s/around/around for questions etc. :)
 * sinzui stabs pidgin
<sinzui> irc/messaging is not the same in karmic
<noodles775> I'm still using xchat... I looked at empathy and pidgin, but went back :/
<sinzui> I tried empathy for a day. It is not suitable for irc
<intellectronica> noodles775: https://code.edge.launchpad.net/~intellectronica/launchpad/bugmail-ui-fixes/+merge/13067
<noodles775> Thanks intellectronica.
<intellectronica> sinzui: indeed. a shame, because i would like to use it for jabber/msn, but i also like having all my communication going on in one program
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: sinzui || queue: [allenap, intellectronica]  || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: bac || queue: [sinzui, allenap, intellectronica]  || This channel is logged: http://irclogs.ubuntu.com
<EdwinGrubbs> noodles775: can you do a ui review of https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-399554-timeline-improvements/+merge/12952
<noodles775> EdwinGrubbs: pop it in the queue - rockstar will be around soon too to help out.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: noodles || reviewing: bac || queue: [sinzui, allenap, intellectronica, Edwin-ui]  || This channel is logged: http://irclogs.ubuntu.com
<allenap> noodles775: BTW, just in case you're waiting for diff generation on <https://code.edge.launchpad.net/~allenap/lp-source-dependencies/ec2-test-race-bug-422433-the-revenge/+merge/13063>, it isn't going to happen, as the only changes are two new files, a zip and a tarball.
<allenap> noodles775: Of course, I'm not at the front of the queue either :)
<noodles775> allenap: yeah, I'm currently looking at bac's ui review as beuno is around to chat about it etc., but will be back to the queue in a tick.
<noodles775> sinzui, allenap: is either of your MPs mega urgent? Otherwise, will you be upset if I focus on UI reviews for the rest of the day while beuno is around and leave your code reviews to the US reviewers?
<sinzui> I have all day since I am CHR
<allenap> noodles775: No, do UI :)
<noodles775> Thanks guys.
* allenap changed the topic of #launchpad-reviews to: on call: noodles || reviewing: bac || queue: [sinzui, intellectronica, Edwin-ui]  || This channel is logged: http://irclogs.ubuntu.com
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: intellectronica-ui || queue: [sinzui, intellectronica, Edwin-ui]  || This channel is logged: http://irclogs.ubuntu.com
<allenap> bac: Could I bother you for a very quick review of https://code.edge.launchpad.net/~allenap/lp-source-dependencies/ec2-test-race-bug-422433-the-revenge/+merge/13063? It's just adding two distribution files to lp-source-dependencies.
<noodles775> beuno: so in case you want to chat about it - I'm looking at the ui for intellectronica at https://code.edge.launchpad.net/~allenap/lp-source-dependencies/ec2-test-race-bug-422433-the-revenge/+merge/13063?
<beuno> noodles775, cool, looking
<intellectronica> wrong branch
<beuno> :)
<noodles775> https://code.edge.launchpad.net/~intellectronica/launchpad/bugmail-ui-fixes/+merge/13067
<noodles775> sorry.
<beuno> intellectronica, could I bother you for screenshots on your MP?
<beuno> my rf is borked
<intellectronica> beuno: np, just a sec
<intellectronica> beuno, noodles775: https://devpad.canonical.com/~tom/bugmail-subscription-links.png
<beuno> intellectronica, thanks
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: Edwin-ui || queue: [sinzui, intellectronica, Edwin-ui]  || This channel is logged: http://irclogs.ubuntu.com
<beuno> EdwinGrubbs, could you provide me with screenshots for your timeline MP?
<EdwinGrubbs> beuno: sure
<beuno> thanks
<allenap> intellectronica: Could I bother you for a very quick review of https://code.edge.launchpad.net/~allenap/lp-source-dependencies/ec2-test-race-bug-422433-the-revenge/+merge/13063? It's just adding two distribution files to lp-source-dependencies.
<intellectronica> allenap: sure, i'll review it
<allenap> intellectronica: Thanks.
<EdwinGrubbs> beuno:
<EdwinGrubbs> Screenshot of https://launchpad.dev/firefox
<EdwinGrubbs> https://chinstrap.canonical.com/~egrubbs/project_index_page_timeline.png
<EdwinGrubbs> Screenshot of https://launchpad.dev/firefox/+series
<EdwinGrubbs> https://chinstrap.canonical.com/~egrubbs/project_full_history_timeline.png
<beuno> EdwinGrubbs, thanks much
<intellectronica> noodles775: are you reviewing the code too, or are you moving on to other ui reviews?
<noodles775> intellectronica: yeah, I'm doing other ui reviews while beuno is here at the same time.
<intellectronica> cool. allenap, would you mind reviewing the code in my branch, then? it's quite simple stuff
<allenap> intellectronica: Sure.
<intellectronica> allenap: also, for reasons i don't understand, i still don't get a diff for your branch on the mp. care to paste the diff?
<noodles775> EdwinGrubbs: I'm guessing you guys are already aware of timeout errors on https://edge.launchpad.net/bzr/+series ?
<intellectronica> allenap: thanks a lot. the mp is at https://code.edge.launchpad.net/~intellectronica/launchpad/bugmail-ui-fixes/+merge/13067
<allenap> intellectronica: You won't get one; OOPS-1377MPCJ1. It's just two binary files anyway, paramiko-xxx.zip and pycrypto-xxx.tar.gz, or something like that.
<intellectronica> allenap: what am i supposed to be reviewing, then?
<intellectronica> allenap: maybe just explain to me what you're doing, i'll pretend to understand and then you can land it with r=me
<noodles775> EdwinGrubbs: actually, someone just commented on #lp - perhaps it's related to the bzr project rather than the page itself.
<allenap> intellectronica: You're just rubberstamping that I've added the correct files to the source dependencies.
<allenap> intellectronica: Heh :)
<intellectronica> allenap: unfortunately i don't have a rubber stamp. is a stamp made from a potato good too?
<allenap> intellectronica: I added a couple of dependencies (http://pastebin.ubuntu.com/288542/) and these files have to be added so that lp can build.
<intellectronica> allenap: i'm very much in favour of anything that ensures LP can build. r=me
<allenap> intellectronica: From you, any kind of stamp is good.
<BjornT> allenap: fwiw, i think you're the first one to request a review for a change to lp:lp-source-dependencies, the rest of us simply add the files without asking :)
<allenap> BjornT: Ah, okay, I'll do that in the future :)
<allenap> intellectronica: Apologies if I've wasted your time wrt. BjornT's comment.
<bac> hi noodles775, beuno: thanks for the review.  noodles775 i like your second suggestion and will go with it.
<noodles775> bac: great.
<intellectronica> allenap: no waste of time. i had a lot of fun
<allenap> intellectronica: Me too. Phew, cold shower time.
<EdwinGrubbs> noodles775: is there a bug for the timeouts? I thought we had fixed the problem on the +series page by not tallying bug counts for obsolete series. I don't know anything about the slowness on the +index page, though.
<bac> jml: ping
<noodles775> EdwinGrubbs: not sure - I just tried to follow the link on the bug related to your MP. BTW, is there an easy way for me to setup all those series and milestones shown in your screenshots locally?
<jml> bac, hi
<jml> bac, so, we were going to talk about the patch to ec2 land?
<bac> jml: you wanted to discuss the ec2 branch?
<bac> :)
<jml> bac, yeah.
<EdwinGrubbs> noodles775: yep, here's the script: http://pastebin.ubuntu.com/288669/
<noodles775> EdwinGrubbs: thanks!
<jml> bac, the Right Way To Do It, I think, is for bzrlib to have a way to get a launchpadlib branch based on a Bazaar branch
<jml> bac, there's a command in bzr called 'lp-open' which does something similarish to your command...
<bac> jml:  ok, that's the kind of thing i wanted to know.  i've limited experience using bzrlib
<jml> bac, cool.
<jml> bac, the stuff that's in bzrlib right now isn't very good, IMO
<jml> bac, bzrlib itself doesn't have any launchpadlib stuff yet...
<jml> so the code that's there is about "guessing the Launchpad branch URL based on the public_location and push_branch values."
<bac> jml: i'll see what i can do.  but it'll have to wait until the weekend. you sure what i've done isn't Working And Good Enough until bzrlib grows the needed support?
<jml> bac, I was just wondering that myself
<bac> jml: i'm not tasked to work on this branch
<jml> bac, but I've lost the diff :)
<jml> no
<jml> got it
<jml> bac, if you change get_merge_proposal_from_branch to try the push_location after the public_branch, ...
<bac> jml: that's a good idea
<jml> bac, and change the call to BzrDir.open_containing_tree_or_branch to pass '.' rather than '', then I think this is good to go.
<bac> jml:  cool.  i'll make those changes and submit it for a proper review.
<jml> bac, thank you.
<bac> noodles775: based on your second screenshot, were you still advocating for a pop-up help?
<bac> noodles775: https://code.edge.launchpad.net/~bac/launchpad/bug-341935-captcha/+merge/13022
<noodles775> bac: no, after seeing that it really belongs in the step 2 text, I don't think a popup is necessary - what do you think?
<noodles775> s/it/the explanation of the captcha field
<bac> noodles775: i agree it is not necessary
<bac> noodles775: this should look familiar: http://people.canonical.com/~bac/captcha-2.png
<bac> noodles775: diff at http://pastebin.ubuntu.com/288678/
<noodles775> bac: great, thanks for that!
<bac> noodles775: cool.  update the MP for me please?
<noodles775> bac: erm, I'd already approved it - it's beuno who marked it needs-fixing... beuno?
<bac> beuno: could you have a look at https://code.edge.launchpad.net/~bac/launchpad/bug-341935-captcha/+merge/13022 and update the review to 'Approved' if you agree?
<beuno> bac, sure
<bac> thx
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [sinzui, intellectronica]  || This channel is logged: http://irclogs.ubuntu.com
<beuno> bac, noodles775, approved, thanks guys
<noodles775> beuno: great. I've just done EdwinGrubbs's ui review too. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-399554-timeline-improvements/+merge/12952
 * beuno looks
<beuno> EdwinGrubbs, noodles775, done
<allenap> intellectronica: I really like your bugmail-ui-fixes branch :)
<allenap> intellectronica: Two questions.
<allenap> intellectronica: Line 49, when it does self.context.context, is that because self.context is a view?
<intellectronica> allenap: exactly. context can be either the target itself or a view
<allenap> intellectronica: Could you add a comment to the else: clause to say that?
<intellectronica> allenap: sure, that's a good idea
<allenap> intellectronica: Question the second. The zcml. Is there an common interface where you can declare the permissions for userHasBugSubscriptions, rather than doing it 7 times?
<intellectronica> allenap: i wish. but no, that's not possible because of the stupid way in which zcml handles the interaction between permissions specified by interface and byb attributes
<allenap> intellectronica: Okay. r=me :)
<intellectronica> allenap: thanks!
<allenap> intellectronica: I missed it first time. What does byb mean?
<intellectronica> allenap: sorry, it's nsfw
<allenap> intellectronica: Hehe, okay, I'll use the Google.
<barry> nobody on call? :(
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: sinzui || queue: [intellectronica]  || This channel is logged: http://irclogs.ubuntu.com
* intellectronica changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: sinzui || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> hi rockstar -- room for another?
<barry> rockstar: hi!  can i get on your queue?
<rockstar> bac, barry, yup.
<bac> rockstar: https://code.edge.launchpad.net/~bac/launchpad/gmp/+merge/13083
* bac changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: sinzui || queue: [bac, barry]  || This channel is logged: http://irclogs.ubuntu.com
<barry> rockstar: thanks!  i'm experimenting with pipes so i'll push the main branch, but i have a smaller clean up branch as a follow on
<rockstar> barry, okay.  Do you know about sync-branches?
<barry> rockstar: is that different than sync-pipeline?
<rockstar> barry, er, sync-pipeline
<barry> rockstar: that's what i'm trying now! :)
<barry> rockstar: hmm:
<barry> % bzr sync-pipeline lp:~barry/launchpad/445713-menu
<barry> bzr: ERROR: Pipeline has no pipe named "445713-menu".
<barry> so that's clearly wrong
<rockstar> sinzui, "The TeamInvitationView allows a team admin accept or decline an invitation" looks like you're missing a "to" in there.
<sinzui> hmm
<sinzui> yes that might help
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: bac || queue: [barry]  || This channel is logged: http://irclogs.ubuntu.com
<bac> rockstar: i messed up on the review type.  can you change it to 'code'?
<rockstar> bac, does it matter?
<bac> rockstar: yes
<bac> rockstar: using ec2 land pulls that info from the MP so that is what gets fed to PQM
<bac> rockstar: thx
<rockstar> bac, ah, I didn't notice it was specified as 'ui'
<bac> rockstar: my bad
<rockstar> bac, although if nothing was specified, it shouldn't matter.
<rockstar> bac, whether it does or not, I don't know, but that seems like a heck of a constraint.
<bac> rockstar: oops, i think that may be a regression in the work i just did.  let me fix that.
<rockstar> "code" review should be implied unless otherwise noted.
<EdwinGrubbs> beuno: noodles suggested moving the series name to the end of the horizontal line, so that it will show up when it is auto-scrolled to the right.
<beuno> EdwinGrubbs, I think that sounds like a good idea. Also, I think there may be no harm in repeating it
<EdwinGrubbs> beuno: Should the series just appear once on the right, or should it appear in the center and perhaps the left for really long series.
<beuno> :)
<beuno> see, you don't even need me anymore
<bac> rockstar: http://pastebin.ubuntu.com/288781/
<rockstar> bac, ah, much better.
<rockstar> barry, where is your mp?
<barry> rockstar: got waylaid with a phone call.  coming soon
<rockstar> barry, okay.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: barry || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> rockstar: finally!  mp sent
<barry> rockstar: lovely.  my mp's oopsed
<rockstar> barry, those are my favorite kind.
<barry> rockstar: wtf: https://lp-oops.canonical.com/oops.py/?oopsid=1377CMP1
<barry> rockstar: and differently: https://lp-oops.canonical.com/oops.py/?oopsid=1377CMP2
<barry> i guess launchpad hates pipes :(
<rockstar> abentley, ^^
<barry> abentley: i guess i will try to mp it the old fashion way
<EdwinGrubbs> beuno: here's what it looks like on the project page: https://chinstrap.canonical.com/~egrubbs/multi_labels_project_page.png
<EdwinGrubbs> beuno: in this screenshot, you can see that I only show the left and right labels if the series line is longer than the viewport (iframe). https://chinstrap.canonical.com/~egrubbs/multi_labels_project_page.png
<abentley> barry: The first one looks like an error in the branch name.
<rockstar> abentley, yeah, that's what I was thinking.
<abentley> barry: The second is probably due to a known bug that when the target has revisions not present in the source, creating the source branch from the merge directive blows up.
<barry> abentley, rockstar so i probably don't quite understand how to push and mp my pipes
<barry> i think i followed the directions here: http://bazaar-vcs.org/BzrPipeline
<barry> abentley, rockstar but maybe i screwed up.  what do you suggest i do now?  repush?  re-mp?
<abentley> barry: Could you forward the message that caused the first failure, please?
<beuno> EdwinGrubbs, looks great to me
<EdwinGrubbs> thanks
<barry> abentley: done
<rockstar> barry, I need to go eat something before I pass out.  Will be back in 30.
<abentley> barry: It looks as though you've run reconfigure-pipeline, but that's not a good choice for launchpad developers.
<barry> :-o
<abentley> barry: Launchpad devels should just create their branches where they normally do, and then create a lightweight checkout.
<barry> abentley: i create branches in my shared repo with 'bzr branch devel 123456-foo'
<barry> abentley: then normally i just cd to 123456-foo, hack, commit, push, send
<barry> abentley: i don't quite grok how that workflow should change with pipes
<barry> abentley: you're right that this time i did a reconfigure-pipeline, then i hacked, committed, pushed, send (but the last three after switching to the appropriate pipe)
<abentley> barry: The smallest change would be running "checkout --lightweight 123456-foo work", and then doing your hacking in "work".
<barry> abentley: so once i've done that checkout, i'm basically going to do everything else for this bug in 'work' and not in 123456-foo?  (i'm asking stupid questions because i've never used lightweight checkouts; they don't quite fit my model of how to work)
<abentley> barry: Correct.
<barry> abentley: when i commit in 'work' it will change 123456-foo though, right?
<barry> abentley: or iow, i do not want to just do a lightweight checkout of my local devel
<abentley> barry: It will change the branch at 123456-foo, but not the working tree there.
<barry> abentley: gotcha.  all the working tree changes will be in work
<abentley> barry: Right.
<barry> abentley: okay! thanks.  i'm assuming i can unfsck myself by doing what you suggest and merging in each pipe from my existing branch, right?
<abentley> barry: I guess you could, but you can just move the branches out of 445713-menu/pipes.
<barry> abentley: oh, cool.  and then i don't need reconfigure-pipeline, right?
<abentley> barry: And then in 445713-menu, run "bzr switch --force ../05-cleanup"
<abentley> barry: Right.  You won't want to use reconfigure-pipeline, because it puts your branches at a different location, and that screws up your locations.conf rules.
<abentley> barry: That's why your source branch location is bzr+ssh://bazaar.launchpad.net/~barry/launchpad/445713-menu/pipes/05-cleanup
<barry> abentley: gotcha.  thanks for all this help.  i think i have my blogging work cut out for me tonight :)  one last question: for hacking lp, sync-pipeline isn't really useful, right?
<abentley> barry: Yes, it is.
<barry> abentley: oh.  i guess i don't really understand what sync-pipeline does then
<abentley> barry: It will push all the branches in your pipeline to Launchpad, and include metadata so that other users can see the pipeline.
<barry> abentley: so i use that instead of 'bzr push'?
<abentley> barry: Yes.
<barry> abentley: okay cool.  thanks, that was very helpful.  i'll give all of this a try now.  rockstar it might be a little while before those mps get sent.
<mwhudson> nhandler: did you get a chance to look at that branch yet?
<barry> rockstar: okay.  i think i have smoked the crack pipeline and have something ready to review.  i'm going to try the mp's again, okay?
<rockstar> barry, it's your song, sing it.
<barry> rockstar, abentley wow. i think it worked!
<barry> bac: remember yesterday when i requested a ui review of the +review-licenses page?  there's a screenshot here: http://launchpadlibrarian.net/33318909/review.png
<barry> bac: that's the mp i requested you ui review with rockstar
<barry> rockstar, bac gotta go get my flu shot now.  will be back in about 60m.
#launchpad-reviews 2009-10-09
<nhandler> mwhudson: I'm still working on it. My python abilities are not that great (I prefer Perl), so it might take me a little while to make the necessary changes
<mwhudson> nhandler: ok
<mwhudson> nhandler: let me know if you need help
<nhandler> mwhudson: Will do.
<nhandler> mwhudson: I figured out how to override the description. Should I override it in both of the two locations that review_type is used? Or only in one of them? Either method will result in the correct string being displayed on the correct page
<mwhudson> nhandler: i'm not completely sure
<nhandler> mwhudson: Overriding it in both locations would probably be safest.
<mwhudson> nhandler: part of me is saying that the description in the interface should be aimed at the programmer, and _all_ of the uses of it in a form should override it to something appropriate for a human
<mwhudson> nhandler: perhaps more particularly, in the interface the description should describe what the field means in the object
<mwhudson> nhandler: as the description in the interface ends up here: https://edge.launchpad.net/+apidoc/#code_review_vote_reference
<nhandler> mwhudson: Alright, I'll update the description in the interface and add overrides in the 2 locations that the review_type is used
<mwhudson> nhandler: awesome, thanks
<nhandler> mwhudson: I think I have a correct patch now. Since I am modifying actual code now, do I need to run some tests on it? Can I do this without having an Amazon EC2 account?
<mwhudson> nhandler: you could probably run the codereview page tests locally; they shouldn't take that long
<mwhudson> nhandler: i'll run all the tests before landing anyway
<nhandler> mwhudson: Exactly how do I run the test? https://dev.launchpad.net/Testing only shows how to do it in ec2.
<mwhudson> nhandler: ./bin/test -vvct stories/codereview i think
<mwhudson> nhandler: that's a pretty terrible page
<nhandler> lib/lp/code/stories only contains branches, codeimport, feeds, __init__.py, and webservice. There is no stories/codereview (unless you are talking about a different stories directory)
<mwhudson> hmm!
<mwhudson> nhandler: try code/stories/branches
<nhandler> Well, I tried actually running the first command. It started to run, but crashed with 'ImportError: No module named Crypto.Cipher
<mwhudson> nhandler: i hope https://dev.launchpad.net/Testing is a little more sensible now
<mwhudson> nhandler: do you have launchpad-developer-dependencies installed?
<nhandler> mwhudson: Yes. It got installed when I went through the guide for setting up Launchpad
<mwhudson> nhandler: hmm
<mwhudson> nhandler: can you pastebin the output of the trying to run the command?
<nhandler> mwhudson: http://paste.ubuntu.com/288999/
<mwhudson> nhandler: hm, that shouldn't happen, but it also shouldn't prevent you from running the other tests
<mwhudson> nhandler: do you have python-crypto installed?
<nhandler> mwhudson: That fixed the issue I had. But I get: Total: 0 tests, 0 failures, 0 errors in 0.000 seconds.
<nhandler> Which does not seem right
<mwhudson> nhandler: try ./bin/test -vvct stories/branches
<mwhudson> (this will run more tests than you really need to, but better than none)
<stub> I assume this topic is out of date?
* stub changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<stub> Trivial and boring review waiting at https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/13048 if someone can be arsed.
<mwhudson> stub: luckily for you i'm working on a tests in the appserverlayer
<nhandler> mwhudson: Tests all passed. I submitted a new merge request (https://code.launchpad.net/~nhandler/launchpad/bugfix296469.2/+merge/13103). https://code.launchpad.net/~nhandler/launchpad/bugfix296469/+merge/13034 can be closed.
<nhandler> And I really appreciate all of the help you have provided. I have learned a lot
<mwhudson> nhandler: spending time to help new developers along is _always_ worth it
<mwhudson> nhandler: you should be able to delete https://code.edge.launchpad.net/~nhandler/launchpad/bugfix296469/+merge/13034
<nhandler> mwhudson: I can delete the merge request, but it also deletes the comments. I thought there was a way to change the status so it no longer shows up in the proposed merges list, but is still available online
<mwhudson> nhandler: you can probably set the status to 'rejected' i guess
<mwhudson> i thought there was a way to withdraw it...
<mwhudson> nhandler: in general, you didn't need to make a new merge request though
<mwhudson> nhandler: a comment saying "can you look again" would have been fine
<nhandler> mwhudson: I can only set the status to 'Needs review', 'Work in Progress', and 'Merged'. The reason for the second merge request was that a second branch was created (due to me not messing around with bzr merge to resolve a divergence)
<mwhudson> nhandler: oh strange
<mwhudson> nhandler: i lose track between what's planned and what's implemented i'm afraid :/
<nhandler> mwhudson: Not a problem. The same thing happens to me with some of my other projects.
<mwhudson> nhandler: review sent -- closer, but not quite there yet
<nhandler> mwhudson: 3rd time is the charm. I just pushed the changes you suggested
<mwhudson> nhandler: awesome
<mwhudson> nhandler: i see you've signed the agreement so i'll see about landing this branch
 * nhandler hugs mwhudson 
<nhandler> You rock mwhudson !
<mwhudson> nhandler: can you supply a commit message at https://code.edge.launchpad.net/~nhandler/launchpad/bugfix296469.2/+merge/13103/+edit-commit-message ?
<nhandler> mwhudson: Sure. Should it follow the format on https://dev.launchpad.net/PQMCommitMessages ?
<mwhudson> nhandler: yes, basically
<mwhudson> nhandler: you don't need to worry about the stuff in square brackets (the [r=foo] stuff)
<mwhudson> nhandler: the script i'll be using adds that
<nhandler> mwhudson: Done
<mwhudson> nhandler: i trimmed it a bit, but thanks!
<nhandler> mwhudson: So there isn't a need to include the bug number?
<mwhudson> nhandler: no, that page is a little out of date really
 * nhandler loves outdated documentation ;)
<wgrant> bigjools: Can I grab a tiny review from you?
<bigjools> wgrant: if it's tiny, yes
<wgrant> bigjools: I hope it's sufficiently tiny: https://code.edge.launchpad.net/~wgrant/launchpad/bug-433739-debug-archive-urls/+merge/13107
<bigjools> yes, it's tiny :)
<bigjools> wgrant: I'll land it for you
<wgrant> bigjools: Thankyou.
 * bigjools wishes I could do that without making a local branch
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<wgrant> bigjools: ec2 land!
<bigjools> I don't use ec2
<wgrant> Pfft.
<bigjools> I think I'd still need a local branch with that anyway
<bigjools> I wish pqm knew about lp: urls
<BjornT> bigjools: you don't need a local branch, all you need to do is to send a mail to pqm telling it to merge the branch
<bigjools> BjornT: true, but not exactly user-friendly :)
<BjornT> bigjools: another option is to hack pqm-submit not to require a local branch :)
<bigjools> or put a "land me!" button on the MP
<BjornT> web UIs are overrated
<bigjools> let's go 100% API!
<jml> more ctk apps.
<jml> gtk apps, I mean
<bigjools> you mean qt
<BjornT> let's not fight over gtk vs qt, and settle for cli instead
<BjornT> adeuring: can you review this small patch? https://code.edge.launchpad.net/~bjornt/launchpad/jscheck-all-layers/+merge/13113
<adeuring> BjornT: sure
<adeuring> BjornT: r=me
<BjornT> thanks
<al-maisan> hello adeuring, how are things?
<adeuring> al-maisan: fine, thanks. Need a review ;)
<al-maisan> sure :)
<al-maisan> https://code.edge.launchpad.net/~al-maisan/launchpad/unembargo-443075/+merge/13114
<al-maisan> adeuring: re. the branch: the code that send the announcement email is moved to PU.realiseUpload() because that method is invoked in a context that has access to the restricted librarian. This is not the case for the acceptFromCopy() method that thus failed in a production setting.
<adeuring> al-maisan: thanks for the explanation!
<al-maisan> sorry for not putting that in the cover letter
<al-maisan> adeuring: I will need to break for lunch in approx. 15 minutes but will be back later.
<adeuring> al-maisan: enjoy lunch!
<al-maisan> :)
<mrevell> adeuring: Nice simple review for you :) https://code.edge.launchpad.net/~matthew.revell/launchpad/help-continue-button-bug-406394/+merge/13115
<adeuring> mrevell: sure. But let me first finish a review for Muharem
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: al-maisan || queue: [mrevell]  || This channel is logged: http://irclogs.ubuntu.com
<mrevell> of course :)
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: mrevell || queue: []  || This channel is logged: http://irclogs.ubuntu.com
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> mrevell-lunch: r=me
<mrevell> thanks adeuring
<adeuring> mrevell: welcome :)
* barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> adeuring: morning!  i am both chr and ocr today, so we'll see how it goes
<adeuring> barry: morning!
<adeuring> barry: if you want to check this sort of multitasking right now, I have an MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-2/+merge/13118 ;)
<barry> adeuring: i'll put it on the list.  i have a meeting coming up but i'll take a shot at it
* barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> barry: thanks!
<allenap> adeuring: Can you review an ec2test-remote.py change please? https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-remote-non-ascii-issue-bug-447247/+merge/13126
<adeuring> allenap: sure
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: allenap,adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<allenap> adeuring: I've noticed an issue with it, sorry. It's small, so I'll add paste you an incremental diff.
<adeuring> allenap: ok, np
<allenap> adeuring: Comment added with the diff. Thanks.
<adeuring> allenap: nice branch! just one question: can we be sure that summary is alway a Unicode instance? Or: Does it alsways have an encode() method?
<allenap> adeuring: I think it will always be a unicode instance, but even str has an encode method so I think it'll be fine.
<adeuring> allenap: ah, right. so, r=me
<allenap> adeuring: Thank you :)
<adeuring> allenap: welcome :)
<barry> adeuring: review sent
* barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: allenap,- || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<adeuring> barry: thanks!
<barry> rockstar: thanks.  i actually had a screenshot in the bug report, but forgot to mention it in the mp.  i wish you could attach things to the mp :/
* adeuring changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<bac> barry: time for a very simple review?
<rockstar> barry, I can has review?
<barry> bac, rockstar put it on the queue.  i can get to it around chr duties i think
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: [bac,rockstar]  || This channel is logged: http://irclogs.ubuntu.com
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-435599/+merge/13141
<bac> barry: it's really more like an rs
<rockstar> barry, chr AND Orc duties today?  Eep.
<barry> rockstar: yep!
<barry> bac: looking
<bac> in fact, i feel almost silly asking
<rockstar> barry, I'm wondering if that's such a good idea.
<barry> bac: r=me :)
<barry> rockstar: probably not :)
<bac> barry: thanks.
<rockstar> barry, I'll add it to the reviewer agenda.
<barry> rockstar: interestingly, this is not the first time it's happened to me
<bac> rockstar: i re-arrange my schedule so it doesn't happen
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: [rockstar]  || This channel is logged: http://irclogs.ubuntu.com
<rockstar> bac, yeah, that's probably a good idea.
<barry> rockstar, bac agreed, though today it's been fairly light
<rockstar> barry, https://code.edge.launchpad.net/~rockstar/launchpad/cleanup-javascript/+merge/13143
<rockstar> barry, no real code changes, just making the existing tests part of a test case with CodeWindmillLayer so that the tests can be run with bin/test
<rockstar> barry, really I should have landed this branch a while ago.
<barry> rockstar: why no diff on the mp?
<rockstar> barry, I think a script has exploded.
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: rockstar || queue: []  || This channel is logged: http://irclogs.ubuntu.com
<barry> rockstar: do you think this will be fixed soon?  i'll be around for maybe another hour
<rockstar> barry, how bout I just give you diff?
<barry> rockstar: that'll work
<rockstar> barry, http://pastebin.ubuntu.com/289549/
<barry> rockstar: delete line 149 or 150
<rockstar> barry, ah yeah.
<barry> rockstar: delete line 235, 236 or 237
<barry> rockstar: nice simple branch.  everything else looks fine.  r=me
<rockstar> barry, thanks.
 * rockstar was a little whitespace crazy
<barry> :)
<EdwinGrubbs> barry: do you have time to review a test restructuring? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-430708-registry-windmill-layer/+merge/13148
<barry> EdwinGrubbs: i can look
<barry> EdwinGrubbs: it looks like you have conflicts in your branch.  see lines 8-12, 21-27
<barry> etc
<EdwinGrubbs> barry: hmm, l merged in trunk earlier today, but I'll do it again.
<EdwinGrubbs> barry: oh, now I see. I was still pushing up my branch changes when launchpad computed the diff. Here is a good diff. http://pastebin.ubuntu.com/289610/   I believe the merge proposal will automatically recompute it in a little while.
<barry> EdwinGrubbs: cool
<barry> EdwinGrubbs: line 132: can't that just be range(5)?
<barry> EdwinGrubbs: also, it looks weird.  why do the wait that way and not just provide a longer timeout?
<barry> EdwinGrubbs: same at line 160
<EdwinGrubbs> barry: I didn't realize that range() had a different behavior with just one argument. I'll do that.
<barry> EdwinGrubbs: yep, start is optional. still, why wait in a loop rather than just increase the timeout?
<EdwinGrubbs> barry: the reason that it can't just have a longer timeout is that it successfully passes the wait and then fails on the next assertion that tests the exact same thing. I really don't know why that would happen.
<barry> yeesh
<EdwinGrubbs> and I've been tweaking it for the last week with no better insight
<barry> EdwinGrubbs: ok.  please add a comment explaining this to both spots
<EdwinGrubbs> sure
<barry> EdwinGrubbs: cool  r=me
<EdwinGrubbs> barry: thanks
* barry changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com
#launchpad-reviews 2009-10-10
<rockstar> Anyone around for a review?
<rockstar> It's not very big, and pretty trivial.
<rockstar> It's 72 lines small.
<rockstar> sinzui, maybe you?
<sinzui> sure
<rockstar> sinzui, pushing now, I'll get you a pastebin you the diff while I propose for merge.
<sinzui> okay
<rockstar> sinzui, http://pastebin.ubuntu.com/289680/  The generated diff is going to be misleading, since it's based on a branch that barry already reviewed.
<rockstar> sinzui, https://code.edge.launchpad.net/~rockstar/launchpad/fix-code-js-tests/+merge/13156
<sinzui> rockstar: I think you decided that testing for a url was not as interesting as testing for the link that uses the url
<rockstar> sinzui, yes.  I was using the url because it worked, but for some reason, it didn't anymore.  I just went for any way that would allow me to click the link.
 * sinzui nods
<rockstar> sinzui, basically, we should use the factory instead of doing this by hand, but canonical_url is still pretty silly for windmill yet.
<sinzui> r=me
<rockstar> sinzui, great, thanks.
<sinzui> now if I could just find someone who would explain why karmic broke my autogen.sh
<nhandler> I got a buildbot failure email from canonical-launchpad@ today. It links to lpbuildbot.canonical.com, but I am unable to view the details of the build. What should I do?
<wgrant> nhandler: It's already fixed.
<wgrant> nhandler: Sadly at the moment we just have to hope it wasn't us.
<nhandler> I was looing at Bug #231168. The +addsubscriber page for bugs says "The person you would like to subscribe to this blueprint. They will be notified of the subscription by e-mail, if they have an active Launchpad account. Should that be updated as well?
<mup> Bug #231168: Subscribe someone else says: "...if they have an active account." <trivial> <ui> <Launchpad Blueprints:Triaged> <https://launchpad.net/bugs/231168>
<jml> nhandler, parsing...
<nhandler> I copied/pasted the wrong message. The +addsubscriber for bugs reallys says "The person's Launchpad ID or e-mail address. You can only subscribe someone who has a Launchpad account."
<jml> nhandler, ahh ok, so the bugs one is basically ok then, AIUI?
<nhandler> jml: The statement is correct that you can only subscribe someone who has a Launchpad account, but is it really needed? I would think that it would make more sense to have that show up as an error message instead of "Invalid value"
<jml> nhandler, ahh, good old "Invalid value"
<jml> nhandler, I'm going to search to see if there's a bug that was marked "Fix Released" precisely because that text got added to the dialog :)
<jml> nhandler, so, it doesn't look like the text was added as a reaction to a bug
<jml> nhandler, I would rather the text stay there until the error message can be fixed, because I personally would read that text as allowing me to subscribe non-launchpad users.
<nhandler> jml: So to make sure I am understanding you correctly, for the message on the bug page, you are fine having the text modified after the error is fixed, correct? And what about the message for blueprints (the one mentioned in the actual bug report)?
<jml> nhandler, sorry, back.
<jml> nhandler, yes that's correct.
<jml> nhandler, allow me to form an opinion on blueprints first :)
<jml> https://bugs.edge.launchpad.net/malone/+bug/344459 is an interesting, related bug
<mup> Bug #344459: Subscribe someone to this bug link should send the person a subscription request if they do not currently have an account <feature> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/344459>
<jml> hmm.
<jml> nhandler, maybe you should remove that message for bugs.
<nhandler> jml: I have to run out for a little bit. I'll look into this issue more when I return.
<jml> nhandler, ok, thanks.
<jml> nhandler, sorry for being so laggy :)
#launchpad-reviews 2010-10-11
<lifeless> mwhudson: if you're still around, I have a monday patch for you. https://code.edge.launchpad.net/~lifeless/launchpad/run/+merge/38091
* lifeless changed the topic of #launchpad-reviews to:  On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/run/+merge/38091]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mwhudson> lifeless: + """kill process and BLOCK until process dies.
<mwhudson> missing an initial capital
<mwhudson> that's it
 * mwhudson does the clicky clicky
* mwhudson 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
<lifeless> mwhudson: thanks; can't land right now of course :P
#launchpad-reviews 2010-10-12
<thumper> lifeless: ping
<thumper> mwhudson: ping
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/better-errors-for-translate-path/+merge/38178
<thumper> jtv: ping?
<jtv> thumper: pong!
<jtv> need a review?
<thumper> please
<thumper> it is very simple
<jtv> I know, I've done it before thanks
 * thumper pokes jtv
 * jtv does not like that
 * thumper pokes jtv
 * thumper sniggers
<jtv> Good thing I just cleaned out the pigsty in the kitchen and got the coffee brewing.  I'm in a good mood.
<thumper> good
<jtv> Yes, good.
<jtv> Wow, that's a long opening sentence in the MP description.
<jtv> thumper: in test_translatePath_branch_alias_no_linked_sourcepackage_branch, isn't the path you're constructing a sourcepackage path?  The comment says it's a distroseriessourcepackage.
<jtv> Sorry, I got the terminology all wrong there.
<thumper> technically
<jtv> I mean: aren't you constructing a distribution source package path?
<thumper> yes
<thumper> I am
<lifeless> thumper: I've rc'd it for you
<jtv> A distroseries source package is actually a source package, isn't it?
<thumper> lifeless: ta
<lifeless> thumper: but please let edwin know directly.
<jtv> hi lifeless
<lifeless> hi jtv
<thumper> jtv: not really
<thumper> jtv: but the linking is at the source package leve
<thumper> l
<thumper> a branch linked for a distribution source package is the source package linked to the active distro series
<jtv> I see
<jtv> thumper: another thingâ¦ I can't make head or tails of the comment in test_translatePath_branch_alias_invalid_product_name
 * thumper hangs head
<thumper> sorry
<thumper> forgot about finishing that
<jtv> Ha!  Got you.
<thumper> finished off and pushed
* StevenK changed the topic of #launchpad-reviews to:  On call: - || Reviewing: - || queue: [StevenK*3]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi jtv, would you have time for a translations review?
<jtv> bac: I would
<jtv> fork it over
<bac> jtv: thanks!  https://code.edge.launchpad.net/~bac/launchpad/bug-652280-pg-trans/+merge/38195
<jtv> bac: how's the heat?
<bac> jtv: 84F ... nothing to complain about
<bac> jtv: the smoke and smog -- those i'll complain about
<jtv> only 29Â°C?  wow, nice & cool
<jtv> or as they would say here, collllddd!
<jtv> It's also about 302 Kelvin
<jtv> apparently
<jtv> bac: with all the unexplained parameters to create_initialized_view that are just passed on to create_view, I'm beginning to wonder if it wouldn't make more sense to accept and pass on **kwargs.
<jtv> (Had somebody done that before, you wouldn't have needed to touch it now)
<bac> jtv: actually i had to do it in another branch too that is stalled...
<jtv> bac: so what say you?
<bac> jtv: sure, i'll do that
<jtv> Excellent.
<bac> jtv: if you have any insight into my other problem i'd like to chat about it.  it is resolved with a fine work-around but i'm bugged that getting and rendering a view doesn't work
<jtv> bac: I haven't gotten to that bit yet, but I've been revisiting my modest recollections of how others did this.
<jtv> Something else again: in project.py, has_translatables:
<jtv> don't use "query_result.count() > 0" to establish existence.  Use "not query_result.is_empty()".
<bac> jtv: yes, of course
<jtv> (Storm's implementation of that is still not optimal, but at least this way you don't stop it from becoming optimal)
<jtv> bac: has the requirement of dromedaryCase in method names finally been abolished?
<bac> jtv: no, i just probably munged something.  where?
<jtv> The helper methods in TestRobotsMixin.
<jtv> And the rest of the test.
<bac> jtv: yeah, i was just on crack
<jtv> oh that's alright then (!)
<jtv> bac: just pulled your branch so I can play along
<bac> jtv: wrt has_translatables on the view, i just saw there is the same method on the context so i can just delete it and the view tests.  yay.
<jtv> bac: but you deliberately replaced references to the context object in the TAL with references to this new view property, and I thought that was actually a good thing.
<bac> jtv: no i didn't
<bac> i replaced context/translatables, which is the resultset
<bac> i should've used context/has_translatable
<jtv> ahhh
<bac> jtv: of course the has_translatable on the model needs to be changed to use is_empty, so i don't escape completely!
<jtv> :)
<jtv> bac: what surprises me is that if I replace the body of get_rendered_contents with just "return self.get_view()()" I only see one test fail.
<bac> yes
<bac> jtv:and that is for distroseries, yes?
<bac> and it fails on 'templates', no?
<jtv> bac: no, it's TestRobotsProductSeries.test_LAUNCHPAD_does_not_block_robots
<jtv> KeyError: 'imports'
<bac> jtv: you would see another failure except i inadvertantly included an extra get_rendered_contents in the test for distroseries
<bac> jtv: delete it and you'll see another failure
<bac> jtv: but i'm glad you just pointed out the redundancy
<jtv> well, "stumbled into it without even realizing it afterwards" would be more appropriate
<jtv> So now test_LAUNCHPAD_does_not_block_robots fails for both ProductSeries and DistroSeries, though not for ProjectGroup.
<jtv> (Is there one for ProjectGroup?)
<bac> jtv: a failure?  no.  likely the page template doesn't use a menu for PG
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: StevenK || queue: [StevenK*2]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> bac: I wonder if it's this lineâ¦
<jtv> lib/lp/translations/templates/person-navlinks.pt:      <a tal:replace="structure view/menu:navigation/imports/render"/>
<bac> yes.  yes that is it
<bac> for distroseries it is the one for 'templates'
<jtv> bac: that makes me wonderâ¦ why is that menu even being referenced for an anonymous access? âthis is anonymous access, isn't it?
<bac> yes, but i don't think it is relevant
<bac> in the web UI those links are visible to the anon user
<bac> again, i think the problem is that the view is registered in the zcml on a layer but not the facet
<bac> and it could be the redirection view stuff y'all do
<jtv> ah
<bac> jtv:  hey, would you mind looking at sqlobject.py's is_empty()?
<bac> i mean in storm
<jtv> bac: not at all, hang on
<jtv> bac: that looks a bit like a bug doesn't it
<bac> yeah, it it really messes up my tests
<jtv> def is_empty(self): result_set = foo() ; return not result_set.is_empty()
<jtv> bugger with bells on
<jtv> that's pretty serious
<bac> so, are we not using it anywhere?  i guess all the call sites using is_empty are pure storm
<jtv> I guess.  We've been using workarounds in Storm, and cast-to-bool in SQLObject.
 * jtv strides off towards #storm
<jtv> bac: I'd file a storm bug for this if I were you.
<bac> i would if i could get to LP.net
<jtv> !?
<jtv> same problem as spiv?
<bac> dunno.  is spiv having a problem?
<bac> it worked moments ago so i expect it is transient
<jtv> He was having problems with SSL connections.
<bac> hmm, dns lookup error
<bac> and we're back
<jtv> â¦except I'm not getting a whois record for code.launchpad.net
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: StevenK || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> gmb: Fancy another one on the stack? Also, can you do UI reviews?
<bac> jtv:bug 659078
<_mup_> Bug #659078: sqlobject is_empty and __nonzero__ are incorrect <Storm:New> <https://launchpad.net/bugs/659078>
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: allenap || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: in order: Yes and No
* allenap changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: allenap || queue: [allenap]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: Er...
<bac> jtv: in light of this bug, i'll put an XXX in the spot i was going to use is_empty until it is resolved.
<allenap> gmb: Oops
* allenap changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: allenap || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> :)
<gmb> allenap: I'm assuming its the one on +activereviews already
<gmb> bug 656823
<allenap> gmb: Yip.
<_mup_> Bug #656823: Subscribing to a search lacks a UI <story-subscribe-to-search> <Launchpad Bugs:In Progress by allenap> <https://launchpad.net/bugs/656823>
<gmb> Cool
<jtv> jtv: yes, makes sense
<bac> jtv: in create_initialized_view, some of the named parameters are actually used in the method (form and method) so they can't really be grouped in with kwargs.  unfortunately they are in the middle of the argument list.
<jtv> bac: would it be too nasty to read them as kwargs.get(parameter, default)?
<bac> jtv: also create_view already takes a kwargs
<bac> jtv: my inquisitiveness into how that would work is trumped by my need to go get dinner.  let's talk about it tomorrow.
<jtv> bac: ok
<jtv> I'll just add my comments to the MP.
<bac> thanks jtv
<gmb> allenap: Code looks good to me.
<gmb> It'll be interesting to see if and how we eventually integrate that with +subscriptions...
* 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
<allenap> gmb: Cheers. Is there a plan somewhere for what +subscriptions will evolve into?
<StevenK> gmb: I've commented on https://code.edge.launchpad.net/~stevenk/launchpad/db-add-derivedistroseries-api/+merge/38189 ; I'm happy with everything else you pointed out.
<gmb> allenap: There's a mockup. Not sure that constitutes a plan.
<gmb> StevenK: Looking now, thanks.
<gmb> StevenK: Okay, I've replied. I suggest that you just expand the loop to a series of `if not <param>: raise DerivationError("Useful message")` statements.
 * gmb -> lunch and errands
* 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
* leonardr changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: lunch || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: lunch || queue: [jcsackett]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> MP for my review is here: https://code.edge.launchpad.net/~jcsackett/launchpad/code-configuration-652142/+merge/38015
<jcsackett> no huge rush, just making sure it's in the queue this morning. :-)
<leonardr> jcsackett, i'll take a look
<jcsackett> leonardr, thanks.
* leonardr changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: lunch, jcsackett || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> jcsackett, r=me
* leonardr changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: lunch, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> leonardr: thanks.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb, leonardr || Reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Ooops.
* gmb changed the topic of #launchpad-reviews to:  On call: leonardr || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gary_poster> leonardr: care to eyeball this to make sure I haven't done something stupid somehow in what should be the dumbest commit ever?  https://code.edge.launchpad.net/~gary/launchpad/production-devel-test/+merge/38239
<leonardr> sure
<gary_poster> thanks
<leonardr> gary, r=me
<gary_poster> cool thanks
<gary_poster> leonardr: could you actually approve it in the MP?  That will make it easier to land
<leonardr> gary: sorry!
<gary_poster> np
<leonardr> done
<gary_poster> thanks
<gary_poster> leonardr: two more, with low priority because I won't land them till after the release.
<gary_poster> (1) https://code.edge.launchpad.net/~gary/launchpad/bug656213/+merge/38242 stub has already approved this one outside of the MP.
<gary_poster> (2) https://code.edge.launchpad.net/~gary/launchpad/bug650343-2/+merge/38224 is big but stupid--a few moved files and making lint happy, in response to a review of an earlier branch.
<leonardr> ok, taking a look
<gary_poster> thanks
<leonardr> gary, line 284-ish of your second branch
<leonardr> you erase the distinction between doctests with logging and doctests without. that's your intention?
<gary_poster> leonardr: yes, because we don't have anymore in that class
<leonardr> ok
<leonardr> gary: line 562: "no signature is treated" is ambiguous, how about "a message with no signature"
<leonardr> line 675, "message message"
<leonardr> er, 576
<gary_poster> leonardr agree, good catch, for both.
<leonardr> gary, can you explain your change to the security policy around line 611? are you running the tests in a different layer to save time?
<leonardr> (ie. the LaunchpadZopelessLayer)
<gary_poster> leonardr: I'm duplicating the way this test was run before, when it was a part of that "doctests_with_logging" code.  It seemed that this was reasonable before, and I didn't think about it much beyond that TBH.
<gary_poster> Using the real Launchpad security policy rather than the permissive one that the zopeless layer usually uses is a Good Thing generally, but that doesn't really explain what is going on here.
<leonardr> gary: maybe you copied that code from test_system_documentation.py?
<gary_poster> that sounds right; verifying
<gary_poster> leonardr: yes, see testSetUp and testTearDown in ProcessMailLayer in est_system_documentation.py
<gary_poster> test_system_documentation.py
<leonardr> gary: is it worth refactoring that code?
<gary_poster> leonardr: you mean to not dupe between canonical/launchpad and lp/services? I briefly considered it.  I rejected it because (1) the canonical/launchpad code is supposed to be all moved eventually, I assume, and I don't know how things will resolve then; and (2) it was so little code that it didn't seem worth it to me.  If you disagree, happy to put it in.
<gary_poster> If you want me to put it in and have any ideas other than two teeny tiny functions for set up and tear down that are used by the two respective bits of code, lemme know.
<leonardr> your #1 reason convinces me
<leonardr> second branch approved
<gary_poster> thank you
<leonardr> trying to wrap my head around the other one
<leonardr> gary: ok, the thing i don't understand about the other branch is why you don't set LPCONFIG when the instance name is 'development'
<gary_poster> leonardr: because it is already considered the default in lincanonical/config/__init__.py ``DEFAULT_CONFIG = 'development'``)
<leonardr> gary: ok, so avoiding that case is how you avoid setting LPCONFIG all the time
<gary_poster> well--I'd say, leveraging that behavior is how I avoid setting LPCONFIG all the time, but maybe that's the same thing
<leonardr> gary: ok, r=me on that one
<gary_poster> cool, thanks again leonardr
<bdmurray> leonardr: would you mind reviewing https://code.edge.launchpad.net/~brian-murray/launchpad/person-subscription-story/+merge/37920
<leonardr> bdmurray, sure
<leonardr> bdmurray, you talk about 'webster' before that user exists. you should mention that you're creating that user
<leonardr> can you explain where the 243656 comes from on line 69?
<leonardr> you're also mentioning bug 16 in the text, though it looks like you removed it from the code
<_mup_> Bug #16: "Swedish" and "Swedish (Sweden)" should be the same language <Launchpad Translations:Fix Released by daf> <https://launchpad.net/bugs/16>
<bdmurray> leonardr: okay re: creating webster.  243656 is the subscriber id.  and got it re # 16.
<leonardr> bdmurray, that subscriber id seems like the kind of thing we're trying to remove from tests?
<bdmurray> leonardr: yeah, I'm working on that now
<leonardr> ok
<leonardr> bdmurray: review sent
* leonardr 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-13
<bac> hi jtv
<bac> jtv: i'm answering your review.  the reason i made 'target' and 'naked_translatable' properties was so i could have the base class show them with a NotImplementedError, which I thought was more explicit than just a comment stating they are expected to be provided by subclasses.  Overkill?
<lifeless> bac: I think that that is ok, but of questionable value
<bac> lifeless: yeah, i gutted it
<lifeless> bac: just a ':ivar target: ....' in the class docstring is enough IMO
<jtv> bac: does bzr lp-send make you write good cover letters somehow?
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jtv: what are you asking wrt lp-send?
<jtv> bac: well I said your cover letter was good, and you replied that it was something to do with bzr lp-send.
<bac> jtv: well, lp-send, if you have the right plugins installed, creates the nice template.  but you knew that, right?
<jtv> no, I had no idea.
<jtv> Could you show me?
<bac> now the quality of the text inserted is up to the user
<jtv> Of course.
<jtv> But a template would help.  I'm just looking at another MP that skips such things as tests, lint, and pre-imp.
<jtv> bac: how do I set up lp-send?
<bac> jtv: http://pastebin.ubuntu.com/512173/
<bac> i *think* lpreview and lpreview_body are required
<bac> lp:lpreview-body, i think
<jtv> I just created the lpreview linkâ¦ where do I get lpreview-body?
<bac> i thought this was common knowledge
<bac> ^^
<bac> bzr lp:lpreview-body lpreview_body
<bac> er, put a 'get' in there
<jtv> Perhaps it is.  But I'm always a thousand emails behind and there are no reviewer meetings timed well for this part of the world.
<bac> this is all abentley's work from ages ago, i think
<bac> jtv: don't i know it!  :(
<bac> 7am is too early for you?
<jtv> bac: yes, that's about 2 hours before I start.
<bac> so if i sleep late i shouldn't count on you to cover me?
<jtv> I could get up earlier, but that comes out of overlap with my team
<bac> between the chickens and the time adjustment i am up plenty early
<jtv> bac: probably notâ¦ it's worth trying, but last night I gave up trying to get help for staging around 02:00 AM.
<jtv> You can imagine what that does to the early rising
<bac> jtv: i look forward to reading your next merge proposal
<jtv> :)
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<flacoste> me
<bdmurray> leonardr: my branch from yesterday is ready for re-review
<leonardr> bdmurray, roger
<leonardr> bdmurray, r=me with minor changes
<jcsackett> anyone have time for a code review on this? https://code.edge.launchpad.net/~jcsackett/launchpad/projectgroup-branches-652156/+merge/38355
<jcsackett> mostly template work and tests.
#launchpad-reviews 2010-10-14
<bac> hi jtv
<jtv> hi bac
<bac> jtv: so i was thinking about the goals stated in the ArchitectureGuide
* 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
<bac> one was new tests under 2 seconds
<bac> i knew the one i recently wrote for testing noindex,nofollow was really slow.  turns out 42 seconds.
<jtv> whoa!
<bac> jtv, so i just spent about 15 minutes reorganizing it and now it runs in under 11 seconds
<bac> good, right?
<jtv> All because of the rendering?
<bac> but at what cost?
<bac> http://pastebin.ubuntu.com/512987/
<jtv> Yes, much betterâthough apparently still not enough
<bac> for one, i avoid the costly setup but putting everything in one test.  so it is much faster but violates the idea that we only test one thing at a time
<bac> caching of some properties helps too
 * jtv hates piling implicit state into test cases
<bac> yeah, so i think it is not as explicit or readable but four times faster
<jtv> I don't think test_robots is so badâ¦ it's still only a few lines, and very readable.
<bac> it isn't that it is unreadable but that it is doing too much
<jtv> Oh crap, I need to get out before the rain
<bac> ok, well i'd like to chat some more later
<bac> funny the rain just hit here too
<bac> it's quite loud crashing on hundreds of tin roofs
<jtv> Must be the same cloud.  The test is not doing _that_ much.  What if you made the implicit state explicit as local variables, then turned the actual testing into a data-driven loop?
<bac> i don't follow
<bac> if you need to go we can talk later
<jtv> A dict could map "for this enum value, I expect the noindex bit to present: {True, False}
<jtv> "
* 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
<jtv> Then test for the various enum values in a loop.
<jtv> Figure out the url etc. once at the head of test_robots, and keep in local variables.
<jtv> Pass those as explicit parameters to your assertion helper.
<jtv> Then test_robots will consist of 3 parts:
<jtv> * Setup
<jtv> * Definition of inputs and expected outcomes
<jtv> * Loop verifying outcome for each input.
 * jtv checks cloud cover
<jtv> yes, I can still just make that
<allenap> gmb: Would you be able to review a branch for me? It's a teensy bit long, but I've added a guide to the changes, and it's mostly moves and deletions anyway. https://code.edge.launchpad.net/~allenap/launchpad/ditch-get-bug-notifications-recipients-bug-659085/+merge/38325
<gmb> allenap: Sure. Tell you what; I'm just writing a cover letter for my branch now. Let's swap :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> gmb: Perfect :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jelmer || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: https://code.edge.launchpad.net/~gmb/launchpad/make-mailnotification-care-bug-656759/+merge/38404, FTR.
<gmb> allenap: I like your definition of "A teensy bit."
<allenap> jelmer: What signal handler does Python install by default? signal.getsignal(SIGPIPE) return SIG_IGN normally, and I can't find a reference to new signal handlers in subprocess? (I'm just interested, not questioning.)
<allenap> gmb: :)
<jelmer> allenap: Hi!
<allenap> jelmer: Hi :)
<jelmer> allenap: Yep, but it needs to be set to SIG_DFL rather than SIG_IGN.
<jelmer> Colin does a much better job at explaining it than I can in this blogpost : http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html
<allenap> jelmer: Oh cool, thanks.
<gmb> allenap: r=me
<allenap> gmb: Woohoo, thank you! I'm just about to start yours, just been finishing off Jelmer's.
<gmb> Cool
* 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> allenap on line 76 of the diff I've got BugNotificationLevel.COMMENTS as a default value for subscribe(level=). I've pushed a fix for the just now.
<allenap> gmb: Spooky, I was just looking at that exact thing. What should it be?
<gmb> allenap It should be level=None
<gmb> (It's bad form to pass values other than None as defaults, since they're then references to the original, which can cause things to blow up)
<gmb> foo=[] is the canonical example of how not to do it.
<allenap> gmb: Ah, I was looking at something else then (I didn't check the line numbers). However, in the branch of mine you just reviewed the level defaults to NOTHING. Should we choose the same default? I don't know which one makes more sense.
<gmb> allenap: Let me look again at your branch...
<gmb> Oh, arse.
<gmb> allenap: So, you're right.
<gmb> The default for subscribe should be COMMENTS (i.e. subscribe to *everything*) but the default for get*Subscribers() should be NOTHING, as you say.
<gmb> I'll update it.
<gmb> Nice catch!
<jelmer> allenap: Thanks for the review and useful feedback. Your first comment has me confused though - since I'm calling assertContentEqual, is it still necessary to sort the lists I'm comparing?
<allenap> jelmer: Ah, I've never used assertContentEqual. I'll take a look...
<allenap> jelmer: Ah, no, that does it for you. Ignore [1] then.
<allenap> gmb: I have to go out now, but I'll finish the review as soon as I'm back.
<gmb> allenap: Ok.
<jtv-afk> gmb: got a review for you, just cleanups really, but close to the 800-line limit.  Would that be okay?
<gmb> jtv: I'm not on-call today, allenap is.
<gmb> jtv: If he doesn't have time to look at it I'll try and take a peek later.
<jtv> gmb: ah, sorry, this client makes the topic too hard to read
<gmb> np
<jtv> allenap: got time for a branch near the 800-line limit?  It's all cleanups, so nothing particularly hard.
<allenap> jtv: Sure :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> allenap: thanksâI just pushed it slightly over the limit though: https://code.edge.launchpad.net/~jtv/launchpad/recife-pre-sharing-permissions/+merge/38407
<Malaria> Hi, I need a review: https://code.launchpad.net/~malaria/launchpad/fixes-bug-608631/+merge/37996
<Malaria> \topic On call: allenap || Reviewing: gmb || queue: [jtv, Malaria] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<Malaria> Sorry
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [jtv, Malaria] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Malaria: Okay, I'll get to it after jtv.
<Malaria> allenap: ok
<bigjools> allenap: can I add to your queue pls?
<allenap> bigjools: Sure, I can probably get to it.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [jtv, Malaria, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> allenap: it's small, cheers
<bigjools> https://code.edge.launchpad.net/~julian-edwards/launchpad/slow-ppa-search-bug-659129/+merge/38307
<jtv> allenap: I have to be off!
<jcsackett> allenap: how stuffed do you feel your queue is? room for another?
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue: [Malaria, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> jcsackett: Mmm, I don't think I'll get to it to be honest. I have to finish a bit earlier than usual today.
<jcsackett> dig, i'll hunt down someone else. thanks allenap.
<allenap> jcsackett: Sorry about that :-/
<jcsackett> allenap: no worries, i understand.
<gmb> allenap: Ta for the review.
<allenap> gmb: Pleasure :)
<allenap> gmb: Sorry it took so long; I have been interrupt driven all afternoon.
<gmb> allenap: No worries.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: Malaria || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> allenap, i'd like to add https://code.edge.launchpad.net/~leonardr/launchpad/automatically-calculate-request-token-expire-time/+merge/38423 to the review queue
<allenap> leonardr: I don't think I'll get to it today.
<leonardr> ok, np
<leonardr> salgado, maybe you want to take it?
<allenap> leonardr: Sorry. rockstar is also down as OCR today, so maybe he's up for it too.
<leonardr> cool
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: [bigjools] || 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: bigjools || 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
<leonardr> benji, why did you mess around with qt widgets rather than using the keyring library?
<benji> leonardr: if you mean http://pypi.python.org/pypi/keyring, I tried to use it but it ended up just having too many issues
<leonardr> hmmm
<leonardr> can we talk about the issues? do you remember?
<leonardr> the qt widgets look like the result of dealing with some issues as well
<leonardr> and even if you can't use the keyring lib, i'd like to see kwallet/gnome keyring/disk hidden behind a uniform keyring lib-style interface
<benji> sure, let's see: the package is hard to use for people using easyinstall or buildout because it sniffs for the various build dependencies and enables/disables the various interfaces (Gnome, KDE, crypto support for simple files) without any feedback so you have to scower the code to figure out what its dependencies actually are
<benji> once you do that it's not obvious how the things its sniffing for map to packages you can actually install
<leonardr> ok...
<benji> and using the deb packages doesn't help because the one that supports KWallet doesn't actually work: https://bugs.edge.launchpad.net/ubuntu/+source/python-keyring/+bug/657070
<_mup_> Bug #657070: python-keyring-kwallet generates IOError <apport-bug> <i386> <lucid> <python-keyring (Ubuntu):New> <https://launchpad.net/bugs/657070>
<leonardr> since kde doesn't use dbus, i guess keyring_impl got set to gnome by mistake?
<leonardr> ok, so keyring has problems
<benji> Can you elaborate on "kwallet/gnome keyring/disk hidden behind a uniform keyring lib-style interface"?  I don't think user of launchpadlib currently has to worry about the different storage options.
<leonardr> the user doesn't have to, but you've got a lot of conditionals in the code
<leonardr> try gnome, then try kde
<leonardr> i'd like to see that stuff localized to one place
<leonardr> keyring has problems, but we're in a good position to have those problems taken seriously
<leonardr> we've got a feature that a lot of people want
<benji> ok, let me refactor it a bit
<leonardr> benji, i've got a call with gary, let's pick this up afterwards. i might end up saying 'let's fix keyring' and then your refactoring would be for naught
<benji> leonardr: ok; you may want to discuss the keyring stuff with Gary, he was heavily involved in the decision to ditch it
<gary_poster> yeah
<leonardr> benji: ok, i've talked to gary briefly
 * gary_poster is on call.  I think we need to see if we can convince leonardr, benji, as a validation.  I also think we need to have doc/bug(s) (I know you have one already).  That said, my underlying concern is that the underlying implementation decisions of keyring (using the -dev) seem problematic
<gary_poster> so fixing is questionable to me
<gary_poster> but we need to be prepared for concerns like these
<gary_poster> which are reasonable
<bigjools> thanks allenap
<leonardr> benji: having trouble giving detailed thoughts, so let me share a couple gut reactions
<leonardr> 1. i believe you that the keyring library has problems, but it works for the people who wrote it, and i don't want to recreate that situation ourselves
<leonardr> (ie. launchpadlib works for us but is difficult for others to use)
<leonardr> i am ok with the gnome keyring code in your earlier branch, but i am squicked by the widget code in the kwallet branch, even though i assume that's the only way to get access to the kde wallet, and keyring probably does something similar
<leonardr> (that was 2)
<leonardr> 3. we don't have to land this code right away. i don't want to do a launchpadlib release with this code until we do desktop-wide integration by default
<leonardr> so that the cleanup code can just look in .launchpadlib, and won't also have to scan through the wallet for per-application keys
<leonardr> 4. if you haven't already, you should file bugs about the setup problems with the keyring lib
<benji> 1) Not wanting to reinvent the wheel is certainly a good impulse.  I would argue that the keyring implementor did just that though; instead of using the existing Python interfaces to the Gnome keyring and KDE wallet they used their C and C++ (!?) interfaces instead to build their own Python interfaces
<leonardr> i agree that that's dumb, and possibly worthy of another bug (since it introduces dependencies most people don't have)
<benji> 2) not being a QT expert I can't be certain if there is a way to avoid the widget or not.  I tried pretty hard to eliminate it, but KWallet seems pretty intent on having one to talk to.
<benji> 3) ok
<leonardr> so, maybe we should bring in a QT expert
<benji> 4) can do; I didn't bother because after talking to the mentor of the Google Summer of Code student that implemented the library I realized that he wouldn't see it as a bug, they decided to do it that was as a learning excersize for the student
<gary_poster> :-/
 * gary_poster tallies another GSoC -1 :-/
<leonardr> well, the student has learned, so now we can improve it?
<benji> I'd be all for improving it, but convincing them of that may be a challenge.
<benji> My version of "improving" it would be to throw most of it away.
<leonardr> ok, the other route goes in the direction of making our own competitor to the keyring library
<benji> I wouldn't to go that far.  Not every bit of code has to be an external library.
<leonardr> it's still a competitor, even if we're the only ones who use it
<leonardr> and if it's better than alternatives, people may start using it--some people use beautiful soup just for the unicode conversion sub-library
<leonardr> so, a plan is taking shape in my mind
<benji> <shrug>
<leonardr> let's talk to the developers i've talked to in the past: didrocks for ubuntu and riddel for kubuntu
<leonardr> riddell, i mean
<leonardr> riddell can give you an expert opinion on interacting with the kde wallet. talk to him first. see if the widget is necessary and if you wrote it correctly
<leonardr> also get a sense of whether the kubuntu developers would be happy with the keyring library as it is, if they would like the same changes we'd like, or if they want some other changes that would change the way both we and the keyring developers do things
<leonardr> you can show your gnome branch to didrocks but i'm pretty sure it's correct. talk to him about the keyring library
<benji> this seems like a lot of effort for a feature that was an afterthought
<leonardr> it's only an afterthought because you and i use ubuntu instead of kubuntu
<leonardr> one way this could go is that we land your gnome code and tell the kubuntu developers to give us something for kde
<leonardr> how about this. show your kde branch to riddell so we can have an expert evaluate your widget code. don't bother asking him about the keyring library
<leonardr> prepare a section in the launchpadlib TODO or something, 'why we're not using keyring'
<leonardr> refactor the code so that the "try gnome/try kde/try disk" loop is isolated to one place
<leonardr> we'll go with that for now
<benji> ok
<leonardr> but, i think "why we're not using keyring" will be more convincing if you could say "both kde and gnome developers we talked to agree with us on this"
<leonardr> that would also give us a starting point for either improving or competing with keyring, should we decide to do so in the future
<leonardr> i don't see any fundamental reason why we can't all get along
<leonardr> benji, https://code.edge.launchpad.net/~benji/launchpadlib/kwallet/+merge/38366
<leonardr> benji, i'll talk to riddell about reviewing your widget code
<benji> ok
<leonardr> benji, if you want to, you could do a branch that just upgrades setup.py
<leonardr> that could land immediately
<benji-lunch> leonardr: (I think you mean bootstrap.py) I'll do that
<jcsackett> salgado: ping
<salgado> hi jcsackett
<jcsackett> salgado: regarding the ui review i sent your way--i saw that you were needing reviews in the reviewer meeting, but i thought i would check that you are able to hit them right now.
<salgado> jcsackett, do you need it done right now?  I'm have some catching up to do from the two holidays I had earlier this week, but I should be able to do it later today or tomorrow if that's ok with you
<jcsackett> salgado: that's just fine. just wanted to make sure i had read it right that you still wanted them sent your way.
<salgado> I do, definitely!
<jcsackett> excellent. i'll probably have another (small one) to send your way tomorrow. :-P
<salgado> cool. :)
<leonardr> rockstar, i heard a rumor you were ocr today?
<rockstar> leonardr, yeah, I am...
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, where's your branch merge proposal?
* sinzui changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/launchpad/automatically-calculate-request-token-expire-time
<sinzui> rockstar, can you review https://code.edge.launchpad.net/~sinzui/launchpad/obsolete-add-edit/+merge/38377 ? I am not is a hurray since I am not working today
<rockstar> sinzui, okay.  I'll look at it after leonardr's branch.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> rockstar: think you could do a review for me today?
<rockstar> jcsackett, yup.  Hop on the queue.
* jcsackett changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> thanks, rockstar. MP is here: https://code.edge.launchpad.net/~jcsackett/launchpad/branch-collection-links-652126/+merge/38456
<rockstar> leonardr, review sent.  r=me with some changes requested.
<leonardr> rockstar, thanks
<jelmer> rockstar: hi
<rockstar> Hi jelmer
<jelmer> rockstar: can I add another branch to your queue?
<rockstar> jelmer, sure!
<jelmer> rockstar: Great! https://code.edge.launchpad.net/~jelmer/launchpad/135610-duplicated-ancestry/+merge/38444
<rockstar> jcsackett, if you want to land this today, I can do a ui review for you, and then you don't need two ui reviews
<rockstar> jcsackett, the code looks fine.
<rockstar> jcsackett, as far as UI though, I think you should put the commit count, branch count, and user count on the same line.
<rockstar> Active reviews should be its own little section.
<jcsackett> rockstar: i tried that, but you end up with either a really weird block of text, or it looks like the branches and users *also* occured in the commit time period.
<jcsackett> rockstar: you think move active reviews back into its own side thing?
<rockstar> jcsackett, X branches, owned by X people.  There were X commits in the last X period.
<rockstar> jcsackett, the intention of the X commits in the last X period is to show how active the project is.
<jcsackett> rockstar: yeah, that just looked wierd to me, but i'll deal with the text block instead of dividing it up as it is.
<rockstar> It doesn't do a good job, but I think laying it out that way would be better.
<jcsackett> rockstar: regarding active reviews; side thingy, or just a separate line?
<rockstar> Active reviews need to be obvious.  Putting text near it makes it hard to identify.
<rockstar> jcsackett, separate line should be fine.
<jcsackett> rockstar: dig.
<jcsackett> i'll make those changes.
<rockstar> jcsackett, for bonus points, it doesn't show if there are now active reviews.
<jcsackett> rockstar: i'll look into it. didn't want this growing beyond the scope of bridging the gap, but if it's easy to throw in (and it should be) i'll hit it.
<rockstar> jcsackett, yeah.  UI reviews often cause feature creep.  Truthfully though, I think it stays in line with the original purpose of the bug.
<jcsackett> rockstar: dig.
<jcsackett> i'll switch that up and then ping you when i've made the changes, rockstar.
<rockstar> jcsackett, great, thanks.
<jcsackett> rockstar: i've updated. you can see the new looks here: http://people.canonical.com/~jc/images/noreviews.png and http://people.canonical.com/~jc/images/withreviews.png
<rockstar> jcsackett, MUCH better.
<jcsackett> yeah, thanks for the suggestions. :-)
<jcsackett> thanks for the review, rockstar!
* rockstar 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-15
* 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> hi wgrant
<bac> wgrant, i'm trying to review your branch for publisher indices.  i cannot get the tests to run due to a cyclical import problem.  did you see that?
<bac> hi StevenK, you around?
<StevenK> bac: Indeed
<bac> hi StevenK, i'm trying to run some archive publisher tests and am getting import errors
<StevenK> bac: Can you pastebin them?
<bac> the class is being exported so i think it is a cyclical import problem. and it shows up in trunk
<bac> StevenK: will do
<bac> StevenK: http://pastebin.ubuntu.com/513667/
<wgrant> bac: That's well-known.
<wgrant> There was a bug filed a couple of days ago.
<bac> wgrant: ah, ok
<wgrant> You can't use -m lp.archivepublisher or various other Soyuzy things.
<wgrant> I just -t archivepublisher.
<bac> wgrant: thanks
<bac> StevenK: ^^, nm
<StevenK> bac: Yes, I saw.
 * wgrant has another 6 branches to follow that one.
 * StevenK can't land anything due to buildbot being in testfix because db-devel failed
<StevenK> And I can't tell why, and lifeless wants to know why
 * wgrant can't land anything because his branches keep getting rejected with seven different errors.
<wgrant> bac: I'm going to hope that you're not in your usual TZ at the moment.
<bac> wgrant: no, i've relocated for a bit
<bac> UTC+7
<wgrant> Aha.
<wgrant> This makes more sense.
<bac> wgrant: you use method(param=[]) a couple of place.  you know that can cause problems?
<wgrant> bac: Isn't that only a problem if I mutate the arg?
<bac> wgrant: yes.  i *thought* we had something in PSG about always avoiding it
 * wgrant needs to eat now, but will remove it later if you can find a reference.
<bac> wgrant: why not change to a tuple?
<lifeless> wgrant: its highly sensitive to bad changes in the future
<lifeless> wgrant: a tuple or None would be safer
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> morning bac
<bac> hi adeuring
<bac> adeuring: not much going on here.  three branches need review in +activereviews.  i'm starting on gary's
* bac changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: gary,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> bac: ok, I'll take brian's. But I need more coffee first ;)
<wgrant> bac: Ah, true, I could. They used to have a single item each, and the extra two characters to tupleise them felt bad.
<wgrant> But it's easy now, so I will do it.
<bac> adeuring: ok
<bac> wgrant, always taking the easy route.  :)
<bac> thanks
 * bac hates the single-element tuple syntax
<wgrant> Yes.
<StevenK> bac: (Why,) ? :-)
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: gary, bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> bac: Fixed. Could you try to ec2 it?
<bac> wgrant: ugh, the hard part
<wgrant> bac: It hopefully can't beat the number of failures that my other branch has.
<bigjools> thanks for reviewing my branch lifeless
* StevenK changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: gary, bdmurray || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> StevenK: could you explain a bit what the difference is HttpServer and FixedHttpServer, and why is fixes a threading issue?
<StevenK> adeuring: The easiest answer I can give you is the thread on launchpad-dev
<adeuring> StevenK: ah, ok...
<StevenK> adeuring: Using FixedHttpServer is mwhudson's suggestion
<adeuring> StevenK: i was in holiday most of this week. can you tell me the subject of the thread?
<adeuring> StevenK: never mind, found it already
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: gary, StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> adeuring: I'm happy to talk to mwhudson or thumper about this branch if you prefer
<adeuring> StevenK: I just want to understand at least a bit what the magic behind this small change is; let me read the ml thread first ;)
<adeuring> StevenK: r=me. Thanks for fixing this issue!
* adeuring changed the topic of #launchpad-reviews to: On call: bac, adeuring || Reviewing: gary, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> adeuring: i leave the reviewing to you as i EOD/EOW.  :)
<bac> that's nerd for TGIF
<adeuring> bac: nice weekend!
<bac> adeuring: thanks, you too.  nerdfest tomorrow, delayed 10.10 release party
<adeuring> bac: enjoy!
<wgrant> adeuring: I've a rather trivial sed branch for you, if you have a moment.
<wgrant> https://code.edge.launchpad.net/~wgrant/launchpad/bug-661109-buildable-architectures/+merge/38529
* wgrant changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [wgrant, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> wgrant: sure, I'll look. sorry for the late answer - was out for lunch
<jelmer> wgrant: btw, thanks for your comments with regard to my duplicated ancestry branch earlier. I've just put up a new one that just gets rid of the assertion completely.
<wgrant> jelmer: Great.
<wgrant> jelmer: I think that's probably the right idea.
<wgrant> Since it doesn't actually care about overrides.
<wgrant> adeuring: publisher-release-cleanup is a bit messy, unfortunately. But the other one is trivial.
<adeuring> wgrant: no problem. At least "mesy cleanup" sounds interesting ;)
<wgrant> It's the first of probably 7 or so branches to clean up that code :/
<adeuring> wgrant: r=me on buildable-architectures
<adeuring> I'll run it through ecs
<adeuring> ...ec2
<wgrant> adeuring: Thanks.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> On call: adeuring || Reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring: Can I add https://code.edge.launchpad.net/~gmb/launchpad/fix-bugzilla-sans-alias-bug-660873/+merge/38533 to your queue?
<adeuring> gmb: sure
* gmb changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: wgrant || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring: Thanks.
<adeuring> wgrant: r=me for your other mp too
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gmb: r=me
<gmb> adeuring: Thanks.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Can I have an rs=you for merging stable into db-devel to resolve conflicts?
<adeuring> allenap: as far as an rs without a look into the sources is possibble, sure ;)
<allenap> adeuring: It's difficult to show what I did really.... the tests pass though!
<adeuring> adeuring: ok, so rs=me...
<allenap> adeuring: Got time for another? https://code.edge.launchpad.net/~allenap/launchpad/check-mappings-and-sets-for-proxy/+merge/38539
<adeuring> allenap: sure
<allenap> Thanks.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> allenap: r=me
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Thanks.
<jelmer> adeuring: can I add a small branch to your queue?
<adeuring> jelmer: sure
<jelmer> adeuring: Thanks - the branch is at https://code.edge.launchpad.net/~jelmer/launchpad/135610-duplicated-ancestry-2/+merge/38521
* jelmer changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jelmer: would it be difficult to avoid this situation that there are (for a short time, if i understand correctly) two PUBLISHED records?
<jelmer> adeuring: It should be possible, but it is nontrivial.
<adeuring> jelmer: ok, so r=me then
<jelmer> adeuring: Thanks!
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> adeuring, hi.  I have a branch for review if you like.
<adeuring> deryck: sure
<deryck> adeuring, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/better-testing-for-status-changes/+merge/38552
<adeuring> deryck: I think it might make sense to change the test setup so that the product owner is not a member of the bug supervisor team; right now, the term "or user.inTeam(pillar.owner)" in canTransitionToStatus() is not executed
<deryck> adeuring, ok, that's a good idea.  I can do that.
<adeuring> deryck: thanks! another suggestion, before you start ;)
<deryck> sure
<adeuring> deryck: you don't test at all for celebrities.bug_watch_updater, celebrities.bug_importer, celebrities.janitor. You could add these tests quickly by definig a base class for tests of "extended permissions", where you do not define the user who is tested, then derive the "real" test classes for supervisor, owner and the celebrities.
<adeuring> That should make the tests a bit shorter and more comprehensive
<deryck> sure, I like that idea.  I can do that, too.
<adeuring> deryck: cool, thanks!
<deryck> thank you!
<deryck> adeuring, it will probably take me a bit to make those changes, so I'll just ping back when done on Monday, if that works for you.
<adeuring> deryck: sure, no problem
<allenap> adeuring: Got time for a very simple one? https://code.edge.launchpad.net/~allenap/launchpad/wire-up-filter-subs-bug-655567-devel/+merge/38562
<adeuring> allenap: sure
<allenap> adeuring: It's already reviewed and landed in db-devel, so I just need a +1 to land on devel.
<adeuring> allenap: so, rs=me
<allenap> adeuring: Thanks.
<allenap> adeuring: For the sake of my extreme laziness, can you approve the merge proposal so I can use ec2 land? ;)
<adeuring> allenap: one
<adeuring> ...done
<allenap> Thanks :)
<EdwinGrubbs> adeuring, gmb, sinzui: Can one of you review my fix for my previous fix that conflicts with someone else's fix? Arg. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/remove-redundant-initialisedistroseries-section/+merge/38571
<sinzui> I can when the diff updates
* adeuring 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, I see you diff
<EdwinGrubbs> sinzui: what does that sentence mean?
<sinzui> EdwinGrubbs, Why do we not see the other inclusion. Was the other section added in another location?
 * sinzui thinks the file should be in alphabetical order
<EdwinGrubbs> sinzui: right, it was added in the middle of the file.
<sinzui> okay r=me to land
<EdwinGrubbs> salgado-lunch: I have a branch that needs a ui review if you want: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-652232-person-code-action-links/+merge/38574
<salgado> Edwin-afk, sure, I'll take it
<abentley> rockstar: could you please review https://code.edge.launchpad.net/~abentley/launchpad/partial-ancestry-scanner/+merge/38582 ?
<rockstar> abentley, ack.
#launchpad-reviews 2010-10-17
<thumper> mwhudson: you approved this, but didn't review :-| https://code.edge.launchpad.net/~thumper/launchpad/move-garbo-to-lp/+merge/38473
<thumper> ec2 land is complaining
<mwhudson> thumper: fixed
<thumper> mwhudson: ta
