#launchpad-reviews 2010-07-05
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi henninge, would you be able to review an MP of Jelmer's for us pls? https://code.edge.launchpad.net/~jelmer/launchpad/archive-arch-fix/+merge/29160
 * henninge looks and considers ... 
<henninge> :-P
<noodles775> :)
<henninge> noodles775: yeah, ok ;)
<noodles775> Great, thanks!
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: jelmer || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: so, do I talk to you about the mp or shall I just reply by mail to jelmer so that he can process it?
<noodles775> henninge: Best to do it on the MP if that's ok.
<henninge> yes, find
<henninge> e
<henninge> dine
<henninge> fine
<henninge> ;)
<noodles775> lol
* henninge_ changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: reviewed but not yet approved.
<noodles775> henninge: thanks for that.
<bigjools> henninge: I'll take over
<bigjools> wasup?
<henninge> bigjools: I replied in the MP
<bigjools> henninge: what's the URL for that please?
<henninge> https://code.edge.launchpad.net/~jelmer/launchpad/archive-arch-fix/+merge/29160
<bigjools> henninge: what are you referring to with "I see more instances of that" ?
<bigjools> oh the dodgy docstrings?
<henninge> right
<bigjools> henninge: replied
<henninge> cheers
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lunch || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lunch || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> henninge: you really want another MP?
<henninge> bigjools: no, I just thought you'll need it to request the r-c, don't you?
<henninge> cp, not r-c
<bigjools> henninge: I can do that on the existing one
<bigjools> it's for r-c
<bigjools> so it goes in this release
<henninge> oh, right, we release
<henninge> ;)
<bigjools> :)
<henninge>  bigjools: as you like it ;)
<bigjools> that restricted architecture stuff that was done as a while ago was a total shambles
<bigjools> this is not the first fix to what you'd think would be an obvious bug
<henninge> uh-oh
<henninge> I am still -1 on the (actual, expected) thing, btw.
<bigjools> it reads so much better to me that way :/
<bigjools> then it's more like how we write code with a '==' operator
<mwhudson> you could always use assertThat(value, Equals(expected)) :-)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adiroiban> jtv, henninge hi, I have updated the branch for POFile statistics, to export them as a dedicated object https://code.edge.launchpad.net/~adiroiban/launchpad/bug-583934/+merge/26965
<jtv> adiroiban: thanks, I'll take a look right now
<adiroiban> i had to create a canonical_url for the new object
<adiroiban> not sure if we want that
<adiroiban> or we want to create StatisticsObject for each statistics type
<adiroiban> or a single StatisticsObject that will handle statistics for POFile, DistroSeriesLangauges, SourcePackages
 * StevenK pouts at adiroiban jumping the queue :-P
<adiroiban> StevenK: sorry for that. This is a pre-imp chat , not a real MP
<StevenK> adiroiban: It's all good :-)
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || reviewing: -,- || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> adiroiban: what you did looks right to me (though I'm not a zope expert) though I wouldn't special-case the "statistics" string in the navigation class.  Wouldn't +statistics (or +stats) be a cleaner way to solve this?
<adiroiban> jtv: afaik, +somethins is for describing a view
<adiroiban> not an object
<jtv> adiroiban: frankly I've always wondered about that.  :)
<adiroiban> also, I don't know how to avoid the special casing on the URL traversal
<jtv> True, you'll definitely have to have _some_ special-casing if we don't want to rearrange our URLs
<adiroiban> jtv: what I am not sure that I did right, is the exposure of TranslationStatistics objects only in the API and not in the browser view
<adiroiban> I am not sure if this is ok
<adiroiban> or the rule is to have a direct match between API and the browser views
<jtv> adiroiban: I don't know of any such rule... maybe someday we'll find it useful to attach a small view to this but that's for later I think.
<adiroiban> ok
<adiroiban> then I will go on and polish the code
<StevenK> henninge, abentley: Which one of you wants to look at my MP? :-)
<henninge> StevenK: already lookin
<henninge> g
* henninge changed the topic of #launchpad-reviews to: On call: henninge, abentley || reviewing: StevenK,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adiroiban> jtv: there is one more think I would like to ask. in model.POFile.translation_statistics , it creates a new statistics object on each call. Would it not be better to create a single statistics object in POFile.__init__, and retrieve it as a singleton?
<jtv> adiroiban: I don't think it's worth it.
<jtv> We try not to cache things in ORM-backed classes.
<adiroiban> jtv: for the future, I was thinking that we can move all translation statistics methods into this new object
<lifeless>  think + is used for many things
<lifeless> +text, for instance
<jtv> adiroiban: that still won't make it very expensive to create one of these objects, and bear in mind the object only lives for the duration of the request so it's not taking up space across requests either.  I just don't think it's a problem.
<adiroiban> jtv: ok :)
<jtv> We have some other classes like this, like SourcePackage.
<adiroiban> jtv: thanks. than I'm good. I will come back with a real MP :)
<jtv> adiroiban: great, thanks!
<adiroiban> lifeless: are you saying that I can use +statistics to retrieve an object and then have something like IPOFile/+statistics/+statisticsview ?
<lifeless> maybe
<lifeless> I'm going to try sleeping again :P gnight
<adiroiban> :)
<adiroiban> good nigh
<StevenK> henninge: No news is good news?
<henninge> StevenK: sorry but most likely yes ;)
<StevenK> henninge: Assuming silence to mean 'r=<nick>' is fraught with peril. And most likely, a lynching.
<henninge> StevenK: yes, I got distracted but I am preparing my reply now. Hang in there.
<henninge> StevenK: The test uses a celebrity for software-center-agent but the test uses this:
<henninge> > +    >>> celebrity = factory.makePerson(name='software-center-agent')
<henninge> Or am I missing a hidden functionality of makePerson that will return an existing person if the name matches?
<StevenK> henninge: software-center-agent doesn't exist in the sampledata
<henninge> But how can it be a celebrity, then?
<StevenK> The celebrity look up is done via name
<StevenK> So Julian and I thought it easier if the tests that needed it (of which there are about 4) just created it
<bigjools> celebs don't need to be in the sampledata
<henninge> I thought they do ...
<bigjools> but StevenK does need to write a database patch to add it to production ;)
<StevenK> I do? It already exists in production
<henninge> bigjools, StevenK: But is it not bad practice to do something like getUtility(ILaunchpadCelebrities).software_center_agent when that is not defined in the interrface?
<bigjools> henninge: what interface?
<StevenK> The interface code calls that
<StevenK> henninge: It only does so for commercial P3As
<jtv> abentley, care to review this tiny branch?  https://code.launchpad.net/~jtv/launchpad/recife-test-fixup/+merge/29216
<abentley> jtv, sure.
<abentley> jtv, what does "recife" mean?
<jtv> abentley: thanks.  Recife is a feature branch we're working on (after the location of the corresponding sprint).
<abentley> What does it mean?
<jtv> It is the location of the sprint corresponding to the feature we're working on.
<abentley> jtv, I'm sure it won't be confused with "recipe" at all :-P
<abentley> jtv, Why does being a feature branch mean that EC2 is no help?
<henninge> StevenK: review sent, r=me but with a little uneasiness.
<jtv> abentley: well, "ec2 land" is no help.
<jtv> abentley: Thinking I could just grab Henning, I failed to explain much about the change.  A template can be attached to either a ProductSeries or a DistroSeries + SourcePackageName.  In a review change to a test I wanted to make a template as a "cousin" to an existing template.  But instead of creating it for a distroseries + sourcepackagename like the existing one, I created it for a productseries.
<abentley> jtv, I see.
<abentley> jtv, r=me.
<jtv> abentley: thanks
* henninge 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
<bigjools> jtv: so you're using code names named after the sprint location too huh?
<bigjools> this is a good trend.  I want a Maldives project.
<jtv> bigjools: not as such, but coming up with a terse description of the feature somehow never became much of a priority with such a catchy name.
<bigjools> jtv: we started tagging bugs "wellington" remember :)
<mwhudson> tahiti would be ok, too
<bigjools> as would tortola
<bigjools> which is a really cool name
<jtv> Read the draft treaty the global-warmists were working out in Kopenhagen...  Bali meeting, Bangkok meeting, Hawaii meeting...  really handy.
<bigjools> there's certainly some increased air temperature at those meetings
* 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-07-06
<jtv> henninge: do you want this one?  https://code.launchpad.net/~jtv/launchpad/recife-another-testfix/+merge/29262
<henninge> jtv: anything to get this merge done ... ;)
<jtv> henninge: the fix should work equally with or without the merge, so should be easy.
<jtv> hi danilos
<danilos> jtv, hi
<henninge> danilos: hi ;)
<henninge> jtv: you are talking "product series" but the change is about "distro series". Why is that?
<jtv> henninge: probably a simple slipup... where exactly?
<henninge> jtv: in the mp description
<jtv> henninge: oh, then it's definitely a typo.  Finger memory.
<henninge> ok
<henninge> let me just verify that htis helps ... ;)
<jtv> As you would have guessed from factory.makeProductSeries(distribution=ubuntu)  :-)
<henninge> jtv: great. it works!
<henninge> jtv: r=me
<jtv> henninge: dankeschÃ¶n!
<henninge> jtv: will you merge it into recife right away?
<jtv> henninge: it's currently pushing
<henninge> great! :-D
<jtv> It Is Pushedâ¢
<henninge> I just remembered that there were also windmill tests failing.
<jtv> henninge: got a log?
<henninge> jtv: yes, it's in the same
<henninge> http://people.canonical.com/~henninge/merged-lp.translations.tests.log
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: lunch || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bigjools changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [bigjools] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * bigjools waves at gmb
<gmb> Hullo
<gmb> bigjools, Diff me.
<bigjools> I just 'bzr send'ed the MP, it should appear shortly
<gmb> RIghto
<bigjools> https://code.edge.launchpad.net/~julian-edwards/launchpad/oops-dspr-page-bug-592417/+merge/29273
<gmb> bigjools, r=me
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> thanks gmb.  And see yer next week.
<gmb> Would've been quicker but I had a tea-based emergency
<gmb> bigjools, Welcome. And likewise, unless I go blind in the interim.
<bigjools> as in you needed more
<bigjools> or as in you spilt it
<gmb> bigjools, It had gone cold.
<bigjools> oh disaster
<gmb> Indeed. Calamity, oh woe is me, etc.
<gmb> Thankfully, it has now been refreshed. Hurrah, etc. Let joy be unconfined.
<bigjools> and on that note I need coffee.  Some Finca el Fany awaits.
<noodles775> bigjools: I've marked the mp as ui=me, but with a small suggestion - see what you think.
<bigjools> noodles775: ok cheers
<bigjools> noodles775: see https://dogfood.launchpad.net/ubuntu/+source/uex/1.0.0.9-1/+index
<bigjools> with that listing change
<bigjools> I agree that the old way looks a bit odd, but this way looks more odderer :)
<bigjools> noodles775: a third option to add to your suggestions would be to centre the data across the three <td>s
<noodles775> bigjools: there are other class (and class combinations) for tables in the stylesheet(s)... I'd recommend going with one of those (as they're what have been developed for consistency).
<noodles775> I think centering the data inside the td's would be odderer still ;)
<bigjools> true
<bigjools> :)
<bigjools> noodles775: remind me where the css files live ....
<bigjools> ah found 'em
<noodles775> Great.
<bigjools> noodles775: unfortunately nothing looks that good
 * noodles775 has a look at product and other pages.
<noodles775> bigjools: every table I can find is using the listing class?
<noodles775> That doesn't mean it looks good ;), but does mean that it would be best to use it and if we later improve the look of table.listing, they'll all be improved together... or is it something particular about this instance that you didn't like?
<bigjools> noodles775: ok what about now?
<noodles775> What don't you like about it? (and definitely not if you're going to land this RC ;) 0.
<bigjools> interestingly FF doesn't render the end of the "Size" heading cell marker
<bigjools> it's too wide
<bigjools> it only needs to fill the space it needs
<bigjools> IMO
<noodles775> That's what the 'narrow' class is for in the stylesheet... not sure why it's not doing anything though.
<bigjools> there's a "compressed" as well but it doesn't do anything either
<bigjools> noodles775: aha, check it now
<bigjools> not sure about the "-" characters
<bigjools> maybe just leave it blank
<noodles775> hmm... it's still wide for me (I've shift-refreshed). What classes did you add?
<bigjools> noodles775: it renders differently on Chrome and FF :/
<noodles775> According to the chromium inspector, the 'narrow' isn't doing anything...
<bigjools> "narrow listing"
<noodles775> ah.
 * noodles775 looks in FF
 * bigjools shakes fist at browsers
<noodles775> bigjools: so right, I think you've found the right classes, and up to you whether you leave the '-' in or not.
<noodles775> Might be worth checking with sinzui or creating a bug about the 'narrow' class seemingly being ignored on chromium.
<noodles775> (just in case he already is aware of it).
<bigjools> yeah, I'll stick with "narrow" then and file the bug
<bigjools> cheers
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: - || queue: [sinzui] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> gmb, mars: I have a branch that removes lots of crufty lint checking
<mars> sinzui, I'll take it
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: -,sinzui || queue: [sinzui] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> sinzui, out of curiosity, does pylint have a test suite?
<mars> specifically, a test suite with sample data
<sinzui> I do not think it does
<sinzui> It does not have a public bug tracker
<mars> sinzui, does your linter choke on conflict markers?  '<<<' and '>>>'
<sinzui> no
<mars> does it flag them? :)
<sinzui> It looks for 7 start or close, which is better than what we had
<mars> 7 start or close?
<sinzui> mars: our linter reports the start of a conflict marker. pocketlint report the start and the end
<mars> ah, ok
<mars> sinzui, why was jslint was removed from the list of checks?  Do we have a backup?
<sinzui> pocket-lint uses spider-monkey too
<sinzui> it however reports problems consistently with all the other linters
<mars> sinzui, a simple test should verify that - remove a semicolon from a JS file, see if it catches it
<sinzui> yep
<mars> I did not even know we had this lp-verbose.py stuff
<sinzui> I do
<mars> it's like rummaging through an attic
<sinzui> You could run lint.sh manually to do a serious analysis of modules to locate duplicate code
<mars> sinzui, reviewed
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: -,- || queue: [sinzui] || RC branches jump the 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: gmb, mars || reviewing: -,- || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> thanks
<sinzui> mars, python-pocket-lint is a launchpad-developer-dependency. Did you update today and do you see that we guarantee the file?
<mars> update-manager died with a system crash, so no, I missed that :p
<mars> How ironic is it that change that come from our PPA are not "a source that supports changelogs"
<mars> sinzui, ok, got it, thanks.  You may want to tell the lp-dev list after this lands.
<bigjools> mars: hysterical raisins
<sinzui> mars, I certainly will. I get to end two action items from the reviewers and team lead meetings
<mars> nice
* gmb changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gary_poster: fancy a review of my branch "make LFA.restricted changable"? https://code.edge.launchpad.net/~adeuring/launchpad/bug-39674-lfa-editable/+merge/29314
<gary_poster> adeuring: what could you possibly be talking about? ;-) Sure, looking
<adeuring> gary_poster: thanks!
<gary_poster> approved, adeuring
<adeuring> gary_poster: thanks!
<gary_poster> np :-)
<adeuring> gary_poster: thanks for your  suggestion to use assertRaises(setarr...)
<gary_poster> adeuring: welcome, glad you like it
<bac> mars are you around much longer for another review?
<mars> bac, gah, no, sorry, being burned by the release now :(
* mars changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> ah right
<bac> no problem
#launchpad-reviews 2010-07-07
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: Ursinha || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: On call: jtv || reviewing: Ursinha || queue: [StevenK] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> On call: jtv || reviewing: foxxtrot || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> jtv: are you not reviewing today?
<jtv> EdwinGrubbs: yes, but done.
<jtv> Well, done accepting new reviews.
<EdwinGrubbs> ok, nm.
<EdwinGrubbs> bac: according to the kanban board, you're waiting for a review. Do you want me to take it? I have one if your bored?
<bac> hi Edwin-lunch
<bac> the kanban is not exactly right.  the branch has been reviewed but i'm making the suggested changes.
<leonardr> Edwin, can you review https://code.edge.launchpad.net/~leonardr/launchpad/grant-permissions-oauth/+merge/29425 ?
<leonardr> gary: i'd also like your opinion on the naming of the new access level and the wording of its description
<gary_poster> leonardr: ack
<leonardr> f'rinstance, manage_permissions might be better than grant_permissions?
<jml> hey, I'm not around
 * leonardr ponders that
<jml> but if I were, I'd be asking whether this is relevant to the gpg/ssh key discussion on the mailing list
<EdwinGrubbs> leonardr: sure
<jml> as it is, I'm not.
<leonardr> jml, let me take a stab at it without knowing the exact details of the current discussion, based on past discussions
<leonardr> there is a solution to this problem that involves gpg keys and digital signatures. however, that solution is more confusing to the unskilled end-user than the 'open a web browser' solution
<jml> hey look, lifeless and wgrant are talking about it :)
<leonardr> all right, i'll go over there
<jml> leonardr, I'm going to wish you the best of luck while I fetch myself a digestiv, open a novel and retire for the evening.
<gary_poster> leonardr: From an end user perspective, Grant Permissions is a much better title because it reflects the common action (and the danger); and let's keep the code name matching it, even though "manage" is more complete and accurate.
<gary_poster> User description wordsmithing: WDYT of changing "Not only will the application will be able to access Launchpad on your behalf, it will be able to grant access to your Launchpad account to any other application." to "The application will be able to grant access to your Launchpad account to any other application "
<gary_poster> IOW, get rid of "Not only will the application will be able to access Launchpad on your behalf,"
<leonardr> makes sense
<gary_poster> cool.  Otherwise, looks good.  Thank you.
<EdwinGrubbs> leonardr: review sent
<leonardr> thanks
#launchpad-reviews 2010-07-08
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> noodles785: are you reviewing today?
<noodles785> EdwinGrubbs: I haven't been no (I'm currently working on an ISD project), but if you've something urgent I can take a look.
<EdwinGrubbs> no, it's not urgent
<EdwinGrubbs> nevermind
<EdwinGrubbs> rockstar: are you reviewing today?
<rockstar> EdwinGrubbs, indeed I am, as soon as I get my shit together for the morning.
<rockstar> EdwinGrubbs, go ahead and throw yourself on queue.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [Edwin] || RC branches jump the queue || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> here's the mp: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-568390-cyclic-membership-error/+merge/29407
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> EdwinGrubbs, maybe validateOwner's abstract method should return False or raise NotImplementedError.
<EdwinGrubbs> rockstar: it's not required that validateOwner() be overridden. The vocabulary normally does a good enough job. This is the only subclass that actually does override it.
<EdwinGrubbs> so raising NotImplementedError isn't necessary, and returning false would be meaningless.
<rockstar> EdwinGrubbs, pass seems like it's very unopinionated
<EdwinGrubbs> rockstar: it's no different than LaunchpadFormView.validate().
<rockstar> EdwinGrubbs, okay.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* 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-07-09
* 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
<noodles785> Hi adeuring, I've got some small cleanups to some branches you looked at last week (it didn't get to land before pqm closed). I've merged them into devel and removed some unnecessary code. Can you take a look when you've time?
<noodles785> https://code.edge.launchpad.net/~michael.nelson/launchpad/598464-get-or-create-in-devel/+merge/29442
<adeuring> noodles785: ok, I'll look
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles785 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles785> It's not really 853 lines, the diff from what you've already reviewed is 100 lines (as on the MP)
<noodles785> Thanks.
<adeuring> noodles775: so, I should look at the diff between r11095 and 11098?
<noodles775> adeuring: yep, which I've pasted there for you (well, except for r11098 adds the __future__ import).
<adeuring> noodles775: r=me
<noodles775> Thanks adeuring
* 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
<bac> hi adeuring
<adeuring> hi bac!
<bac> doesn't look to be too busy today
<adeuring> bac: yeah, I did just review.
* 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
