#launchpad-reviews 2010-08-02
<thumper> lifeless: want to eyeball a boring as crap branch which just moves errors from canonical.launchpad.webapp.interfaces into lp.app.errors?
<lifeless> no
<lifeless> +1
<thumper> rs?
<lifeless> yes
<lifeless> you break it you bought it
<thumper> ack
<thumper> lifeless: 2542 lines (+256/-235) 147 files modified :-)
<lifeless> not that app.errors is really any better, but I get the psych value of it ;)
<thumper> lifeless: well, lp.app is where much of canonical.launchpad.webapp is going
<lifeless> yeah, I know
<lifeless> just saying that A.bigbag == B.bigbag :)
<thumper> actually...
<thumper> we are breaking up a general interfaces big bag into two
<thumper> errors and interfaces
<thumper> so in a way, it is being a lot broken up
<lifeless> \o/
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/malone/revision/11225
<lifeless> thats an incremental patch on my make-search-faster branch
<lifeless> anyone care to ok it ?
<thumper> lifeless: where is the merge proposal?
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/malone/+merge/30904
<lifeless> the proposal is approved
<lifeless> but ec2land told me I was foolishly hopeful
<thumper> lifeless: why tweak sample data instead of adding a bug with the factory
<thumper> ?
<thumper> it raises my ick factor
<lifeless> thumper: minimal change to work in with the existing test
<lifeless> thumper: adding a new bug causes the rest of the entire doctest to go boom
<thumper> :(
<lifeless> indeed
<thumper> lifeless: commented.
<lifeless> thanks
<wgrant> lifeless: Nice easy DB review for you: https://code.edge.launchpad.net/~wgrant/launchpad/per-archive-build-debug-symbols/+merge/29671
<lifeless> mmm
<lifeless> is archive really the right scope for this ?
<lifeless> its a bit of a shame all our config knobs are spread all over
 * lifeless tries not to use flags for everything
<wgrant> lifeless: What would be a better scope?
<lifeless> packageset-in-archive ?
<lifeless> I dunno, it just seems rather coarse, or something.
<wgrant> It is coarse, yes.
<wgrant> Packagesets are an interesting idea, but they're not workable yet.
<lifeless> wgrant: dbgsyms is an unnecessary abbreviation in at least one place in the patch
<lifeless> its also a redundant comment in that place too
<wgrant> True.
<lifeless> given the variable name is pretty clear
<wgrant> This branch is a year old, and the terminology throughout the rest of the branches has changed since then.
<wgrant> It is probably removable indeed.
<lifeless> please don't use the string literal there
<lifeless> for commercial-admins
<lifeless> rather use the sampledata module
<wgrant> Hm. Does it really only have one value at the moment?
<wgrant> Or is there another one I'm missing somewhere?
<lifeless> its pretty small right now
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [wgrant(http://bit.ly/dvBcHk)] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> I'm reviewing james_w's sampledata-0 branch btw,
<jml> bigjools, getting to your branch now. sorry about the delay.
<jml> bigjools, done.
<bigjools> jml: you rock, thanks
<jml> bigjools, my pleasure.
<bigjools> jml: the next branch will clean up these damn tests I hope
<bigjools> the "if in doubt return a Deferred" approach is a good one
<bigjools> jml: BTW there's nowhere in the guidelines that says we use US spelling in code
<jml> :(
<bigjools> you mean: :)
<jml> bigjools, no, I don't.
<bigjools> lol
<jml> I hate having to think unnecessarily when reading code
<bigjools> I prefer "international English"
<bigjools> me too
<jml> color
<bigjools> that's why I go out of my way to make the English agnostic
<jml> color, never ever ever colour
<bigjools> the mighty British Empire says otherwise
<jml> what a pity they didn't invent C
<bigjools> ha
<bigjools> FWIW "dialog" to me means a popup.
<bigjools> a dialogue is a conversation
<jml> if you spell it slaveDialogueEnded in the code it's going to look weird to lots of people and eventually someone is going to change it to be slaveDialogEnded
 * bigjools sighs heavily
<jml> feel sorry for the French
<jml> I bet they have discussions about whether it should be self.ordinateur or self.la_ordinateur
<bigjools> jml: ok, so slavePowowEnded it is
<james_w> thanks jml. Just a couple of questions in https://code.edge.launchpad.net/~james-w/launchpad/no-more-sampledata-0/+merge/31470
<james_w> jml: plus there's a nice juicy Collection branch sitting there :-)
<jml> bigjools, "prefer the standard to the offbeat", also, the OED spells it powwow or pow-wow :P
<bigjools> jml: grumble...smartarse :)
<bigjools> does the OED carry US spelling?
<benji> I need a review of a small patch to lazr.restful: https://code.edge.launchpad.net/~benji/lazr.restful/nail-versions/+merge/31421
<benji> it just nails some versions so the tests pass again
<jml> bigjools, yes. main articles are in real English, but it is always explicit when there are regional spelling variations.
<bigjools> jml: so if I use slaveConversationEnded will that satisfy your reviewing juices?
<jml> bigjools, yes.
<bigjools> hurray
* abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: -  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> james_w: replied. I might look at the archive-collection thing.
<james_w> jml: no rush
<bigjools> jml: reply incoming
<bigjools> jml: btw thanks for the collections explanation.  I think I get it, but since I'm more of a JFDI than a theoretical kind of man I think more concrete examples will help.  In due course.
<jml> bigjools, np.
<james_w> bigjools: have you seen https://code.edge.launchpad.net/~james-w/launchpad/archive-collection/+merge/31499 ?
<james_w> I did it because I wanted something very similar to getArchivesForDistribution in the deletion branch, but with one more collection. While you can continue to add more methods to ArchiveSet with the requirements of every caller, letting them construct their query is more flexible.
<bigjools> james_w: yes, right.  Does it build up the same query or issue multiple queries?
<james_w> one query
<james_w> the shape of that query may not be optimal, but that's the case with any new or modified queries
<bigjools> jml: I don't understand why you want IBuilder.getActiveBuild
<bigjools> oh scratch that
<bigjools> I can't fucking read
<jml> bigjools, np. otp right now :)
<bigjools> jml: FWIW it also returns a proxied object and that is force of habit for me :)  anyway ...
<james_w> jml: just to clarify, when you say "test_returnsAll... style rather than testReturnsAll style" do you mean "test_returnsAll_does_something" or "test_returnsAllDoesSomething" ?
<jml> james_w, test_properName_does_a_thing
<james_w> jml: ok
<jml> james_w, archivecollection reviewed.
<benji> I need a review of a small patch to lazr.restful: https://code.edge.launchpad.net/~benji/lazr.restful/nail-versions/+merge/31421
* abentley 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-08-03
<thumper> mwhudson: a trivial review? https://code.edge.launchpad.net/~thumper/launchpad/move-branch-errors/+merge/31602
<mwhudson> thumper: done
<thumper> mwhudson: ta
<mrevell> Anyone care to review a very small change? it's the removal of text from the LP footer. https://code.launchpad.net/~matthew.revell/launchpad/remove-survey-summer-2010/+merge/31628
<jelmer> mrevell-lunch: I'll take it.
<mrevell> thanks jelmer
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: -  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> jelmer, ping, any movement on your buildd-trivial branch?  It looks like you asked Lamont to review it?
<jelmer> mars: Hi
<jelmer> mars: Yep, I've asked for review from him but he's on holidays at the moment. There's no urgency though, just some drive-by cleanups.
<mars> ok, any chance someone else could review it to get the work out of the queue?
<mars> I looked at it, but it is over my head
<jelmer> mars: I'd rather wait for lamont, perhaps I can make it not show up in the general queue somehow though.
<mars> only by switching it to "Work in Progress", which would be a hack
<mars> jelmer, ok, don't worry about it
<jelmer> I've just voted "Abstain" on it, it's no longer waiting for review from launchpad-reviewers now.
<mars> oh, neat idea
<mars> thanks
<adeuring> mars: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-612779/+merge/31637 ?
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant  || queue: [abeuring] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant  || queue: [adeuring] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> adeuring, sure, you're next
<adeuring> mars: thanks!
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: adeuring  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> adeuring, ping
<adeuring> mars: yes?
<mars> adeuring, uhm, that class name, StreamOrRedirectLibraryFileAliasView, is very suspicious
<adeuring> mars: perhaps... What do you mean by "suspicious"?
<mars> if you have AorBorCView, then I would say "use polymorphism and create AView, BView, and CView implementations coming from a factory"
<mars> StreamOrRedirectLibraryFileAliasView and SafeStreamOrRedirectLibraryFileAliasView sounds like an object design problem
<mars> or a poorly named class
<adeuring> mars: sounds reasionable. But lifeless is anyway going to reorganize the whole implementation of acces to restricted LFAs, so I think we can leave this somewhat confusion stuiff as it is for now
<mars> adeuring, perhaps you could shed some light on this: is StreamOrRedirectLibraryFileAliasView two classes, or is it once class and the class name is telling you what the implementation does? (Stream, and it might redirect the stream)
<adeuring> mars: well, I think it is intended to mean "either stream some content or redirect to another URL"
<mars> ok, so an object design problem then
<adeuring> probably
<mars> if it was just a bad class name, that would be easy to fix
<mars> oh well
<mars> adeuring, ok, thanks for the explanation.  I'll make a note about it in my review for when lifeless reads it
<adeuring> ok
<adeuring> mars: this class has other issues as well: It first downloads all data from the Librarian, and once that is done, it begins to deliver the data to the client. Not very cleer for large files...
<mars> eeww, yuck
<mars> hope the librarian isn't slow that day
<mars> oh well
<adeuring> mars: but again, lifeless idea is to serve restricted data directly from the Librarian (secured by some token), so that won't be an issue soon
<mars> I wouldn't know how to fix it myself anyway.  I don't know if the app servers can flush data in their response buffer or not - you would need that if you were to fix it
<mars> interesting.  ok.
<adeuring> mars: right
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> mars: hi, are you reviewing?
<mars> bigjools, yep!
<mars> bigjools, feel free to jump in line
<bigjools> mars: excellent, I've got a small branch
<bigjools> thanks
* bigjools changed the topic of #launchpad-reviews to: On call: mars || reviewing: wgrant  || queue: [bigjools] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: bigjools  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> bigjools, done
<bigjools> mars: thank you
<bigjools> mars: so, no, the doctest is absolutely not the right place for that test but I don't have the time to convert that all to unit tests and I am loathe to write a new unit test and not include the existing ones.
<bigjools> but honestly, that's quite a nice doctest file by soyuz standards :)
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> bigjools, I would really prefer if you added just this one test.  For me, finding a home for new tests is a barrier to entry - if you blaze that trail, then everyone can point others in that direction later
<bigjools> mars: I really, really hate splitting tests up between files, it's extremely confusing
<mars> bigjools, what would be split, exactly?
<bigjools> the unit tests
<bigjools> it's the crappy situation that we've been trying to eradicate in Soyuz tests for the last three years
<mars> they would be split from the doctests, and that's bad?  Or they would be split across N unit test files, and that would be bad?
<bigjools> it's so much harder to develop when you start making changes and think you've fixed all the tests only to get a bunch more fail in a file you didn't remember
<bigjools> so some would be in the doctests and some in the new unit test we're proposing
<bigjools> I would rather move them all at once
<mars> bigjools, and when will you or your team have a single block of time to spend rewriting the entire doctest file into a proper doctest/unit test pairing?
<bigjools> does it matter?
<mars> well, if the answer is "never"... :)
<bigjools> we have worse problems to fix
<bigjools> but tests split all over the place has pissed me off so much in the past
<bigjools> consider me jaded
<bigjools> this was supposed to be a quick fix!
<mars> I understad, but it is quick at the expense of increasing the team's work a few years down the line, and at the expense of our documentation.
<bigjools> I simply don't have the time for this right now, I'm sorry.  I budgeted some time to make this essential fix and now I have 6 other things that all need doing yesterday.
<bigjools> I'll pass it on to someone else if you insist on the change.
<mars> I do insist.  Land what you have, but please have someone refactor it in a follow-up branch, or build on this one.
<mars> bigjools, I'll mark the proposal accordingly so you can land it.
<mars> before your EOD
<bigjools> mars: thanks
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: -  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> mars, can I get a quick review from you? https://code.edge.launchpad.net/~rockstar/launchpad/person-recipe-listings/+merge/31688
<mars> rockstar, yep
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: rockstar || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> rockstar, "earma" ?
<rockstar> mars, no idea what that's about.  Reverting.
<mars> do you use Vim or something?
<mars> :)
<mars> rockstar, looks good, r=mars.  I am surprised no portlet view tests died on this change.
<rockstar> mars, yes, but I also using some fun painkillers, so I might as well be using emacs...
<mars> hehe
<mars> rockstar, approved
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-08-04
* mars changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> lifeless, mwhudson, did either of you get a chance to look at my test_on_merge.py fixes branch?  https://code.edge.launchpad.net/~mars/launchpad/fix-test_on_merge-578886/+merge/31207
<mwhudson> mars: i looked over it, but i thought the previous one was ok so ... :-)
<mars> mwhudson, alright, could you perhaps mark this one as 'Approved' then?  I resubmitted the proposal, which wipes the slate clean.
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/branch-target-adapters/+merge/31698
<thumper> mwhudson: I forgot to add the prerequisite branch on the moved errors from yesterday
<thumper> mwhudson: as I thought it landed
<thumper> mwhudson: but ec2 just didn't land them nor send me an email :(
<mwhudson> mars: looking now
<thumper> mwhudson: it should be only 100 lines or so
<mwhudson> thumper: maybe you should fix that thing to let us set the prequisite branch :-)
<thumper> mwhudson: yes, I'm going to :)
 * thumper goes to lunch
<wgrant> Could someone please review https://code.edge.launchpad.net/~wgrant/launchpad/per-archive-build-debug-symbols/+merge/29671? It has two db reviews already.
<mwhudson> wgrant: i guess a buildd change is following on?
<wgrant> mwhudson: I made the buildd change months ago.
<mwhudson> wgrant: oh ok
<mwhudson> wgrant: + def setBuildDebugSymbols(self, archive, build_debug_symbols):
<mwhudson> 243	+ """Helper function."""
<mwhudson> given that it's invoked preciselye once, and is one line long, i'm not sure how this is helping
<mwhudson> oh
 * mwhudson opens his eysys
<mwhudson> *eyes
<wgrant> I may have copied that from another test in that file, I think.
<wgrant> But it's called a couple of times.
<mwhudson> wgrant: you missed my 'patronizing guide to what makes a good unit test' at the epic :-)
<wgrant> I did indeed.
<mwhudson> there seem to be three tests mushed together really:
<mwhudson> anyone can read
<mwhudson> archive owner can't set
<mwhudson> commercial admin can set
<mwhudson> wgrant: why do you log in as the archive owner to test that anyone can read it?
<wgrant> mwhudson: Good question.
<mwhudson> thumper: could you please split it into three test cases?
<mwhudson> err
<mwhudson> wgrant: ^^
<mwhudson> wgrant: everything else looks fine
 * thumper looks up
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/branch-target-adapters/+merge/31698 plz?
<wgrant> mwhudson: Thanks.
<wgrant> What's the preferred test method naming convention?
<wgrant> I've seen testFooWorks, test_fooWorks, test_foo_works...
<thumper> I use test_<func>_extras
<mwhudson> wgrant: i think it's test_$thing_descriptive_words
<mwhudson> wgrant: if $thing is camel cased, it stays camel cased in the test name
<wgrant> Thanks.
<mwhudson> thumper: reviewed
<thumper> ta
<wgrant> mwhudson: Tests split.
<wgrant> Can you land it?
<wgrant> https://code.edge.launchpad.net/~wgrant/launchpad/bug-605002-P-a-s-linux-any/+merge/31451, https://code.edge.launchpad.net/~wgrant/launchpad/bug-612157-ppa-quota-ddebs/+merge/31471 and https://code.edge.launchpad.net/~wgrant/launchpad/bug-241129-queue-binary-scaling/+merge/31604 would also like to be landed.
<mwhudson> wgrant: no, not yet, still remembering how to configure bits and pieces
<wgrant> mwhudson: Oh, it's that broken?
<wgrant> lifeless: Any chance you could land the four branches above?
<mwhudson> wgrant: yes
<wgrant> mwhudson: :(
<lifeless> wgrant: mwhudson: thanks
<lifeless> wgrant: list the branches please
<wgrant> mwhudson: Hm, you didn't actually approve that branch that you reviewed.
<wgrant> lifeless: https://code.edge.launchpad.net/~wgrant/launchpad/bug-605002-P-a-s-linux-any/+merge/31451, https://code.edge.launchpad.net/~wgrant/launchpad/bug-612157-ppa-quota-ddebs/+merge/31471, https://code.edge.launchpad.net/~wgrant/launchpad/bug-241129-queue-binary-scaling/+merge/31604
<wgrant> mwhudson reviewed https://code.edge.launchpad.net/~wgrant/launchpad/per-archive-build-debug-symbols/+merge/29671, but didn't click the right buttons, apparently.
<lifeless> sending those tree off
<wgrant> Thanks.
<adeuring> stub: could you have a look at this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-39674-update-retricted-flag-of-private-bugattachments/+merge/31563 ? (Robert's concerns are addressed in a different branch that already landed9
<stub> k
<stub> adeuring: done
<adeuring> stub: thanks!
* 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
<jtv> Review needed!  https://code.launchpad.net/~jtv/launchpad/recife-traits/+merge/31729
<jtv> henninge, are you free to review it by any chance?
<wgrant> jtv-eat: If you have time, could you please take a look at https://code.edge.launchpad.net/~wgrant/launchpad/multi-arch-builders/+merge/31740?
<bac> jtv-eat: may i add a simple one to your queue?  https://code.edge.launchpad.net/~bac/launchpad/progressbar_feature_switch/+merge/31749
* bac changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [wgrant, bac] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mrevell> jelmer, ping
<jelmer> mrevell, hi
<mrevell> jelmer, thanks for approving my branch :)
<jelmer> mrevell: you're welcome. are you going to publish the results from the user survey to the list at some point?
<mrevell> jelmer, absolutely :)
<mrevell> jelmer, I'll look through them after the release and work out a good way of using the results.
<bac> EdwinGrubbs: ^^^
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: - || queue: [wgrant, bac] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: I can review your branch. I assume that's what you're pointing at.
<bac> yes, EdwinGrubbs
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: bac || queue: [wgrant] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs, could you please review https://code.edge.launchpad.net/~abentley/launchpad/build-duration/+merge/31655 ?
<EdwinGrubbs> abentley: yes, please put it in the queue
* abentley changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: bac || queue: [wgrant, abentley] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: the feature looks good. I stumbled onto a corner case that probably should have a bug submitted for it. If the bug tracker is set as an email address, it correctly displays a checkmark for it, but the "Report a bug" link is still grayed out with a tooltip that sounds like it is unconfigured.
<EdwinGrubbs> wgrant: is your per-archive-build-debug-symbols branch still waiting on more reviews, or did someone just forget to mark the entire mp as approved?
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: - || queue: [wgrant] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs: thanks for finding that corner case.  it'll be fixed when i redo the 'official_*' properties
<bac> EdwinGrubbs: and thanks for the review
* jtv changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> EdwinGrubbs: I just did wgrant's
<jtv> ...and am bowing out.
* jtv changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mwhudson> gary_poster: ping
<mwhudson> gary_poster: did you see https://code.edge.launchpad.net/~mwhudson/launchpad/test_traverse-set-participation-bug-611570/+merge/31731 ?
<mwhudson> gary_poster: sorry for only bugging you with these deep zope-type branches :-)
<gary_poster> mwhudson: did you see I approved it 26 minutes ago ;-)
<mwhudson> gary_poster: heh, no
<mwhudson> i did check after i got up though...
<mwhudson> gary_poster: thanks
<gary_poster> welcome, mwhudson, thanks for very nice change
<mwhudson> gary_poster: the thing about browser:page that grated with me is that it makes zcml kinda unavoidable
<mwhudson> well, not unavoidable, i proved that
<gary_poster> heh
<mwhudson> but unavoidable for sane people
<gary_poster> so the class stuff I already mentioned.  you also meant the security stuff?
<mwhudson> well
<mwhudson> basically i think the metaconfigure functions should all be fairly short
<gary_poster> oh I think I see
<mwhudson> gary_poster: could i have taken the zope.publisher.browser.BrowserPage approach in my helper?
<gary_poster> if metaconfigure functions are short, then presumably they are calling out to python that is already factored out for reuse, and so then zcml (or, more generaically, configuration) is les necessary
<gary_poster> yes, though it would have been annoying in two ways:
<mwhudson> gary_poster: exactly
<gary_poster> 1) you would have had to pass the class in to your helper, rather than a function
<gary_poster> 2) you still would have had to do the security dance
<mwhudson> gary_poster: well, it's not really making configuration less necessary per se, but more equally accessible to code and zcml
<mwhudson> gary_poster: oh ok
<gary_poster> (you could have done the security dance once actually, with a trick, but I thought what you did was better than the mysterious trick of using a __Security_checker__ on the class, IIRC)
<gary_poster> more equally accessible: ack.
<mwhudson> gary_poster: heh, not heard of __security_checker__
<mwhudson> anyway, it works as is now, so i don't really want to touch it again :-)
<gary_poster> +1 :-)
<mwhudson> bah
#launchpad-reviews 2010-08-05
<lifeless> EdwinGrubbs: https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/31805
<lifeless> (if you're still oncall)
<mwhudson> lifeless: looking
<mwhudson> lifeless: you've seen TestCase.assertStatementCount ?
<mwhudson> it's sort of a bit like but not quite like QueryCounter
<lifeless> I looked for it
<lifeless> but found QueryCounter
<lifeless> oh yay two totally different ways of doing the same thing.
<lifeless> uhm
<lifeless> sigh, thanks.
<lifeless> I think my refactoring is not harmful
<lifeless> in fact, I think QueryCounter is useful because its aimed directly at the request, not at the storm layer
<lifeless> so its useful for 'is this page request going to be ok'
<lifeless> which, record_statements on allmembers is not a good answers for
<mwhudson> ah, i just commented on the mp
<mwhudson> lifeless: looking at the implementation, the two things are actually doing very similar things
<mwhudson> also this makes me wince:
<mwhudson> def clear_request_started():
<mwhudson>     """Clear the request timer.  This function should be called when
<mwhudson>     the request completes.
<lifeless> where is that ?
<mwhudson> canonical.launchpad.webapp.adapter
<lifeless> oh heh
<lifeless> nasty file that
<mwhudson> yes
<mwhudson> lifeless: anyway, perfectly happy to approve branch as is, just wanted you to know about the other way
<mwhudson> lifeless: approved
<lifeless> mwhudson: thanks!
<lifeless> mwhudson: I think its ok to have two semanticly different things
<lifeless> mwhudson: because I can imagine as we get more event driveny that they will be rather different
<mwhudson> lifeless: i guess i'd be happier if get_request_statements did something closer to what its name suggested, it's not reaaaaaaally tied to the request at the moment
<mwhudson> it's just 'queries issued in this thread between beginPublication and endPublication' or whatever
<mwhudson> no getting around that with an implicit database connection i guess
<lifeless> right
<lifeless> so I can see that getting fixed
<lifeless> e.g. if we get more twisted into the appservers (long poll!)
<lifeless> or whatever
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> jtv: Hi.
<jtv> hi wgrant!
<StevenK> wgrant: Stalker!
<wgrant> jtv: Thanks for the review yesterday.
<wgrant> jtv: But you approved but didn't approve it.
<jtv> wgrant: ah yes, sorryâ¦ that's something we often do for ourselves.
<wgrant> jtv: Yeah, I can't do that :)
<jtv> I sort of got in the habit of letting the author set the status for themselves after they process any review suggestions I give them, but since I didn't do that either...
<wgrant> Heh.
<jtv> StevenK: if you say william's stalking me, note I'd been online for several hours.  Wouldn't be very efficient stalking.
<jtv> wgrant: there, done.  Now I suppose you'll want me to land it as well?
<wgrant> jtv: I want to run it past bigjools again this evening.
<wgrant> So, please don't.
<wgrant> Just in case, you know.
<jtv> wgrant: soâ¦ just the sort of thing you want to defer the Approved status for?
<wgrant> jtv: hmmm.
<lifeless> so, approved should mean
<lifeless> 'the reviewer does not need to see it again'
<jtv> say mwhudson, would you be free for an oversized review?  Couldn't get anyone yesterday and it's starting to hold me up.
<lifeless> which is != 'it is ready to land'
<jtv> lifeless: an approved _vote_ means the reviewer doesn't have to see it again.  But multiple people are involved in an MP.
<lifeless> jtv: that too is true
<jtv> So the Approved status comes about when everyone who's planning to say anything about the branch, including the author, feels it's ready to ladn.
<jtv> land.
<lifeless> jtv: that does not match the model the code team have.
<jtv> what do they do?
<lifeless> the approved status is /intended/ to mean 'the branch is approved from the point of view of the approval policy for the project, whatever that may be'
<jtv> lifeless: I should amend what I said to: anyone who's _supposed_ to say anything about the branch.
<lifeless> the 'queued' status is used for 'land it now' - but is disabled in the web UI for a few reasons.
<jtv> lifeless: I didn't even know that state existed.  What's wrong with defining approval policy the way I said?
<lifeless> jtv: it implies a blocking transition where one may not be needed
<jtv> May not.  If that needs to be established, you've got a blocking transition.
<lifeless> so, on a per project, per review basis - sure
<lifeless> you can say 'this proposal is not ready to approve' and approve it when it is ready.
<mwhudson> jtv: i should probably do some linaro stuff really, is it large and complicated or just large?
<jtv> mwhudson: ah yes, I forgot about linaro.  It's reasonably complicated, so I'll find some other suc^H^H^Hhelpful soul
<lifeless> I was told when I did a bunch of reviews that the LP policy is 'approved status + needs-fixing vote' to say 'fix this up and then land it', and so on.
<jtv> lifeless: I wasn't aware of thatâI thought "needs fixing" meant "fix it and then come back to me."  Seems to match the privileges better: only the reviewer can change that vote but the engineer can usually change the status.
<jtv> thumper, are you someone I can bother for an oversized review at the moment?
<thumper> jtv: no
 * jtv puts up the it's-alright-I-understand act
<thumper> lifeless: if I say fix it and land, I vote approve with 'conditional' or something
<jtv> Same here.
<thumper> lifeless: to me needs-fixing means get back to the reviewer
<jtv> lifeless: you know I hate to lean on you so heavily but is this oversized review something you could do?
<jtv> It's this one: https://code.edge.launchpad.net/~jtv/launchpad/recife-traits/+merge/31729
<lifeless> jtv: I'm playing catchup myself today :(
<lifeless> jtv: is it mechanical
<jtv> Alas, no
<lifeless> jtv: or does it require thought?
<jtv> Alas, yes
<jtv> so that leavesâ¦ jml?
<lifeless> jml, stub, danilo, all of eastern europe
<jtv> I don't see any of those right now
<lifeless> right, I'm talking candidates-while-you'l;-be-awake :)
* jelmer changed the topic of #launchpad-reviews to: is On call: jelmer || reviewing: stevek ||  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> jelmer: jtv has a big patch he's begging for a review of
<jelmer> k
<lifeless> jtv: ^
<jtv> lifeless: thanks otp
<wgrant> jelmer: Could you review https://code.edge.launchpad.net/~wgrant/launchpad/destroy-publishedpackage/+merge/31813, when you have a moment?
<jelmer> wgrant: yes, of course
<lifeless> wgrant: I reviewed it from a code angle fwiw
* jelmer changed the topic of #launchpad-reviews to:  On call: jelmer || reviewing: stevek || queue: [jtv, wgrant]  ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> lifeless,wgrant: what other angle does it need review from?
<lifeless> none AFAIK
<wgrant> lifeless, jelmer: Ah, sorry, I'd assumed that was a DB review.
* wgrant changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: stevek || queue: [jtv]  ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> lifeless: Can you land that, then, please?
<lifeless> wgrant: e-rather-late; jelmer can :)
<jelmer> sure, no problemo
<wgrant> Thanks.
<jtv> jelmer: thanks for putting me on the queue.  Overweight branch, with apologies.
<jelmer> StevenK: You mention in your MP that IDistroSeries.initialiseFromParent() has nothing to do with DistroSeries in general, which puzzles me a bit.
<jelmer> it seems like a reasonable thing to have on IDistroSeries, if only as a convenience method
<StevenK> jelmer: It looked to bigjools and me as completly seperate ...
<lifeless> FWIW
<lifeless> it looks to me like its conceptually a class method
<lifeless> as in
<lifeless> it creates a new type X from another type X
<lifeless> thats kindof like __new__
<lifeless> if you squint
<jelmer> lifeless: Yep, though it's called on a freshly created DistroSeries, it doesn't create one itself.
<bigjools> i-f-p is a soyuz function that acts on a registry object
<bigjools> it's *nothing* to do with Registry and should not live on IDistroSeries
<lifeless> so IDistroSeries has nothing to do with registry ?
<jelmer> lifeless: IDistroSeries is owned by registry, i-f-p is sozuz
<lifeless> jelmer: I *still* don't get why
<lifeless> but its late, and I'm this || far away from trolling by mistake
<lifeless> so I'm going to stop work :)
<bigjools> lifeless: you're twisting what I said
<jelmer> bigjools, StevenK: Thanks. I still think it makes sense conceptually to have i-f-p or a convenience method that calls out to i-f-p on IDistroSeries, but I can understand why it's not there.
<bigjools> i-f-p initialises a distroseries in a soyuz context
<bigjools> our model objects are too big already
<lifeless> bigjools: see above, really, epically tired and there is stuff I need to learn here but I won't after being up for 17 hours
<bigjools> lifeless: !
<bigjools> go to bed
<lifeless> I *don't* want to troll by accident
<lifeless> When I troll, I want to mean it :)
<bigjools> go to your bed under that bridge over there
<jelmer> StevenK: r=me
* henninge changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: stevek || queue: []  ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jelmer: I took jtv's branch
<jelmer> henninge: ah, great - thanks
* jelmer changed the topic of #launchpad-reviews to:  On call: jelmer || reviewing: -  || queue: []  ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * jelmer lunches
<jtv> henninge: any luck with that review?
<jtv> (I'm pretty much EOD at this point)
<jtv> So if you're stuck on anything, do say so soon!
<henninge> jtv: sorry, still working on it.
<jtv> No worries.  I'll eod then.
<henninge> jtv: what's the purpose of MessageSideHelper?
<henninge> jtv: couldn't those methods be simply on Translationmessage?
<jtv> henninge: it's a bit legacy: it's where I extracted the Traits from.  One thing it does though is provide a place where the incumbent messages can be cached.
<jtv> (Can't do that in the TM model class of course)
<henninge> jtv: sorry, POTMsgSet, I guess.
<jtv> Can't do it in any model class.
<jtv> I'd like to avoid relying automatically on a method that queries for incumbent messages.
<jtv> At the moment we do check that in makeCurrentUpstream and makeCurrentUbuntu; I'd prefer to have that check a bit higher up somewhere, or at least let the traits go to a lower-level setter.
<jtv> That also means we won't need the duplication between those two methodsâit'd be a single traits-based implementation.
<henninge> jtv: we will have to continue that tomorrow morning. I still have to switch locations for the rest of the day.
<jtv> henninge: and me for the night.  :)  Bis dann!
<henninge> TschÃ¼ss
<james_w> jelmer: https://code.edge.launchpad.net/~james-w/launchpad/stop-publishing-nagging/+merge/31851 please
<jelmer> james_w, yessir
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: james_w  || queue: []  ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<james_w> jelmer: oh, second revision pushed to remove the import too
<jelmer> k
<EdwinGrubbs> jelmer: can you review this branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page/+merge/31856
<jelmer> EdwinGrubbs: yes, sure.
<jelmer> EdwinGrubbs: great to see a fix for that bug.
<jelmer> EdwinGrubbs: There seems to be a (unnecessary?) conflict in that branch
<jelmer> EdwinGrubbs: lib/lp/testing/factory.py
<EdwinGrubbs> jelmer: I fixed the conflict and pushed it up.
<jelmer> EdwinGrubbs: Thanks
<jelmer> EdwinGrubbs, lib/lp/registry/model/sourcepackage.py has a print statement in it, is that intentional?
<EdwinGrubbs> jelmer: no, that was a mistake, the changes I just pushed should have removed that.
* jelmer 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
<jelmer> EdwinGrubbs: I'm off soon, once but I'll finish the review of your branch.
<EdwinGrubbs> ok
<jelmer> EdwinGrubbs: I've just followed up
<EdwinGrubbs> thanks
<abentley> rockstar, I can has rubberstamp? https://code.edge.launchpad.net/~bzr-loom-devs/bzr-loom/trunk/+merge/31873
<rockstar> abentley, rs=me
<abentley> rockstar, I can has rubberstamp? https://code.edge.launchpad.net/~bzr-hg/bzr-hg/trunk/+merge/31876
<rockstar> abentley, rs=me
<abentley> rockstar, how long does a branch upgrade usually take?
<rockstar> abentley, depends on the size of the branch.
<rockstar> abentley, in most cases, a few minutes maximum.  In other cases, hours.
<abentley> rockstar, this branch is taking longer than I would expect: https://code.edge.launchpad.net/~launchpad-pqm/bzr-loom/trunk
<abentley> rockstar, takes ~2 seconds locally.
<rockstar> abentley, it might have other branch upgrade jobs in front of it.
<rockstar> abentley, all it's really doing is copying the tree over a local file transport, and upgrading the branch.  I can't imagine it would be that slow.
<abentley> rockstar, it's done now, but it did take 10 minutes or so.
<rockstar> abentley, yeah, we need some better logging.  There's an open bug about that.
<rockstar> abentley, maybe it's small enough that I can get to it sometime today.
<james_w> if anyone is up for a review https://code.edge.launchpad.net/~james-w/launchpad/soyuz-factory-improvements/+merge/31890
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/no-more-sampledata-2/+merge/31891 is a follow-on from that one
<rockstar> thumper, i can has revyooz?  https://code.edge.launchpad.net/~rockstar/launchpad/recipe-build-table/+merge/31882
 * mwhudson also has a review request, for a bug that's breaking staging and will break edge: https://code.edge.launchpad.net/~mwhudson/launchpad/disable-vostok-when-not-configured/+merge/31901
<rockstar> mwhudson, we just talked about this on our standup.
<rockstar> mwhudson, r=me
<mwhudson> rockstar: thanks
#launchpad-reviews 2010-08-06
<mwhudson> thumper: can you look at https://code.edge.launchpad.net/~mwhudson/launchpad/vostok-traverse-distro/+merge/31241 again?
<thumper> ok
<thumper> mwhudson: do you have a few minutes for me to bounce some stuff off you?
<thumper> skype like?
<mwhudson> thumper: ok
<mwhudson> thumper: i'm online
<thumper> mwhudson: I'll have to defer for now, other people calling
<mwhudson> thumper: ok
<mwhudson> thumper: the call or the review or both?
<thumper> the call
<mwhudson> k
<mwhudson> thumper: NotFound is a zope thing
<mwhudson> it's not in lp.app.errors
<mwhudson> so i'm going to ignore that comment?
<thumper> um...
<mwhudson> thumper: NotFound != NotFoundError
<thumper> ah
<thumper> yeah
<thumper> I just worked that out
<mwhudson> for better or worse
<thumper> ok
<mwhudson> something inside traversal converts the latter into the former
<mwhudson> cool
<henninge> jtv: Why do you want to cache the incumbent message in the traits helper?
 * henninge is back to business ;)
<lifeless> hi henninge
<jtv> henninge: good!  :)
<henninge> Hi lifeless! ;)
<henninge> jtv: I am starting to see what Danilo meant by "heavy weight".
<jtv> henninge: those are the helpers, and as I said, those are a bit legacy in that they may not be needed any more.
<jtv> That's not the traits; it's what I extracted the traits out of.
<henninge> jtv: I understand but I would have expected that to go into the class that uses the traits.
<jtv> henninge: can't do that in the class that uses the traits.
<henninge> do what, the caching?
<jtv> The class that uses the traits can be a model classânot good to cache this sort of thing there.
<jtv> We can do it in the code.
<henninge>  ok
<jtv> But as I said, this is just what's left after extracting the traitsâthe caching is the possible reason not to remove the helpers entirely once we have the traits.
<lifeless> do you guys know much about the API export stuff?
<jtv> And I'm not saying we need the caching, just that it's an appropriate and convenient place for it if we do.
<jtv> lifeless: not that much
<henninge> I see
<jtv> henninge: cleaning up the helpers is a possible next step, but the branch was big enough.  :)
<henninge> Okay, so other users of the traits are not expected create helper classes, too?
<jtv> henninge: no, not at all
<jtv> As I explained yesterday: one class does it all, and the existence of an older helper class that I extracted them out of is merely a distraction in all this.
<jtv> Look at the actual TranslationSideTraits class: is that heavyweight?
<henninge> jtv: no, except ...
<henninge> ;)
<henninge> getCurrentMessage
<jtv> The implementation is a bit daft because of the transient mess with getImportedTranslationMessage and getCurrentTranslationMessage.  I'd prefer to replace the "if" with separate methods.
<jtv> But that's all under the hood.
<henninge> jtv: which "if" ?
<jtv> henninge: inside getCurrentMessageâ¦ that's not what you meant?
<henninge> sorry, yes.
<henninge> jtv: Although I was wondering if a specific method like that belongs into the traits at all ...
<jtv> If we had all queries stormified and available from the interfaces, then no.  But in the real world it's a primitive.
<henninge> jtv: what do you bean by "available from the interfaces"?
<jtv> The interfaces underneath wherever we need this.
<henninge> Sorry for being pita about this but I want to make sure I understand why I am on your side with this ... ;-)
<jtv> No worries.  It's good.
<jtv> What I mean is, sometimes we'll need to ask for the current message for a given potmsgset, template & language on a given side.
<jtv> If we could just return a Storm object for the right condition, then getFlag(TranslationMessage) would probably do the trick.
<jtv> Of course we could also do that and go straight to Storm to ask for the current message, but that seems a bit messy.
<henninge> jtv: You lost me on the last sentence
<henninge> And you literally mean "getFlag(TranslationMessage)"?
<henninge> I mean, on the class.
<jtv> I mean: we could probably do without getCurrentMessage if we were willing to do something like Store.of(TranslationMessage).find(traits.getFlag(TranslationMessage)).one()
<jtv> Yes
<henninge> jtv: is there another class that will have the flags?
<jtv> Yes, there might be a ClassAlias in Storm.
<henninge> I'd like to see Store.of(TranslationMessage).find(traits.current_flag).one()
<henninge> Oh
<jtv> Well what you just said is actually a very good way to formulate it.
<lifeless> as long as its not one per-row-per-page :)
<jtv> *If* we only have the one class, of course
<henninge> jtv: I'd provide both.
<henninge> a "current_flag" attribute for most use cases
<henninge> and a "getCurrentFlag(cls)" for the alias case.
<jtv> lifeless: maybe you know this one...  I just evaluated TranslationMessage.is_current_ubuntu and ubuntu_traits.getFlag(TranslationMessage).  They both return the same PropertyColumn object.  Yet a Storm query will work with the former and not with the latter.
<henninge> oh, it does not?
<lifeless> so the former is a class attribute
<jtv> Yes
<lifeless> is the latter perhaps returned a securityproxied object?
<jtv> I think the __repr__ would have told meâ¦ in both cases it says:
<jtv> <storm.properties.PropertyColumn object at 0xbcc7b3c>
<jtv> But there must be _something_ with proxies, because the error says:
<jtv> CompileError: Don't know how to compile type zope.security._proxy._Proxy of <storm.expr.Eq object at 0xc332fac>
<lifeless> ahha yes
<lifeless> :)
<lifeless> print dir(thing), and thing.__class__
<jtv> Heh... if I removeSecurityProxy then the query does work.  Madness reigns.
<henninge> jtv: no, it does not
<jtv> lifeless: the __class__ are the same for both
<lifeless> thats interesting
<henninge> it's because you regeistered TranslationSideTraitsSet as a securedutility.
<henninge> "does not" referred to madness reigning, btw.
<jtv> henninge: okay, but then why don't I see the proxy?
<jtv> Right, took me a while to figure that out :)
<henninge> jtv: that's waht proxies do. They pretend to be the real thing.
<jtv> Okay, but this is an incredibly effective pretense!
<jtv> At least, I had a memory of the proxies showing up when I printed objects in "make harness"
<jtv> But yes, dir() exposes the difference.
<henninge> jtv: another thing I wonder about it "other_side"
<jtv> henninge: would you prefer something like "other_side_traits"?
<jtv> Or would you prefer it to be a TranslationSide that you'd use to look up the traits?
<henninge> jtv: no, I don't wonder. ;)
<henninge> anymore
<jtv> oh ok :)
<henninge> jtv: but "other_side_traits" seems preferable.
<jtv> OK
<henninge> My wondering was about it not being set but I saw that it happens in TranslationSideTraitsSet.__init__.
<henninge> That's good.
<jtv> henninge: I renamed other_side and also removed MessageSideHelper.getFlag (because that code moved to the traits in its entirety)
<henninge> jtv: all the calls to "removeSecurityProxy" in your test - are they a result of abel's work?
<jtv> henninge: it's not unthinkable that they contributed, but the main thing is as in the cover letter: using Zope in tests that didn't use it before.
<jtv> Sorry: it's not unthinkable that *it* contributed.
<henninge> 653	         external_template = self.factory.makePOTemplate()
<henninge> 654	-        external_template.productseries.product.official_rosetta = True
<henninge> 655	+        product = external_template.productseries.product
<henninge> 656	+        removeSecurityProxy(product).official_rosetta = True
<henninge> jtv: how is zope involved here that was no before?
<henninge> And I wonder why "official_rosetta" is not settable in the first place ...
<jtv> henninge: layer change
<henninge> Oh, I did not know that affected how the factory creates objects.
<jtv> AIUI it doesn't, directly, but it means no proxies.
<jtv> Correction again: running Zopelessly means no proxies.
* 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 changed the topic of #launchpad-reviews to:   On call: adeuring || reviewing: james_w || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: why is "test_getFlag_and_setFlag" one test?
<jtv> henninge: because it tests consistency between the two, more than either one of them specifically.  Not very imaginative, sorry.
<henninge> jtv: ok, I see it now.
<jtv> (There are two separate tests in the concrete test classes that tie the internal consistency of the traits to the reality of the two flags)
 * jtv tries DSL againâ¦
<jtv> Well that was disappointing.  Still no DSL.  Wonder if a bill got lost again.
<stub> https://code.edge.launchpad.net/~stub/launchpad/cronscripts/+merge/31934
<stub> adeuring: ^^
* stub changed the topic of #launchpad-reviews to:   On call: adeuring || reviewing: james_w || queue: [stub] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> stub: sure; let me just finish a review for james_w
* adeuring1 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: thanks!
<henninge> jtv: welcome
<henninge> you were gone to check DSL when I wanted to tell you. Is it working again now??
<jtv> henninge: nope
<adeuring> stub: r=me
<stub> ta
<jtv> henninge: can't lend using edgeâ¦  I'll try again.
<jml> james_w, I'll take a look at your no-more-sampledata-2 branch.
<jml> (next week, I'm going to do less coding stuff, promise)
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: stub || 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, bac || reviewing: stub, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> morning adeuring
<adeuring> hi bac!
<bac> adeuring: many takers today?
<adeuring> bac: a few. Could be worse ;)
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: james_w, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jml: can you do a db review if i don't get it done before stub goes EOD?
<james_w> thanks adeuring and jml
<jml> bac, sure.
<jml> james_w, done!
<james_w> jml: did you get bored? :-)
<jml> james_w, not so much bored as struck with a desire to come up with many ways of saying the same thing :)
<adeuring> james_w: r=me for another branch, again one minor nitpick ;)
<james_w> adeuring: thanks, though the last one wasn't so minor :-)
<james_w> adeuring: I thought the same thing about isinstance, but the test proved me wrong. Maybe something needs to be imported for the monkeypatch to happen?
<adeuring> james_w: interesting that you need to remove the proxy: IIRC zope.security.porxy or somesuch directly changes the built in isinstance function.
<adeuring> But anyway, if you must remove the proxy, then be it so. But perhaps gary_poster knows a bit more?
<gary_poster> no, isinstance is not modified
<james_w> jml said that you can "from zope.security.proxy import isinstance as zope_isinstance"
<gary_poster> it's not called that
<jml> gary_poster, we have code in Launchpad that does exactly that.
<gary_poster> at least I don't think so (and I use the one from zope.proxy)
<adeuring> gary_poster: thanks -- seems that I am somewhat confused...
<gary_poster> but that's the right idea
<gary_poster> ok, jml, fair enough.  that's certainly the right idea, as I said.  zope.proxy has the more general functions with which I am more familiar
<jml> gary_poster, more general functions sounds like a good idea :)
<gary_poster> :-) well, looks like my memory is just faulty.  The general functions are there that work with the core proxy stuff, but there's not a direct isinstance equivalent.  (Available functions are listed in eggs/zope.proxy-3.5.0-py2.6-linux-x86_64.egg/zope/proxy/interfaces.py fwiw)
<gary_poster> a function called "zope_isinstance" offends my naming sensibilities ;-)
<adeuring> so, importing from zope.security.proxy would be the roght way?
<gary_poster> yes, adeuring
<gary_poster> sorry for being confusing
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<james_w> adeuring: hello again, could I get an incremental review of https://code.edge.launchpad.net/~james-w/launchpad/soyuz-factory-improvements/+merge/31890 ?
<adeuring> james_w: sure
<james_w> oh, conflicts, I need to merge devel
<james_w> done
 * adeuring pulls the latest version
<bac> jml: can you do a db review for https://code.edge.launchpad.net/~bac/launchpad/db-bug-613442/+merge/31955 ?
<jml> bac, sure, after this call.
<bac> thx
<adeuring> james_w: again r=me. nice work!
<james_w> adeuring: thanks
<jml> bac, approved.
<bac> thanks jml
<bac> adeuring: may i get a code review for https://code.edge.launchpad.net/~bac/launchpad/db-bug-613442/+merge/31955 ?
<adeuring> bac: sure
<bac> thanks
<adeuring> bac: why didn't you add a column for bug tracking?
<adeuring> (and for code hosting, for projects)
<bac> adeuring: bug tracking and code hosting can be properly derived from other pillar data.
<bac> adeuring: it is better to have a derived attribute than a column in the db that may become out of sync
<adeuring> bac: what about "bugs are tracked elsewhere"?
<bac> adeuring: that would be indicated by having an external bug tracker noted
<bac> adeuring: this email thread lays out much of the thinking, dense as it is: https://lists.launchpad.net/launchpad-dev/msg03952.html
<adeuring> bac: Ah, right. And these is at present no need to indicate this in the comuted attribute?
<bac> adeuring: i don't understand your question.
<adeuring> sorry, my usual typing problems...
<adeuring> bac: shouldn't the value "bugs are tracked elsewhere" being returned by bug_tracking_usage? (for porjects which do that)
<adeuring> gah, that's done
<adeuring> sorry, I was reading the diff for distros...
 * adeuring should read the entire diff before asking pointless questions...
<adeuring> bac: r=me
<bac> thanks abel
<EdwinGrubbs> jelmer_: do you have time to answer some of my questions on the branch you're reviewing?
<jelmer_> EdwinGrubbs, sure
<jelmer_> EdwinGrubbs: I don't see a followup from you after my last reply
<EdwinGrubbs> jelmer_: oh, sorry, I didn't see your reply before.
* adeuring 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> jml: you approved the db review of my branch.  what should i do about patch number?  next sequential?  how do i know there is no conflict?
<bac> jml: i assume stub has a Big Chief Tablet where he coordinates the numbering
<jml> bac, I don't have answers to any of your questions, I'm afraid :(
<bac> jml: well i'll pick the next and cross my fingers.  (after looking at the unlanded MPs first)
* sinzui changed the topic of #launchpad-reviews to:  On call: bac || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> bac, that seems like the best approach
<sinzui> bac: I have a branch that move the registration slot.
<bac> sinzui: ok
<bac> sinzui: are you going to get a UI review too?
<sinzui> I was hoping rockstar could look at the picture and say this is agreeable to him, me, and henninge
 * rockstar looks up
<rockstar> sinzui, what picture?
<sinzui> rockstar, the picture listed in https://code.launchpad.net/~sinzui/launchpad/launchpad-header-1/+merge/31975
<rockstar> sinzui, ui=rockstar
<rockstar> sinzui, I still hate that we feel that should go at the top of the page.
<sinzui> yes. Maybe I will take a week off and write an alt-css that allows launchpad to get that non-sense out of the way
<benji> speaking of, is there any way to remove the "Registered by" slot for a project?  Every time I visit the page for my little project Manuel I'm slightly jarred by the fact that it says "Registered 2010-01-13 by BjÃ¶rn Tillenius"
<benji> (he registered it to work on some branches for Manuel before I used LP)
<bac> sinzui: r=bac
<sinzui> thanks bac. thanks rockstar
<bac> benji: no.  why do you find it objectionable?  it is a true statement.
<rockstar> benji, no, and that's why I think it should just go away.  It can be misleading.
<rockstar> bac, it's a true statement, yes.  But it's almost worthless to put right at the top.
<benji> bac: it's strange (or misleading as rockstar says); it's odd to place such an unimportant bit of information in such a prominant place
 * benji assigns rockstar as his spokesman on this topic.
<rockstar> bac, it has some useful information sometimes, but it's not information that should go at the top.
<benji> I can only assume that it was done that way initially to reward people for registering projects in LP, but I think we're beyond that now.
<sinzui> I look at who registered a project, bug, question, and mirror every day. It is not important, but it is information users actually want to know
<rockstar> In most cases, the registrant is the same as the owner.  In other cases, it's just misleading.  In a few cases, you actually need to know it.  Those people can go hunting on the page for it.
<sinzui> Not that you can so anything about someone who reports a bug that launchpad causes flatulence in donkeys
<benji> bug, question, and mirror make sense to me, but it's not really that interesting to know that I registered the Firefox project if I don't really have anything to do with the project itself
 * benji goes looking for high-profile projects that aren't yet represented in LP in a "Registered by" landgrab.
<sinzui> If you are looking for missing upstream information, the registrant is one of the people you want to contact
<sinzui> Most projects are missing information about upstream services
<benji> oh, don't get me wrong, I think the information should be recorded and visible, but I don't see why regiterer should be more prominent than homepage, owner, driver, etc.
<bac> rockstar: i updated to PG8.4 so i could cleanly make new sample data.  by the time i got ready to do it, there was a new version...made with 8.3.  i give up
<rockstar> bac, you should blame on that and see who did that.
<bac> is it me, or is PG8.4 much slower for 'make schema'?
<sinzui> benji, I think  we all agree. The heading was design by canonical's design team to address the need of projects and people to see their identity prominently as a means to encourage adoption
 * sinzui did not ask for permission to change the header
<sinzui> benji, I disabled about 5000 projects that were not appropriate. Most land grabs last a few hours
<jelmer_> sinzui: wow, 5000? That's quite a few.
<sinzui> I have these numbers because I started writing a summary of project review. What you need to know is that I temporarily exceeded your record for linking projects to packages
<jelmer_> sinzui: ah, thanks
<deryck> Hi, bac.  I have an easy one-line change to versions.cfg for your review, if I may get in line.
<bac> sure
* deryck changed the topic of #launchpad-reviews to:  On call: bac || reviewing: - || queue: [sinzui, deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to:  On call: bac || reviewing: deryck || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> thanks, bac!  The diff is large because I'm also re-enabling some reverted work.  All that is new is the change to versions.cfg in the diff.
<deryck> https://code.edge.launchpad.net/~deryck/launchpad/update-storm-17-add-back-dupe-changes/+merge/31983
<bac> sinzui: your name in the queue was leftover, right.  you have nothing outstanding for review?
<sinzui> right, but I will add it back in 30 minutes
<bac> deryck:  one line or 1296 lines?
<bac> deryck: which one of those lines do you want reviewed?
<bac> should i just pick one?
<bac> i approve of line 157.  it is poetry.  r=bac
<benji> lol
<deryck> bac, the change at line 1293 of the diff is all that is new.  I'm just being lazy and adding back previous work that was reverted as I update the version of Storm.
<deryck> heh
<deryck> bac, I noted the actual line in scrollback if you really didn't see.  Line 1293. :-)
<bac> deryck: wow.  ok, it looks fine given that explanation.
<deryck> bac, great, thanks.
* sinzui changed the topic of #launchpad-reviews to:  On call: bac || reviewing: deryck || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> bac: do you have time for a cache fix: https://code.edge.launchpad.net/~sinzui/launchpad/registry-cache-4/+merge/31998
<bac> sinzui: sure
<sinzui> bac I see a bad comment. in the second test. I wait for you challenge to make it a sensible comment
<sinzui> bugger, I lost my bug id to
<bac> sinzui: so line 51 should be miss.  is that what you're referring to?
<sinzui> yes
<bac> with that change it looks fine to me
<bac> r=festus
<sinzui> thanls
<bac> any gunsmoke fans in the house?
<sinzui> thanks bac
<benji> bac: I would have gotten the reference if you'd picked any other popular US series from the 70s; not into Gunsmoke
 * benji has a sudden desire to watch The Rockford Files.
<bac> benji: "Who was Micah" for 20 points?
<benji> I think I need to phone a friend
<bac> no "Gunsmoke" or "The Rifleman"?
<bac> Hoss Cartwright?
<bac> Joe Friday?
<benji> not really into westerns
<benji> I'm down with Joe Friday; my son would vote for Reed and Malloy (Adam 12)
<bac> for some reason i preferred "Emergency!"
<benji> I tend to favor comedies from the 70s, most of the "action" series are pretty slow and linear by today's standards.
* bac changed the topic of #launchpad-reviews to:  On call: bac || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bryceh> benji, my wife is a Rockford Files fanatic
<bryceh> we found out that netflix has the full run of Rockford in their online instant view thingee, so she can watch it whenever she wants.
