#launchpad-reviews 2009-11-30
* sinzui changed the topic of #launchpad-reviews to:  on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * henninge forgot what "Monday" means ... ;)
<henninge> sinzui: ping
<sinzui> hi henninge
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> sinzui: I am looking at the obsolete-styles branch.
* abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> sinzui: I am not sure about this kind of indention: http://paste.ubuntu.com/331726/
<sinzui> I am
<sinzui> I am sure that is the best indentation
<henninge> sinzui: ;)
<sinzui> henninge: CSS is like C, there is no indention, so we choose what we like. I like Python
<henninge> sinzui: I was about to ask if that is the goal.
<sinzui> henninge: That is also the indentation in style-3.0.css
<henninge> sinzui: I see
<henninge> sinzui: should style.css be fixed eventually or will it go away, anyway?
<sinzui> It should go away
<henninge> ok
<sinzui> once this branch lands, We can move the official global styles to the new CSS. We then have to decide what to do about the locale rules in the old stylesheet.
<henninge> sinzui: I see added vertical space between the footer and the content on branch and translations pages. It looks like this is now the same as on the overview page and I think it looks good. Was that intentional?
<sinzui> Yes. There were a few pages that were normalised by this
<henninge> cool
<mrevell> Hey, anyone want to take a look at the what's new branch? The diff is tiny -- https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-3111/+merge/15426
<henninge> abentley: ^
* abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: sinzui, mrevell || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> mrevell: r=abentley
<mrevell> thanks abentley!
* abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> sinzui: r=me
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> henninge: thanks.
<adiroiban> henninge: regarding MP reviews, when adding a new MP, should I ping someone, or it is ok just to add them in LP ?
<henninge> adiroiban: it is always a good idea to ask here.
<henninge> adiroiban: the rule is: *You* are responsible for getting a review for your branch.
<adiroiban> henninge: ok. then I should ping the current reviewer and ask him/her to look to my MPs ?
<henninge> adiroiban: which is me, so you already did ... ;)
<adiroiban> yep. One of my MPs is handled by Danilo
<henninge> adiroiban: how many are there?
<adiroiban> henninge: right now I got 4... for trivial bugs
<henninge> adeuring: I see two, bug-135008 and bug-97293
<henninge> wow, these are old ;)
<henninge> adiroiban: ^
<henninge> adeuring: tschuldigung
<adeuring> henninge: kein problem :)
<henninge> adiroiban: which of these will be reviewed by danilo?
<adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-487970/+merge/15250
<adeuring> adiroiban: sorry, I failed bady on Friday merging your branch :( I'll try it again when PQM opens again
<adiroiban> this one is reviewed by Adel https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750/+merge/15292
<adiroiban> but there are other 2 without a reviewer
<adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-135008/+merge/15347
<adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-97293/+merge/15391
<henninge> adiroiban: I'll take one and slap eon on the queue. Maybe abentley will take one, too.
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: adiroiban, - || queue [adiroiban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> oh, that one is trivial
<adiroiban> henninge: yep. all of them are trivial :p 
<henninge> adiroiban: are you familiar with YUI?
<adiroiban> nope
<abentley> henninge: Which one are you doing?
<henninge> abentley: 97293, bug I can do both if you are busy, they are quite small.
<abentley> henninge: No, I'm not busy.
<henninge> ;)
<henninge> adeuring: instead of document.getElementById we use Y.get( '#...')
<henninge> adiroiban: ^
<adiroiban> henninge: ok. I can do that. I was just looking at selectWidget function from the same file
<henninge> or maybe that should be Y.one in yui3?
<henninge> adiroiban: yes, this code is older than the introduction of yui3
<adiroiban> henninge: selectWidget
<henninge> adiroiban: let me look at the full code ...
<henninge> adiroiban: yeah, this whole file would need to be converted to use yui. So never mind yui3 for now...
<adiroiban> henninge: i'm still learning about LP dev practice. In the future I will try to use YUI3
<henninge> adiroiban: that is good! but in this case not using it probably was the better choice, to keep the code consistent.
<abentley> henninge: Do you know why there's a search_lang attribute on LanguageSet?  It doesn't seem to be part of the model.
<adiroiban> abentley: search_lang is the HTTP_GET argument . not part of the model
<adiroiban> abentley: it is only relevant for the form. Is the string used to match language names while doing the search
<adiroiban> abentley: maybe I did something stupid, and âsearch_langâ should have been defined in another place
<henninge> abentley: Let me look at it.
<abentley> adiroiban: If it is only relevant for the form, why is it on the ILanguageSet interface?
<abentley> henninge: It's adiroiban's addition, I see now.
<adiroiban> abentley: i didn't know where it needs to be defined... so i put it there
<adiroiban> abentley: can you suggest me how it should have been implemented? I can fix that :)
<abentley> adiroiban: I suggest defining a new interface called ILanguageSearch in lib/lp/translations/browser/language.py
<adiroiban> abentley: ok. you can leave all your observation as a comment in MP and I will try to solve those requests
<abentley> adiroiban: Since ILanguageSearch will have only the member you want, you won't need the field_names list.
<abentley> adiroiban: What was your motivation for changing LanguageSetView into a LaunchpadFormView?
* abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: adiroiban, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: i was thinking this is the âstandardâ way of adding JS autoselect entry to a form
<danilos> adiroiban, hey, thanks for all the work, I see you are coding on steroids :) I've reviewed your branch, do note that we won't be able to land any one of them before Wednesday, and most likely not before next Monday (we are only allowing release-critical landings right now)
<adiroiban> abentley: I looked at all the other forms that have auto-focus enabled and they were all using LaunchpadFormView
<abentley> adiroiban: Why do you tal:define script and use script in tal:content, instead of doing tal:content="view/focusedElementScript" ?
<henninge> adiroiban: r=me
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: -, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adiroiban: you will have to wait until next week to land it, though, as this will not be considered release-critical.
<adiroiban> abentley: at is was use both in tal:condition and tal:content
<henninge> adiroiban: please ping danilo or me on Monday to do it for you.
<adiroiban> abentley: as is was use both in tal:condition and tal:content. I can change it if required
<abentley> adiroiban: I am asking questions so I understand your patch better.  I'm not implying your choices are wrong.
<adiroiban> abentley: np. 
<abentley> adiroiban: So the only problem I see is the first one: putting search_lang on ILanguageSet.  The rest looks fine, now that I understand it.
<adiroiban> abentley: ok. I will fix that.
<bac> henninge, abentley: could one of you have a look at https://code.edge.launchpad.net/~bac/launchpad/ec2_attached/+merge/15272
<henninge> bac: looking
<abentley> adiroiban: The Interfaces in our "interfaces" directories are meant to correspond with model objects.  ILanguageSet shouldn't specify anything that LanguageSet can't provide, which is why search_lang didn't belong on ILanguageSet.  Does that make sense?
<adiroiban> abentley: yep. I was not aware of that policy
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: bac, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bac: stray blank line at line 8 of the diff.
<danilos> henninge, adiroiban: re bug-97293, there's a jslint warning: was that there before as well? if it was, never mind it, but if it wasn't, you should fix it first :)
<henninge> danilos: oops, missed that
<adiroiban> danilos: it was there
<adiroiban> i did not touched that line 
* abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: bac, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adiroiban, ok, if you can figure out how to fix it, that'd be great, if not, never mind :)
<adiroiban> danilos: I have no idea how to fix it 
<danilos> adiroiban, ok, then just ignore it (or perhaps file a bug against launchpad-foundations)
<bac> henninge: gone
<adiroiban> danilos: i can read about JS closures and try to fix it ... if I will fail, will file a bug
<henninge> bac: I am surprised there are not more parameters to the Option constructor but I guess boolean values defaulting to False are the default, right?
<danilos> adiroiban, I think this is best left for whoever made it such, the code where it happens does look ugly with a debugging function being defined in a loop
<bac> henninge: i can double-check that
<henninge> bac: the Option 'force' above that looks the same ...
<bac> henninge: yes, that was my example
<danilos> adiroiban, it's not really a function definition, it's a YUI3 way of doing things, so I just wouldn't worry about it :)
<henninge> danilos: I agree.
<adiroiban> danilos: ok. then no need to file a bug?
<danilos> adiroiban, you can if you want to, but don't spend time trying to fix it :)
<adiroiban> danilos: ok
<henninge> bac: where does run get called? I see the extension of the parameter list but would expect that to happen at the call site, too.
<henninge> bac: oh, it's from EC2Command
<bac> henninge: let me look.  it's part of the 'commandant' machinery
<henninge> bac, yes, I just just figured.
<henninge> bac: r=me
<bac> thanks henninge.
* henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: regarding the addition of LanguageSearch interface. Should I put it in lp.translations.interfaces.language rather than lp.services.worlddata.interfaces.language ?
<abentley> adiroiban: As you explained, it has no purpose outside the context of the view.  Therefore, I suggest putting it next to the view.  I don't think it has value as a public API.
<adiroiban> abentley: next to view means putting the interface in the same file ?
<abentley> adiroiban: Yes.
<adiroiban> abentley: :) ok
<abentley> I specified that file here and in the review.
<adiroiban> abentley: I have pushed the required changes. What would be the next step?
<abentley> adiroiban: I'll have a look.
<adiroiban> abentley: what is the normal LP process. Should I add a comment, ping you... or both ?
<abentley> adiroiban: Normally, you'd add a comment saying you've made the requested changes.
<abentley> adiroiban: Why have you added ILanguageSetSearch to lib/lp/services/worlddata/interfaces/language.py ?
<adiroiban> abentley: it should be an error.
<abentley> adiroiban: I don't understand.
<adiroiban> abentley: it's just me being stupid . I'll fix that right away
<abentley> adiroiban: Okay.
<abentley> adiroiban: This looks good to me now.
<adiroiban> abentley: sorry for messing it 
<abentley> adiroiban: np
<adiroiban> abentley: ok. Should i ping you on Monday, or everthing is ok?
<abentley> adiroiban: Please ping me on Monday.
<adiroiban> abentley: ok. Henning  told me I responsible for getting a review for your branch. I am also responsible for getting it merged into devel ?
<abentley> adiroiban: I'm not sure.  This is the first patch I've handled from a non-committer.
<adiroiban> abentley: ok.
<abentley> adiroiban: It would be simpler if this wasn't week 4.
<henninge> abentley, adiroiban: I'd say you are responsible as long as non of us says "I'll handle it from here on" ;)
<henninge> adiroiban: So get back to us about it in a week and we'll take the landing off your hand.
<adiroiban> henninge: ok. no hurry to have them accepted/merged. I just wanted to learn the workflow
<henninge> abentley: you can just land it, it will be credited to Adi.
<abentley> adiroiban: Have you signed the contributor agreement?
<henninge> adiroiban: we are in feature-freeze this week because of the roll-out, that is the only reason for not going straight ahead.
<adiroiban> abentley: yep
<adiroiban> abentley: you can check here https://edge.launchpad.net/~contributor-agreement-canonical
<henninge> adiroiban: It *may* be best to have one person land all your branches.
<henninge> but I am not sure.
<adiroiban> henninge: ok. in that case, what is the next action I will have to do ?
<henninge_> adiroiban: The next step is run the full test suite on it (on ec2) and land the branch
<henninge_> adiroiban: oh, and since a week will pass, wou will also have to merge devel again to check for conflicts.
<adiroiban> henninge_: hm...  
<adiroiban> henninge_: i can do the devel merge... but not sure about ec2 testing
<henninge_> adiroiban: once that is done (and resolved), ping me and I'll do the testing and landing.
<henninge_> adiroiban: no, you cannot.
<adiroiban> henninge_ ok.
<henninge_> adiroiban: you *could* run the test suite locally (make check), that'll take a few hours, but there is no need for that.
<adiroiban> henninge_: on my computer it will take days :)
<adiroiban> henninge_: but I'm running the translation test suite
<adiroiban> and also testing for specific stories
<henninge_> cool
<henninge_> adiroiban: I will go home now. ttyl
<adiroiban> henninge_: have a nice evening
* henninge_ changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge_> adeuring: you too!
<adeuring> henninge: ...and you ;)
<deryck> abentley, I'm about to go EOD, but I have a really simple css fix.  care to take a look, or can you do an offline review later?
<abentley> deryck: I'll do an offline review later.
<thumper> deryck: I've grabbed the branch
<thumper> deryck: and happy to look at it
<deryck> thumper, excellent, thanks!
<deryck> thanks anyway, abentley (since thumper has it now)
<deryck> thumper, I've got to bail for the day.  Will check back later about the review.
<bac> abentley: i've got a branch coming up which is targeted to the next cycle.  would you have time to look at it?
<abentley> bac: probably.  on a call.
<bac> abentley: ok.  i'll paste it when it shows up.  no rush.  thanks.
<bac> abentley: when you have time: https://code.edge.launchpad.net/~bac/launchpad/bug-429802/+merge/15449
* abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> abentley, rc-ping, would you feel comfortable reviewing the JavaScript for https://code.edge.launchpad.net/~mars/launchpad/fix-picker-result-parsing-487975/+merge/15436?  39 lines.
 * thumper looks
<abentley> mars: There seems to be an extra space before the = in line 9 of the diff.
<abentley> mars: Aside from that, it looks fine to me.
<mars> abentley, nice catch.  Thanks for the review.
<abentley> mars: np
* abentley changed the topic of #launchpad-reviews to:  on-call: - || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> abentley: heh, I said exactly the same thing :)
<mars> thumper, thank you too.  I'll send off this branch, then finish up your reply
<thumper> mars: ok
<mars> thumper, the question I had about creating a new wrapper was the commit_message_listener function on line 25.
<thumper> mars: I still feel like I'm missing something
<mars> thumper, that has a green and a red flash on the link.  The current editor wrapper flashes the message itself.  I take it this is an extra, additional bit of flash?
<thumper> mars: ah
<thumper> mars: this is flashing the menu link not the editor itself
<thumper> mars: if the commit message ends up being empty
<thumper> mars: we hide the multiline editor
<thumper> mars: show the link again, and flash the link
<mars> ah
<mars> ok
<thumper> mars: it would be nice to be able to use widget.on('save')
<thumper> mars: rather than widget.editor.on('save')
<thumper> mars: is this possible, because at the moment it doesn't work
<mars> yep
<mars> it didn't work in PR2.  It probably works fine in 3.0 final.
<mars> rather, that feature didn't work in PR2 - I tried to code it already, and gave up :)
<flacoste> mars: what is that?
<flacoste> that's parsing the HTML representaion?
<flacoste> why is it doing that
<flacoste> we shouldn't be doing that
<mars> flacoste, one of many approaches I take it.
<flacoste> you should be requesting the field HTML output directly
<flacoste> that's a deprecated approach
<mars> this is an original widget
<flacoste> we talked about making that view a 404
<flacoste> to fix that broken behaviour
<flacoste> and it's the third time that somebody "fixes" it
<mars> good thing you didn't then :)
<flacoste> that's crazy
<thumper> mars: it doesn't work in 3.0 right now with the widgets we have
<mars> thumper, yep, I know.  I would have to add it back into lazr-js, then publish that new version in LP before you could use it.
<thumper> ok
<mars> thumper, so maybe in a release or two.  Your branch did show some serious API problems though.  I like that.
<mars> flacoste, is there a bug for "Don't use the XHTML view" I can reference with an XXX?
<flacoste> probably, but it will have been marked as 'Fix released'
<mars> flacoste, IIRC publishing instructions for the Right Way To Do It is on the UI Action items list
<mars> ok, I'll exhume the bug
<mars> flacoste, I can't find the bug number for XHTML representations.  I'll just file a new one and mark it as an XXX.
<mars> flacoste, anything else needed for an r-c?
<flacoste> mars: nope, just updated the m-p
<mars> thanks
<thumper> mars: so want to approve the js review? https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260
<mars> thumper, doing so now
<thumper> mars: thanks
<thumper> salgado-afk (or flacoste): https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 rc?
<mars> thumper, two small fixes, r=mars
<thumper> mars: ok, ta
<mars> thumper, to clarify the code, a quick comment with the same reason you gave me on IRC should be enough.
<flacoste> thumper: rc=me
<thumper> flacoste: ta
#launchpad-reviews 2009-12-01
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/fix-date-review-requested/+merge/15464 anyone?
 * mwhudson looks
<thumper> mwhudson: thanks
<mwhudson> thumper: will sort_order now always be non-None?
<thumper> mwhudson: yes
<mwhudson> it seems like it would be if you could create a review in the WIP state
<thumper> mwhudson: but then it wouldn't be in the list
<thumper> mwhudson: so it won't need to be sorted
 * thumper is heading to get shoes for the girls
<thumper> back later
<mwhudson> thumper: ah ok
<thumper> mwhudson: I could add another fallback to date_created if you think it is worthwhile
<mwhudson> thumper: nah
<mwhudson> thumper: reviewed 
<thumper> ta
<mwhudson> thumper: if you get the chance: https://code.edge.launchpad.net/~mwhudson/launchpad/code-import-paranoia/+merge/15467
<noodles775> Hey gmb, I assume you're doing bugs RC stuff today and would prefer not to be reviewing non-urgent non RC branches?
<gmb> noodles775: Pretty much, yes. I might have a chance to take a look at anything like that later, though.
<noodles775> gmb, thanks. There's no rush for this one at all, so just see how you go.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: - || queue [] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> morning gmb
<gmb> Hi bac
<bac> i'll be reviewing today but not until 1700UTC
* noodles775 changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: - || queue [noodles] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * noodles775 pops one in the queue just for whoever has time later (not urgent at all)
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/487009-db-generalise-ibuilder-1b/+merge/15477
<BjornT_> gmb: can you review this small patch? it's an rc candidate: https://code.edge.launchpad.net/~bjornt/launchpad/choiceedit-click-handler/+merge/15484
<gmb> BjornT_: Sure. I'll take a look at it in a couple of minutes.
<BjornT_> thanks
<gmb> BjornT_: r=me
<BjornT_> gmb: thanks!
<salgado> https://code.edge.launchpad.net/~stub/launchpad/bug-490239-session-tuning/+merge/15403
<salgado> https://code.edge.launchpad.net/~stub/launchpad/auth-mirror-tables/+merge/15414
<salgado> gmb, it'd be great if you could take the two above ones as well; both are RC candidates
<gmb> salgado: Will take a look at those after lunch.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: - || queue [stub(rc), stub(rc), noodles] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> salgado: Also, https://code.edge.launchpad.net/~gmb/launchpad/indexed-message-parents/+merge/15486 is an RC candidate. adeuring is reviewing it now.
<gmb> salgado: It's a fix for bug 394097
<mup> Bug #394097: accessing message 0 of bug 371281 gives wrong return type of IMessage <api> <bug-page> <oops> <Launchpad Bugs:Triaged by allenap> <https://launchpad.net/bugs/394097>
<salgado> gmb, cool, thanks for fixing that one!
<gmb> salgado: Working it out was mostly allenap's work, I assure you. I just contributed the patch to stop it from happening.
<gmb> salgado: Do you want me to request the rc review now or wait until adeuring is done with it?
<salgado> gmb, it's ok to request it now
<gmb> Ok
<gmb> ARGH, bloody person picker.
<gmb> salgado: Requested. Doesn't say "RC" but I think you can work that out.
<salgado> yep, I can
<gmb> adeuring: Note: the docstring for TestBugIndexedMessages is bong; fixing it now.
<salgado> gmb, do you still need an RC review for https://code.edge.launchpad.net/~gmb/launchpad/bugzilla3.4-see-also-bug-419134/+merge/15404 ?
<gmb> salgado: No; QAing was too much of a pain, so we'll find a better way to do QA and CP it later.
<gmb> (If absolutely necessary)
<salgado> ok
<gmb> salgado: I've made the cahnges you requested.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: stub(rc) || queue [stub(rc), noodles] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: stub(rc) || queue [noodles] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Hurrah for branches that are all removals.
<gmb> Boo for branches that contain sampledata changes
<gmb> salgado: Should I land my branch on devel or db-devel?
<salgado> gmb, devel
<gmb> Cool
<gmb> salgado: I'm a little leery of approving https://code.edge.launchpad.net/~stub/launchpad/auth-mirror-tables/+merge/15414. Not because I don't think it looks right - so far, so good - but I don't have the SQL nouse to spot errors, I suspect.
<salgado> gmb, I know how that feels, but I find it unlikely we have much people that can spot errors on SQL written by our DBA
<gmb> salgado: Heh. Fair point. Just wanted to make you aware of it :)
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: noodles || queue [] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775: r=me with a few minor changes.
* gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: - || queue [] || Currently reviewing urgent and RC branches only || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb!
<gmb> np
* gmb changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: do you want to have a look at https://code.edge.launchpad.net/~thumper/launchpad/fix-date-review-requested/+merge/15464
<salgado> bac, sure, I'll review it later
<bac> salgado: another: https://code.edge.launchpad.net/~stub/launchpad/auth-mirror-tables/+merge/15414
<salgado> bac, reviewing that one right now. :)
<bac> salgado: great.  just doing clean up.
* sinzui changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<ursula> bac, hi :)
<bac> hi ursula
* bac changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<ursula> oh :(
<bac> ursula: what's up?
<bac> ursula: you have a review?  np
<bac> ursula: at line 53 of the diff, please factor out the test for 'if product_series'
<ursula> bac, let me see
<bac> ursula: this isn
<bac> ursula: this isn't for an RC is it?
<ursula> bac, nope, I just wanted to have a review before going on vacations
<bac> ursula: ok
<bac> that code change is one thing
<ursula> bac, what do you mean by factor out the test?
<bac> ursula: does this new field show up in the UI?
<bac> ursula: i mean:
<bac> if product_series is not None:
<bac>      if blah:
<bac>        do something
<ursula> ah, ok :)
<bac>     elif
<bac>     do something else
<bac> ursula: so how is this field set?  is it in the UI?
<ursula> bac, yes, in the +changetranslators one
<ursula> I think I mentioned that in the MP..
<bac> ursula: right, i see that now.
<bac> ursula: you're going to need some tests for this.  a doc test, a story, and webservice too.
<ursula> bac, I see
<ursula> bac, well, I guess I'll have to do that when I return... or someday in this meantime
<bac> ursula: ok.  sorry.
<bac> ursula: you'll also need a schema change, no?
<bac> ursula: which will require a db review from jml and stub.
<ursula> bac, already did that, in a separated branch, that has already landed
<bac> ursula: excellent!
<ursula> bac, do you know an example so I can write the tests that are missing?
<ursula> I'll try to do that while I wait at the airport
<bac> ursula: i'd look at how some of the companion fields are tested, like translationpermission
<bac> ursula: that should give you an idea.
<ursula> bac, awesome, thanks muchly
<bac> ursula: have a nice vacation!
#launchpad-reviews 2009-12-02
* noodles775 changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: Hi! could you do a ui review for bug 425583?
<mup> Bug #425583: Improve language set index page <post-3-ui-cleanup> <Launchpad Translations:Triaged> <https://launchpad.net/bugs/425583>
<henninge> noodles775: the bug has a mockup attached to it, there is no branch or mp yet.
<noodles775> henninge: sure.
<henninge> Seeing how this release week is a quiet one for you ... ;-)
<noodles775> henninge: I don't suppose I can do a swap with you?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/487009-db-more-soyuz-extraction/+merge/15533
<henninge> noodles775: sure you can ;)
<noodles775> Thanks!
<noodles775> henninge: ok, done. It looks great! I added a few thoughts that came up as I thought about interacting with it.
<henninge> noodles775: cool, I'll look at it in a minute
<henninge> noodles775: about your branch
<henninge> without wanting to check each line, I trust that the code moved to dispatchBuildToSlave, verifyBuildRequest, _cachePrivateSourceOnSlave, _extraBuildArgs was copied verbatim?
<noodles775> Almost verbatim, I had to check all references to self, so that self.url would become self._builder.url.
<henninge> noodles775: and you had to add a call to _extraBuildArgs
<noodles775> henninge: The extra build arguments used to be created in startBuild(), so yes, I copied that code out into BinaryPackageBuildBehavior._extraBuildArgs() and called it from the specific dispatchBuildToSlave instead.
<henninge> noodles775: I am trying to decide if this is a pure refactoring branch or if it adds functionality that would need tests.
<henninge> noodles775: what about the new code for IdleBuildBehavior?
<noodles775> henninge: yep, as mentioned in the MP, I'm going to add some documentation for the build behavior infrastructure in the next branch, but I've certainly tried to ensure that I'm not adding any new functionality to IBuilder in this branch, just pure refactoring.
<henninge> noodles775: also the return value of "status" is not demonstrated/tested.
<henninge> noodles775: ah, I see
<noodles775> henninge: so, the current behavior of the status property is tested in doc/builder.txt (not exhaustively), and I've tried to ensure that I haven't modified its behaviour.
<noodles775> But without a doubt, it could be better tested.
<henninge> noodles775: oh, I missed that that is moved code, too. Found it now at the end of the patch ;)
<noodles775> Ah, great :D
<noodles775> Note, in that case it's split code, rather than moved.
<henninge> noodles775: let me just verify that the test is really passing and then you're good to go
<noodles775> Ta!
<henninge> yes, the 'Idle' went to the Idle...Behavior
<henninge> noodles775: done r=me
<noodles775> Thanks henninge !
<salgado> allenap, do you plan to write a windmill test for your fix to bug 489342?
<mup> Bug #489342: "'display_name' is undefined" JavaScript error on development <javascript> <Launchpad Bugs:In Progress by allenap> <https://launchpad.net/bugs/489342>
<allenap> salgado: Maybe, but I don't really know where to start right now, and I want to get it landed ;)
<allenap> salgado: I can promise to write a test and do manual QA for the release.
<allenap> If that's okay with you.
<salgado> allenap, that's ok, yes, but can you file a bug for the missing test so that we don't forget about it?
* noodles775 changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> salgado: Certainly. Thanks.
<allenap> salgado: I'll go land now.
<salgado> allenap, please land on db-devel
<allenap> salgado: Sure.
<salgado> I need to get devel closed
<allenap> salgado: Gah! I just submitted it and it's gone in for devel.
<allenap> salgado: I can resubmit to db-devel if you want to close devel.
<salgado> allenap, delete the branch from LP
<allenap> salgado: Okay.
<salgado> or push --overwrite a 'devel' branch on top of it
<salgado> and then resubmit
<salgado> seems to have worked
<henninge> noodles775: thanks for your comment on the languages page. I just replied to it.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> allenap, stub's got a RC branch up for review, can you take it?
<salgado> https://code.edge.launchpad.net/~stub/launchpad/opstats-nodb/+merge/15543
<mars> allenap, ping, would you be able to review a brief set of fixes to the JS unit test suite?  https://code.launchpad.net/~mars/launchpad/fix-js-unittests/+merge/15546
<allenap> salgado: Sure
<allenap> mars: Sure.
<allenap> salgado, stub: r=me
* allenap changed the topic of #launchpad-reviews to: on-call: allenap || reviewing: sinzui || queue [mars] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: sinzui || queue [mars] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> allenap: would you like me to take mars' branch?
<allenap> EdwinGrubbs: I did it already, sorry.
* allenap changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> you shouldn't be sorry. I should be thanking you for saving me work.
<mars> lol
<mars> allenap, thanks for the review
<allenap> Heh :)
<allenap> mars: Welcome :)
<deryck> allenap or EdwinGrubbs -- I have a simple js unit test fix.  30 line diff.
* deryck changed the topic of #launchpad-reviews to: on-call: allenap, Edwin || reviewing: - || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> https://code.edge.launchpad.net/~deryck/launchpad/fix-me-too-unit-test-491441/+merge/15561
<EdwinGrubbs> deryck: I can take it.
<deryck> EdwinGrubbs, cool, thanks.
* allenap changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> deryck: I assume this is not an RC candidate.
<deryck> EdwinGrubbs, no, it's not.
<deryck> Just want it ready when pqm opens is all.
<deryck> simple fix, so catch it while I can.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> EdwinGrubbs: you have time for a testfix/reroll candidate?  https://code.edge.launchpad.net/~gary/launchpad/opstats-nodb/+merge/15569
<EdwinGrubbs> gary_poster: sure, looking now
<EdwinGrubbs> gary_poster: r=me
<gary_poster> thank you
#launchpad-reviews 2009-12-03
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: Guten Morgen! ;)
<noodles775> Hi hennige!
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> :-D
<henninge> noodles775: ah, you are the ocr! :)
<noodles775> Yeah, myself and al-maisan are around today.
 * al-maisan will be around after preparing his cappuccino <wink> :)
<henninge> noodles775: cool, let me prepare an mp for a final ui review. I'd like to have your input on the languages page.
<noodles775> Wow - that was fast?
<henninge> noodles775: well, I did not go for using and improving the lazr-js widget, I am sorry.
<henninge> noodles775: but I quite like what's come out of it and I only had yesterday to do it. ;)
<noodles775> henninge: no, I didn't expect you to do that as part of the initial branch (it's not necessary for the improved behavior, it'd just be a nice bling later).
<henninge> noodles775: It *does* have the client-side filter, just not as-you-type. You need to click.
<henninge> oh, and windmill test ist still missing ...
<noodles775> Aha.
<henninge> noodles775: ok, mp is done.
<henninge> noodles775: I'd ask you to please branch it and have a look at the page.
<noodles775> Great, yep, I always do :-)
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: - || queue [henninge/ui,henninge/code] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: also, I am not sure if I am writing good js/yui code.
<noodles775> OK, I'll check it out too.
<henninge> noodles775: fyi, just pushed a new version.
<noodles775> henninge: is this dependent on other changes currently in db-devel only? Or why are you targeting db-devel?
<henninge> noodles775: well, I just don't want to get caught in yui3final changes by working against the latest version.
<henninge> noodles775: Also, after the roll-out (when this lands) db-devel will have become the new devel. So eventually it will be merged into devel.
<noodles775> ah, I thought the yui3final stuff was to be merged back into devel by now, maybe it wasn't (or I misunderstood). But yes, +1 for landing this on devel (so it hits edge) after the roll-out.
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: -, - || queue [henninge/ui,henninge/code] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: henninge/ui, - || queue [henninge/code] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> henninge: wow - I didn't realise that you had used the picker for the filtering! It should be really easy to update what you've done to filter on keypress.
<henninge> noodles775: beg your pardon?
<noodles775> henninge: I didn't realise that you'd actually done the filtering via JS - I was expecting the normal GET to +languages?blah
<noodles775> It's great!
<henninge> noodles775: Ah! Yes, that was fun I had after hours yesterday ... ;)
<henninge> noodles775: oh, I just read about the "unseen" css class.
<henninge> windmill doc is out-of-date ... :(
<noodles775> which one?
<noodles775> or you mean the actual documentation?
<henninge> https://dev.launchpad.net/Windmill
<henninge> I read about the "unseen" class there but it does not seem to exist. But I am still checking.
<noodles775> BTW: I think you'd be best creating JS unit-tests for your new module...
<noodles775> (you'd still need one very basic windmill test though, hrm, so maybe it's easier just to do the windmill test for the moment).
<noodles775> henninge: also, all_languages should be a module-level var I think? (ie. so it's not evaluated each time hide_and_show is called)?
<henninge> noodles775: yes, sounds reasonable.
<henninge> noodles775: the "unseen class does exist in style.css". Is that not generally available?
<henninge> I could just move it to style-3.0.css
<noodles775> I'd be careful - Curtis is landing a branch very soon that removes a lot of the old styles... so yes, perhaps move it but be prepared for conflicts.
<noodles775> bbiab - OTP
 * henninge has to go afk for 15 min
<noodles775> al-maisan: are you ok to do the code review for henninge's branch? (beuno has a rule about not doing code reviews *and* ui reviews for the same branch).
<noodles775> (I think to do with making UI assumptions based on code and vice-versa, but if it's a real hassle for you at the moment, I can do it).
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: - , - || queue [henninge/code] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: actually, you'll probably want to wait until the ui review is finished anyway (as henninge might have a few more changes from the UI review.)
<henninge> noodles775, al-maisan: yes, and the windmill test is still missing.
<henninge> noodles775: I am reading the review atm
<noodles775> k
<al-maisan> ok, thanks for letting me know.
<gmb> noodles775: Can I get a preliminary UI review for https://code.edge.launchpad.net/~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave/+merge/15599 please? It's pretty rough around the edges but I'd rather get some feedback before starting to polish it.
<noodles775> gmb: sure!
<gmb> Thanks
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: gmb-ui , - || queue [henninge/code] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> gmb: er, wow.
<gmb> noodles775: That's not a good word.
<noodles775> Well, I was just amazed - it's super responsive :-)
<noodles775> and the animation looks really nice.
<gmb> noodles775: Heh - I suspect it won't be quite so responsive with a metric assload of bugs.
<noodles775> Ah, actually, I think the animation is unintended (yep, I just read your MP)
<noodles775> Well, still faster without a full page load hopefully :-)
<gmb> Yeah :)
<noodles775> gmb: sent.
<gmb> Cool
<gmb> noodles775: Thanks; plenty for me to chew on. I'll let you know how I get on :)
<noodles775> Great, thanks.
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: - , - || queue [henninge/code] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> al-maisan: ok, we've finished the ui review for henninge's branch :)
<henninge> noodles775: cool, thank you!
<noodles775> np!
<al-maisan> noodles775: thanks for letting me know, I can go ahead and review the other branch now?
<henninge> al-maisan: please wait until I commit the windmill test. Getting there ...
<al-maisan> henninge: fine .. I need to break for lunch anyway :)
<noodles775> al-maisan: it's the same branch, but yep (what he said).
<henninge> al-maisan: the branch is ready for review now. Thanks for waiting!
<henninge> noodles775: can you please enter your vote on the mp page? Thanks. ;)
<henninge> al-maisan: I will relocate now, back in 30 min max.
<al-maisan> henninge: please point me to your branch first
<henninge> :)
<al-maisan> henninge: where is it?
<henninge> al-maisan: https://code.edge.launchpad.net/~henninge/launchpad/bug-425583-languages/+merge/15594
<al-maisan> henninge: thanks!
<henninge> noodles775: sorry, didn't see your vote. Forgot to reload the mp page ;-)
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: - , henninge/code || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> hello henninge, could you please take a look at http://pastebin.ubuntu.com/333878/ ?
<henninge> al-maisan: looking
<henninge> al-maisan: ah yes, I just noticed myself and fixed it.
<al-maisan> henninge: great!
<al-maisan> henninge: did you push the changes?
<henninge> pushing them as we "speak" ;-)
<henninge> done
<al-maisan> thanks
<henninge> sorry about that ...
<al-maisan> henninge: I also get http://pastebin.ubuntu.com/333882/
<henninge> al-maisan: haven't seen that before ...
<al-maisan> hmm .. that's funny, I get it when running "bin/test --layer=TranslationsWindmillLayer --test=filter_languages"
<al-maisan> henninge: it may be a problem with my local system though .. as long as you land this through ec2 I am happy.
<al-maisan> henninge: there's a small typo in lib/lp/translations/browser/tests/language-views.txt
<henninge> al-maisan: I am just doing that myself again and don't get this error.
<al-maisan> henninge: see my remark about landing via ec2 above
<al-maisan> re. the type please s/For s user/For a user/
<henninge> al-maisan: does ec2 run windmill tests?
<henninge> al-maisan: oh, thanks :)
<al-maisan> err .. good question.
<al-maisan> BjornT_just sent out an email about this I believe
<al-maisan> henninge: so, will this be submitted to db-devel or the jscheck builder?
<henninge> al-maisan: I have never submitted to the jscheck builder ... 
<henninge> am I supposed to do that?
<al-maisan> From BjornT's email:
<al-maisan> next week I'd like us to get rid of the jscheck buildbot builder, and
<al-maisan> instead run the Windmill tests in the normal devel and db-devel (and
<al-maisan> later also production-devel) builders.
<henninge> al-maisan: ah
<al-maisan> so, no windmill tests in the devel builder at present
<BjornT_> henninge: no, ec2 doesn't run windmill tests. i tried running them, but it can run them. the images need to be updated
<al-maisan> s/builder/buildbot/
 * al-maisan has been dealing too much with Soyuz builders :P
<henninge> al-maisan: I had planned to land this next week on staging. by then, db-devel will have become devel.
<henninge> s/staging/devel/
<al-maisan> I see
<al-maisan> henninge: this is a good branch, very nice work, r=me
<henninge> al-maisan: thank you very much!
<al-maisan> you are welcome 
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: - , - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> noodles775, or al-maisan -- I have a 12 line diff for review, to fix a broken js unit test.
* deryck changed the topic of #launchpad-reviews to: on-call: noodles775, al-maisan || reviewing: - , - || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> https://code.edge.launchpad.net/~deryck/launchpad/subscriber-unit-test-fix-491453/+merge/15609
 * noodles775 looks
<al-maisan> noodles775: thanks.
<noodles775> deryck: r=me
<deryck> noodles775, thanks
<EdwinGrubbs> noodles775, al-maisan: can one of you review my tiny branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-230801-renewing-membership/+merge/15610
<noodles775> EdwinGrubbs: sure!
 * noodles775 is loving the 4 or 5 liners :)
<noodles775> EdwinGrubbs: is this targeted to db-devel for a reason? (or are you planning on landing it on devel?)
<EdwinGrubbs> noodles775: I'm planning on landing it on devel after pqm opens, and I wanted to prevent merge conflicts later, since devel still didn't have some of the revisions when I started the branch.
<noodles775> k
 * al-maisan bows out
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: -  || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: on-call: noodles775, rockstar || reviewing: -  || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi gary.  could i interest you in a short lazr.lifecycle review?
<bac> gary_poster: ^^
<gary_poster> bacsure
<gary_poster> bac: sure
<bac> gary_poster: https://code.edge.launchpad.net/~bac/lazr.lifecycle/snapshot/+merge/15612
<bac> thanks
<gary_poster> bac: I like it.  two thoughts, one trivial, the other speculative.
<gary_poster> 1) this is a new feature; therefore, I'd recommend releasing this as 1.1 not 1.0.1
<noodles775> All yours rockstar :-)
* noodles775 changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> noodles775, cheers!
<gary_poster> 2) Did you consider adding sugar so that the documented mechanism is something like ``ignore_me = do_not_snapshot(Attribute('ignored attribute'))`` (or doNotSnapshot, whatever convention lazr.lifecycle is following now; probably camelCase).
<gary_poster> s/./?/
<gary_poster> in an explicit, non-vim kind of interpretation of the substitution :-)
<bac> gary_poster: 1) ok and 2) that's a good idea.  it is rather unwieldy now
<gary_poster> bac: ok cool.  You could keep the same mechanism, so it really is just sugar
<gary_poster> bac: ok, do you want me to approve now, trusting the change, or look at the revision?  Up to you.
<bac> gary_poster: i'll run the sugary fix past you
<gary_poster> bac: ok cool.  ping me when you are ready
<bac> gary_poster: something like: http://pastebin.ubuntu.com/333934/
<gary_poster> looking
<gary_poster> bac, I like that.  Do you like that?
<bac> gary_poster: sure, with the addition of a doc string
<gary_poster> bac: +1
<gary_poster> bac, I approved, also with a request to update the CHANGES file (or whatever it is called)
<gary_poster> oops
<beuno> if wants free karma, I have a branch that just adds an SVG for merging: https://code.edge.launchpad.net/~beuno/launchpad/ppa-icon-svg/+merge/15620
<sinzui> karma?
<sinzui> beuno: rs=me
<beuno> sinzui, ã©ãããããã¨ã
<sinzui> mr. roboto
<beuno> :)
<bac> hey rockstar you still have another review in you?
<rockstar> bac, yup
<bac> rockstar: thanks - https://code.edge.launchpad.net/~bac/launchpad/bug-488762-snapshot/+merge/15628
<bac> rockstar: in case you're interested, it uses this lazr.lifecycle change: https://code.edge.launchpad.net/~bac/lazr.lifecycle/snapshot/+merge/15612
<rockstar> bac, in lib/canonical/launchpad/webapp/snapshot.py you have a # DO NOT CHECK IN
<rockstar> I'm not sure which one you're not supposed to check in.
<bac> rockstar: well, i'm smart enough to put in traps
<bac> rockstar: thanks for spotting it
<bac> i thought i'd reverted that file...
<rockstar> bac, why did you delete the branches property in lib/lp/registry/model/product.py
<bac> rockstar: it is not part of the IProduct interface and sinzui and i have concluded it is a dangerous leftover
<bac> rockstar: now 'getBranches' should be used
<bac> rockstar: as a code guy please tell us if you think that's wrong for some reason
<sinzui> rockstar: product.branches once cause some problems with snapshot, but that was a year ago
<rockstar> bac, hm, does getBranches take any arguments?
<bac> optional ones
<rockstar> bac, ah, okay.
<sinzui> visibility perhaps?
<bac> yeah, user and status, i think
<rockstar> Usually, I use IBranchNamespace, but that's because I never have to work with IProduct
<bac> rockstar: but i didn't change any call sites.  no one is using that property as best i can tell
<rockstar> I think it's a leftover that can go away.
<bac> rockstar:  and i deployed sinzui's super-grepping prowess on the task too
<sinzui> I think we switched from a property to a method when we introduced private branches
<rockstar> sinzui, yeah, I would assume that privacy probably has something to do with it.
<rockstar> Although if getBranches didn't take any arguments, I'd suggest that it should just be a property.
<rockstar> Since it does, we'll leave it as is and just kill the property.
<rockstar> Since IProduct.branches isn't exposed through the REST API, there doesn't seem to be any noticeable impact.
<sinzui> rockstar: bac: annotate sees this:
<sinzui> Tim Penhey 2009-06-11 16:20:56 +1200
<sinzui> Remove IProduct.branches and replace with IProduct.getBranches inherited from IHasBranches.
 * rockstar nods
<bac> ah, so he just didn't get the other bit
<rockstar> So it looks like thumper missed the first bit.  :)
<thumper> ?
<rockstar> thumper, IProduct.branches still exists, and is being removed in bac's branch.
<rockstar> bac, are you wanting to RC this?
<thumper> ok
<bac> rockstar: no
<rockstar> bac, ok.
<rockstar> bac, r=me
<bac> thanks!
<sinzui> bac: rockstar: since thumper fixed the branches issue, I think we can lower the snapshot shotlist mac
<sinzui> max
<bac> sinzui: how low shall we go?
<rockstar> sinzui, I defer to you on that then.
<bac> 100?
<bac> er, 1000?
 * sinzui looks at annotate
<sinzui> bacL yes, 1000 was the limit
<bac> sinzui, rockstar: ok, i'll change it back
<bac> to 1000
* sinzui changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: -  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> rockstar: I have a branch to review, but my QA instructions are bugus--could not complete them because a edge timeouts. I am now working on the branch to fix the edge timeouts
<rockstar> sinzui, where is your branch?
<sinzui> rockstar: https://code.edge.launchpad.net/~sinzui/launchpad/remove-sp-packaging-bug-272343/+merge/15617
#launchpad-reviews 2009-12-04
<mwhudson> thumper: i'd ask you to review a branch if you could see it :-)
<thumper> mwhudson: if it is small you could pastebin :)
<mwhudson> thumper: http://pastebin.ubuntu.com/334267/
<mwhudson> thumper: its the next layer of the bzr-svn onion
<thumper> mwhudson: I don't suppose you feel like killing updateFromData ?
<mwhudson> thumper: i could give it a go i guess
<thumper> config.codeimport.default_interval_subversion, ?
<thumper> sorry
<thumper> I missread
<thumper> nn
<thumper> nm
<thumper> damn friday fingers
<thumper> mwhudson: line 171 of diff
<thumper> mwhudson: what about using the with statement ?
<thumper> mwhudson: won't it close files on context exit?
<thumper> mwhudson: the for loop at 181... what is the purpose?
<thumper> shouldn't it error if it can't connect at all in the setup?
<mwhudson> thumper: for the former, wow yes, python 2.5 :)
<mwhudson> thumper: it takes a little while for the svnserve process to start accepting connections, that's what the for loop is for
<thumper> mwhudson: my suggestion is that if we don't get acceptance perhaps the setup should fail
<mwhudson> thumper: no, it usually connects the second time around the loop
<mwhudson> oh i see what you mean now
<mwhudson> friday brain :)
<thumper> I know what you mean
<thumper> mwhudson: other than that it looks good
<mwhudson> thumper: thanks!
<thumper> mwhudson: do we have the BZR_SVN enum in prod already?
<thumper> mwhudson: it would be interesting to see what happens on vanilla production branch looking at a bzr_svn import
<mwhudson> thumper: yeah, it's been around for ages
<thumper> mwhudson: as long as it doesn't die in a heap, we should be ok
<mwhudson> thumper: yeah, i'll see if i can do that locally
<mwhudson> thumper: i know +code-import-list will explode
<thumper> mwhudson: ah, why?
<thumper> I don't remember that view
<mwhudson> thumper: getImportDetailsForDisplay
<mwhudson> (another reason we should move to a single url field, i think...)
 * thumper nods
<thumper> mwhudson: movie and pizza time, I'll check back in later :)
<mwhudson> thumper: certainly, don't watch the cricket
* adeuring changed the topic of #launchpad-reviews to:  on-call: adeuring || reviewing: -  || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bigjools: care to review? ;) http://paste.ubuntu.com/334439/
<bigjools> henninge: +1
<henninge> bigjools: as in r=bigjools? ;-)
<bigjools> henninge: r=bigjools :)
<henninge> bigjools: I made it a formal MP to be able to properly request an r-c. Please add your vote there, too. Thanks ;)
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/access-to-translationgroup-table/+merge/15643
<bigjools> sure
<bigjools> henninge: thinking about it, I think I'd be happier with a test
 * henninge was just thinking about that on the loo ...
<bigjools> TMI
<henninge> bigjools: how would I test that ...
<bigjools> henninge: the best test is to upload a package with an attached translation that would need to use the code you changed
<henninge> bigjools: so this happens whenever I store an upload in the librarian?
<henninge> that should not be too hard to simulate
<bigjools> henninge: soyuz calls sourcepackagerelease.attachTranslationFiles()
<bigjools> that's all I know :)
<bigjools> let me see if there's some existing tests you can copy
<henninge> the traceback goes through realiseUpload - publish - publish_ROSETTA_TRANSLATIONS
<henninge> oh, nm, missed the next line 
<bigjools> hmmm it appears there are no tests for uploading translations at all
<bigjools> which is not surprising since this bug would have been caught
<bigjools> henninge: we need a package with translations attached in lib/lp/archiveuploader/tests/data/suite
<bigjools> then add a test in lib/lp/archiveuploader/tests/test_uploadprocessor.py
<bigjools> I am happy to help
<bigjools> since it's probably making your eyes glaze already
<henninge> bigjools: ok, I'll look at it and come back to pester you ... ;)
<BjornT_> henninge, bigjools: uploading a package with translation is good as an integration test, but it's also good to have move specific tests that only call the method in question. that test can be run as a number of different db users.
<bigjools> BjornT_: well right now there are *no* tests that upload packages with translations
<bigjools> I find it hard to see why the translations team should write a test that calls their code using a soyuz db user
<BjornT_> bigjools: right, and i'm not saying that you shouldn't add one :) i just want to avoid you adding too many, so that you don't use integration tests to test edge cases
<henninge> we are all one team ... ;)
<bigjools> yes, I would not suggest adding more than one :)
<BjornT_> bigjools: they should write such tests, if their code is executed in a soyuz db user context
<BjornT_> bigjools: that's a good way of avoiding errors like these
<bigjools> BjornT_: they should, and the test should run in the Soyuz area
<BjornT_> bigjools: what do you mean by 'in the soyuz area'?
<bigjools> i.e. I don;t think the translations tests should know about soyuz specifics
<bigjools> in our test directory
 * henninge is thinking
<BjornT_> bigjools: that's the point, they only have to know about the db user, nothing more
<bigjools> so once we have a test that uploads a translations package, there can be unit tests in soyuz module that call the SPR methods
<BjornT_> bigjools: it depends a bit on which method is tested, of course
<bigjools> yeah I don;t really like that they need to know about it.  Am I being too sensitive about that?
<henninge> bigjools: is it not the same the other way round?
<henninge> I mean, if we add a translations upload test to the soyuz test directory?
<BjornT_> bigjools: but for example, look at lp/bugs/doc/bug-set-status.txt. where should that live?
<bigjools> soyuz uses translations, translations doesn't use soyuz
<BjornT_> bigjools: that's one of tests to make sure that closing bugs via changelogs works
<bigjools> BjornT_: yeah, we have a similar one the in the archiveuploader module
<BjornT_> bigjools: if it's very similar, it's probably redundant
<bigjools> lib/lp/archiveuploader/tests/nascentupload-closing-bugs.txt
<bigjools> and lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt
<bigjools> BjornT_: if you think it's ok for translations to know about soyuz db users then I am fine with it
<bigjools> I just wanted to raise the question
<BjornT_> bigjools: i think it's fine for now. we should find a better way of structuring it, so that it's easier to find all the tests related to a given feature (it's a bit tricky, since one test can belong to multiple features). at the moment either translations have to know a bit about soyuz, or soyuz have to know a bit about translations. which is better?
<bigjools> soyuz already knows a lot about translations :)
<bigjools> it has to deal with the tarballs attached to source packages
<BjornT_> bigjools: maybe it knows too much :)
<bigjools> heh
<BjornT_> bigjools: i just read henninge's mail. that tells me that there probably are tests already for setStatus and addOrUpdateEntry, and those tests should be run using the soyuz db user (as well as the db users it currently runs with)
<bigjools> right
<BjornT_> bigjools: it might make sense to have a tests harness in soyuz land, but the tests should be re-used
<henninge> bigjools, BjornT_: also, that sounds to me like a much quicker way to get a valid test set up for this branch ... ;-)
<bigjools> well I still want a full translations package upload test
<BjornT_> henninge: what bigjools said
<bigjools> henninge: yep
<bigjools> I'd be ok with him landing this with that shorter test, as long as a bug is file to write the bigger test, what do you think BjornT_?
<henninge> bigjools: was about to ask that, too.
<BjornT_> bigjools: which is the shorter test?
<bigjools> BjornT_: re-using existing tests with a new db user
<BjornT_> bigjools: right. i think that's the easiest way of trying to prevent errors like this to happen in the future. i'd be happy with those tests for the first landing
<bigjools> great
<bigjools> makes your life easier for the moment henninge :)
<henninge> cool
 * henninge will do that right after lunch.
<bigjools> mmm food
<noodles775> Hi adeuring, got time for this one?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/487009-db-extract-get-build-records/+merge/15649
<adeuring> noodles775: sure
<noodles775> Thanks!
<adeuring> noodles775: r=me
<noodles775> Thanks adeuring.
<salgado> henninge, that db permission fix is not release-critical, since stub's already done it on the production DB, right?
<sinzui> barry: Your mailing list branch is approved. I had two points you may want to address before landing it
<barry> sinzui: awesome, thanks
<henninge> salgado-lunch: I am not sure how persistent stubs change is and what happens when "something" is restarted or rebuilt etc.
<henninge> something = appserver or dbserver or stub's car ... ;)
<salgado> henninge, indeed; I was thinking of a schema change, and if that was the case it would only be possible to revert it manually.  but since this is actually a permission change, it might be possible to revert it accidentally if someone applies security.cfg to the DB
<james_w> anybody fancy reviewing a lazr.restfulclient branch? https://code.launchpad.net/~james-w/lazr.restfulclient/fix-caching/+merge/15635
<james_w> tiny diff, ugly bug
<jpds> hello.
<noodles775> james_w: adeuring is OCR - so worth pinging him :)
<adeuring> james_w: sure, I'll look
<adeuring> james_w: this looks like a really good bug fix! Just two formal nitpicks: (1) Could you move the last parameter in the changed call on a new line? we try to keep the length <= 78 characters. (2) I think most of your commit message is worth to appear as a comment for the changed method call so that readers know why http_method.upper()  makes sense here?
<james_w> makes sense
<adeuring> james_w: thanks! r=me
<james_w> adeuring: new revision pushed, could you land it for me, I don't have the rights?
<adeuring> james_w: sure, will do.
<james_w> thanks
<bac> hey jpds -- i was going to chat with you about the review but then just submitted it anyway
<henninge> bigjools: I just pushed the branch.
<henninge> bigjools: After a not-so-successful high-level approach (calling addOrUpdateEntryFromTarball), I went for low-level testing on canSetStatus
<henninge> bigjools: it actually produced more problems with access to pofile and potemplate tables, so I included these, too.
<bigjools> henninge: can a translations person review it for you?
<henninge> danilos: ?
<henninge> danilo__: ?
<bigjools> CaptainAmerica to the rescue
<henninge> are you sure he has an alarm on that name?
<henninge> he's also the only one from my team around.
<jpds> bac: Is it just the registered by Mark text I have to edit?
<bac> jpds: yes.  it just needs to match the new output
<bac> jpds: test it with:  bin/test -vvt distributionmirror-views.txt
<sinzui> beuno: I am still trying to reconcile how to handle official verses unofficial packages for projects. look at https://edge.launchpad.net/bzr
<sinzui> ^ beuno, the "Packages in distributions" portlet does not have a version line for the unofficial packages. Should I hide them everywhere or do it make it clear that they are only available via PPA
<sinzui> or some other archive
<sinzui> We really cannot hide them if we do not know the version, because we can only know the Ubuntu versions--we do not track versions in any other distro
<beuno> sinzui, give me 30 and we'll think about it together
<jpds> bac: OK, will work on it when I can time.
<sinzui> beuno: I have a lunch appointment. I will be back in a hour.
<bac> jpds: when you make the changes let me know and i'll update the review and land it for you, if PQM has opened by then.
<jpds> bac: Done.
<bac> jpds: it still doesn't work, though.  You left out "Linux" in the mirror title
<bac> jpds: run:  /bin/test -vvm lp.registry  -t distributionmirror-views.txt
<bac> jpds: oops
<bac> bin/test -vvm lp.registry  -t distributionmirror-views.txt
<bac> (no leading slash, of course)
 * jpds ponders, the only thing I did with "Linux" was remove it from "Official Ubuntu Linux Mirror registered by ...".
<jpds> And it shouldn't be on mirror pages: https://edge.launchpad.net/ubuntu/+mirror/archive.ubuntu.com
<bac> jpds: you did remove the word "Linux" from the expected test output
<bac> "Ubuntu Linux" is the context/title
<bac> jpds: so stuff "Linux" back in there and you will be good
<jpds> bac: But if Linux isn't displayed on the live pages... why is it in the tests?
 * bac looks
<bac> jpds: if you look here you'll see that the text is the same as it appears in the test:  https://launchpad.dev/ubuntu/+mirror/archive-mirror2
<bac> that's because the distro title is "Ubuntu Linux".  the name property is simply "Ubuntu"
<jpds> Oh, right.
<EdwinGrubbs> sinzui: are you still looking for a reviewer, or is the irc topic just stale?
<jpds> bac: Well I've made the change as you've said, can't get ec2test to work nicely though.
<sinzui> EdwinGrubbs: That is stale
* sinzui changed the topic of #launchpad-reviews to:  on-call: adeuring || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to:  on-call: - || reviewing: -  || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jpds: if you've never used ec2 i wouldn't bother now.  there is quite a bit of setup involved.
<bac> jpds: did you run the simple local test i listed?
<jpds> bac: No, I'll work on it later.
<bac> jpds: it's a one-liner. should only take a few seconds
<bac> jpds: nm, i grabbed your new branch and the test passes
<bac> jpds: one last thing.  if you go to https://code.edge.launchpad.net/~jpds/launchpad/fix_196703/+merge/15644 and set the commit message i'll be able to land it for you through ec2.  just include the text of the message, not the review name or any of that stuff.
<jpds> bac: Sorry, was having food, and done.
#launchpad-reviews 2009-12-05
<allenap> []
<allenap> Oops, children.
<allenap> Anyone around want to do a short review?
#launchpad-reviews 2009-12-06
<thumper> mwhudson: could I get you to put an official reviewer stamp on https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model/+merge/15524?
<thumper> mwhudson: last revision has just been pushed, diff should be updated shortly
<mwhudson> thumper: looking
<thumper> mwhudson: diff is updated
<mwhudson> man, a slider to increase context on the diff would sure be nice...
<thumper> mwhudson: heh
<thumper> that would be awesome wouldn't it
<thumper> the guts of it is to use .first() instead of .one()
<thumper> we still try to control that there should only be one
<thumper> but don't die horribly if there isn't
<mwhudson> thumper: done
<thumper> mwhudson: ta
<mwhudson> thumper: can you approve https://code.edge.launchpad.net/~mwhudson/launchpad/code-import-paranoia/+merge/15467 ?
<thumper> mwhudson: done
<mwhudson> thumper: ta
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-attempt2/+merge/15718
<thumper> and
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-browser-integration/+merge/15719
#launchpad-reviews 2010-12-06
* wgrant changed the topic of #launchpad-reviews to: On call: - || Reviewing:  - || queue: [wgrant, wgrant] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> wgrant: those regular expressions for filenames are confusingly named, as they do more than just tell you whether the file is a source file :-/
<wgrant> jelmer: Yeah... I should probably have renamed them when I refactored all that stuff a year ago.
<wgrant> Thanks for the review, anyway.
<jelmer> wgrant: These would probably also be good candidates for migration to python-debian.
<wgrant> jelmer: More than likely.
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing:  - || queue: [wgrant, wgrant] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing:  wgrant || queue: [wgrant] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> wgrant: Hi!
<henninge> wgrant: what's the second review?
<wgrant> henninge: Ah, jelmer already took care of it.
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [wgrant] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> Forgot to take it off the queue, sorry.
<henninge> wgrant: np.
<wgrant> henninge: I was going to replace the doctests, but decided that it might be better in a different branch. You want it in this one?
<henninge> wgrant: Well, instead of adding the corner cases to the doctest, yes, I'd like that.
<wgrant> henninge: OK. Thanks.
<henninge> It's not a big branch yet
<wgrant> No, but I don't generally like mixing code refactors and test replacements.
<bigjools> henninge: can I add this to your queue please: https://code.launchpad.net/~julian-edwards/launchpad/ppa-pockets-bug-684321/+merge/42824
<wgrant> bigjools: Yay.
<bigjools> wgrant: should speed things up  a little, but not as much as the components fix
<wgrant> bigjools: Yeah.
<wgrant> bigjools: Did the components fix make it into the release?
<bigjools> wgrant: just wait for some b******s to ask for components+pockets in PPAs now!
<wgrant> bigjools: I've done both in the past :P
<wgrant> But then I asked for multiple PPAs per person, and that was granted, so I am happy now :)
<bigjools> wgrant: I think it's missed the release :/
<bigjools> shoulda landed it on db-devel....
<wgrant> Hm?
<wgrant> Don't we rollout from devel now?
 * wgrant checks.
<bigjools> wgrant: yes but we pick a revno and release that
<bigjools> stable, not devel
<wgrant> Er, that, yeah.
<wgrant> But the db-stable merge is the latest stable rev.
<wgrant> So everything before that must be included.
 * bigjools looks more
<bigjools> ah yes, so it will be in
<wgrant> The removal of one or both flushes should make things significantly faster too.
<wgrant> But I need to test that on DF next week :)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bigjools || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bigjools: Hi! ;)
<bigjools> wgrant: yes :)
<bigjools> henninge: hi!
<henninge> bigjools: reviewing now
<bigjools> henninge: cherers
<bigjools> cheers, too
<henninge> bigjools:
<henninge> 93	+        # Cast to a list so we don't trip up with the security proxy not
<henninge> 94	+        # understandiung EnumItems
<henninge> Is that a known bug?
<bigjools> henninge: I don't know.  It's easily fixed in lp_sitecustomize though
<bigjools> depends on what the foundations chaps say
<henninge> bigjools: So did you discover this? I am relly just asking if there is a bug for it ... ;)
<bigjools> henninge: well I discovered it but like Australia, there's some debate over whether I was first or not
<henninge> bigjools: ;-)
<bigjools> henninge: if you look in lib/lp_sitecustomize.py you'll see the proxy is removed for a few objects
<bigjools> Deferred being one that I previously added :)
<bigjools> we could add EnumItems there too but I want to check with Gary first
<henninge> yes, that's actually all I was going to ask you to do ... ;) (Talk to gary)
<henninge> thanks
<henninge> bigjools: r=me
<bigjools> henninge: : thanks
* 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 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
* henninge_ changed the topic of #launchpad-reviews to: On call: - || Reviewing: lunch || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: -/- || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> abentley: rc needed https://code.launchpad.net/~thumper/launchpad/makefile-fix/+merge/42810
<thumper> abentley: and morning
<abentley> thumper: :-)
<abentley> thumper: approved.
<thumper> abentley: thanks
#launchpad-reviews 2010-12-07
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: sinzui || 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: benji || 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: bigjools || 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: thumper || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gmb: may I add this MP to your queue: https://code.edge.launchpad.net/~adeuring/launchpad/librarian-filealiases/+merge/42942 ?
<gmb> adeuring: Sure. I'll take it now, actually.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: adeuring || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gmb: great, thanks!
<henninge> thumper: looking at your UI review. A screenshot or two would make this a lot quicker, though.
<henninge> branching now ... ;)
<henninge> gmb: did you add the UI review to thumper's mp?
<henninge> just wondering because he is most likely in bed
<gmb> henninge: I just clicked "request another review"
<henninge> gmb: what I meant ... ;)
<gmb> henninge: Seemed to make sense since he was likely in bed :)
<henninge> gmb: thanks ;)
<gmb> henninge: Ah, yes. For no apparent reason I inserted the word "How" at the start of your sentence.
 * gmb likes to misread questions
 * henninge likes to do that, too ... :-P
<gmb> adeuring: r=me
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gmb: thanks!
<sinzui> gmb, can you review my reply to https://code.launchpad.net/~sinzui/launchpad/faq-mailing-list-permissions-0/+merge/42888
<gmb> Sure
<gmb> sinzui: r=me.
<sinzui> thanks
* mars changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: -,- || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> good afternoon gmb
<gmb> Hi mars
<henninge> gmb: Tim's branch is broken, btw.
<gmb> henninge: Really?
<gmb> henninge: How so?
<gmb> henninge: Ah, I see.
<henninge> gmb: I was not able to submit the form and create a new ppa
* gmb changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> mars: Fancy a short simple code-removal branch? https://code.launchpad.net/~allenap/launchpad/prune-bug-notification-recipients/+merge/42972
<mars> allenap, sure
* mars changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: allenap || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> Thanks :)
<mars> allenap, r=mars
<mars> that is a great change to see
<allenap> mars: Cheers.
* 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-12-08
<jelmer> mars: are you still on call?
* 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> given your time zone I'm going to assume "no", if you're still there please speak up :-)
<bigjools> jelmer: review coming your way in a couple of minutes!
 * jelmer braces for impact
<bigjools> jelmer: https://code.launchpad.net/~julian-edwards/launchpad/delete-packages-api-bug-687298/+merge/43098
<bigjools> waiting for the diff  ...
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> bigjools: r=me
<bigjools> jelmer: lovely, thanks
<jelmer> bigjools: I think we need a policy on what to name web API compatibility functions
<jelmer> I've seen various prefixes: web_, api_, _
<bigjools> jelmer: totally.
<bigjools> api_ seems an obvious choice
<bigjools> jelmer: you could bring it up on the dev list?
<jelmer> bigjools: I was thinking of adding it to the proposed items list for the reviewers meeting on the wiki, do you think emailing the list is better?
<bigjools> jelmer: I think the list is better, I expect it'll get bikeshedded to death
* 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
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-12-09
* 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> Anyone around to review a tiny regression fix?
<wgrant> https://code.launchpad.net/~wgrant/launchpad/bug-522800-getFileByName/+merge/43159
 * mwhudson takes a look
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi jcsackett!
<henninge> I have a branch that is actually quite simple.
<jcsackett> hello, henninge. i would be happy to look at it.
<jcsackett> link?
<henninge> But it contains a sample data change that makes the diff look big.
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/recife-611668-fix-tests/+merge/43236
<henninge> jcsackett: thanks
<jcsackett> henninge: you're welcome.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> henninge: in pofile.txt did you move the bit "msgid = potmsgset.msgid_singular.msgid..." out of the loop just to speed things up, since it appears to be a value that won't change in iteration?
<jcsackett> or was there another purpose?
<henninge> no, just because there was no reason for it to be in the loop
<henninge> It may even speed  up the loop, yes. ;)
<jcsackett> henninge: cool, thanks.
<henninge> The test was failing because of the output from the format string further down.
<henninge> The old format produced trailing spaces but I don't understand why that worked before.
<henninge> jcsackett: I have to run out right now but will be back in about an hour. Sorry.
<jcsackett> henninge: no problems.
<EdwinGrubbs> jcsackett: I've added my branch to the queue: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas/+merge/43116
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: henninge || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> EdwinGrubbs: okay.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> EdwinGrubbs: are you planning on fixing the lint errors that you put in the MP?
<EdwinGrubbs> jcsackett: yes, except for the one where it complains about two blank lines because of a comment.
<jcsackett> EdwinGrubbs: yeah, i was ignoring that one. just confused b/c i generally clean up lint before submitting, but either way works as long as they'll get fixed before merging.
<henninge> jcsackett: Hi
<jcsackett> henninge: hey there.
<henninge> jcsackett: There is a prerequisite branch, as mentioned in the MP
<jcsackett> henninge: yes, i got that. i didn't have failing tests. :-/
<henninge> hm
<EdwinGrubbs> jcsackett: sometimes, reviewers don't like it in the mp diff, because it makes it harder to understand the real changes.
<jcsackett> EdwinGrubbs: fair point.
<jcsackett> henninge: i will look again after reviewing Edwin's branch. there is nothing else but the recife branch, the prereq listed, and yours to consider, correct?
<henninge> jcsackett: actually, there are three other branches before that but I thought they'd be included in the one branch (because it depends on them)
<henninge> jcsackett: the other problem is that the original branches were started against db-devel
<henninge> but now the the merge is into recife.
<henninge> because that was backed out of db-devel again.
<jcsackett> hm.
<jcsackett> henninge: what branch were you looking in that had the errors?
<henninge> jcsackett: db-devel-bug-611668-filtermethods-2 merged into recife
<jcsackett> henninge: okay, that is what i thought i was looking at, but i will rebuild it and double check.
<jcsackett> btw, henninge: where does the name "recife" come from?
<henninge> jcsackett: it's the place of the sprint where this branch was started ... in March :/
<henninge> Recife, Brazil
<jcsackett> henninge: ah. cool.
<jcsackett> EdwinGrubbs: there are a few comments on your MP.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> henninge: taking another look now.
<henninge> jcsackett: I think I know what is going on here
<jcsackett> henninge: hit me.
<henninge> does pofile.txt fail for you?
<henninge> That is the test that was originally failing for me.
<jcsackett> henninge: tell you in a second, building bin/test for your branch now.
<henninge> I fixed that by updating the sample data, as described.
<jcsackett> rather, building bin/test for recife now.
<henninge> After that, the other two tests failed because the ordering of the translationsmessages changed.
<henninge> They were ordered by "date_last_touched"
<henninge> and two had been touched by the migration script.
<henninge> So I "fixed" that by manually setting the "date_last_touched" back to what it was before (in the sampledata) instead of changing the order in the test.
<henninge> So it's correct, the two tests translationsperson.txt and xx-person-activity.txt will pass before and after branch.
<henninge> So, only pofile.txt is being fixed by this branch, the others were just "not broken"...
<EdwinGrubbs> jcsackett: thanks for the review
<jcsackett> henninge: that seems to be the case (coupled with me possibly messing up purity of branches when checking pofile, which didn't fail on recife before).
<jcsackett> but carefully rebuilding the state of the branch before your MP does indeed show pofile failing.
<jcsackett> henninge: r=me; i am mentoring so sinzui will need to follow up. i have already requested a review from him.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jcsackett: thanks ;-)
<jcsackett> henninge: your welcome. sorry i evidently goofed on checking the tests. :-P
<henninge> jcsackett: and I mislead you by implying that these tests were failing.
<bac> jcsackett: time for a quick review?
<jcsackett> bac: sure.
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-662994/+merge/43299
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: bit of a delay; the diff has not yet appeared. :-P
<bac> jcsackett: it is there now
<jcsackett> bac: huzzah.
<jcsackett> bac: are there no unit tests around this the review view?
<bac> jcsackett: i didn't find any.  i held my nose and updated the story
<jcsackett> hrm. i'm tempted to file a bug against the lack of actual unit tests in place of stories...
<bac> jcsackett: i can defer this one and create the unit tests
<jcsackett> bac: i would buy you a beer if you did that. :-)
<bac> ok
<jcsackett> i like the code though. looks like a good branch.
<jcsackett> you want me to still claim the MP, or leave it till later?
<jcsackett> if you want me to go ahead and claim it, i'll happily take a look at it whenever you're done.
<jcsackett> bac ^
<bac> i'll just retract it
<jcsackett> bac: okay. thanks for putting tests in place: you're a gentleman and a scholar.
<bac> bah
* jcsackett 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
<jcsackett> is there anyone available to review: https://code.edge.launchpad.net/~jcsackett/launchpad/membership-status-matters-676494/+merge/43303
<jcsackett> it's a test + a small change to a sql query
#launchpad-reviews 2010-12-10
* 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
<henninge> adeuring: Hi! Are you ready to do a review? ;)
<henninge> It will only be a minute.
<henninge> ... until I have the mp ready
<adeuring> henninge: hrmmm, if it's not too long. Actually, I'm again struggling with Â§$"&Â§&Â§/ private librarian files...
<henninge> adeuring: It's actually a fairly mechanical change, so it should be easy.
<adeuring> henninge: ok, where is the MP?
<henninge> in a minute, still fighting lint ;)
<henninge> adeuring: https://code.launchpad.net/~henninge/launchpad/db-devel-688479-kill-ut-in-tests-1/+merge/43342
 * adeuring is looking
<adeuring> henninge: overall, the changes look good. But the disabled tests conern me a bit: Couldn't you simply use the old factory method makeTranslationMessage() unitl the new method can be properly used in these tests?
<adeuring> s/conern/concern/
<henninge> adeuring: yes, I could. OTHOH that bug will be the next to fix after this bug and I want to remove the old factory method in the next branch for this bug.
<henninge> adeuring: sorry for being afk, btw.
<adeuring> henninge: well, you can fix the factory method first and then change the tests. I am perhaps a bit paranoid, but I remeber a somewhat weird bug in Zope 2 which looked like somebody made a short coffee break while working on a feature but never properly finished that feature aufter the coffee break ;)
<henninge> ;-)
<henninge> The factory method will not be fixed but removed.
<adeuring> henninge: right, what I meant is: I think you should still use the old mehtod until the new one works properly for the currently disabled tests
<henninge> adeuring: we keep running into these hen-egg problems with this branch. ;)
<henninge> In this case it's not the factory that needs to be fixed but the method under test is still referring to the old model.
<henninge> adeuring: but I can do that, if you insist ;-)
<henninge> np
<henninge> I'll have to keep the old factory around a little longer.
<adeuring> henninge: I would feel defeinetly better if all tests stay enabled ;)
<henninge> adeuring: It is important that you feel well!
<henninge> ;-)
<adeuring> ;)
<adeuring> henninge: ok, so, r=me
<henninge> adeuring: thanks
* bac 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
<bac> hi adeuring.  busy today?
* bac 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
<adeuring> bac: well, I must admit that I focused more on understanding the cause of a bad branch I committed yesterday...
<bac> adeuring: a worthy goal
<jcsackett> i've got one to throw on the queue, adeuring and bac: https://code.edge.launchpad.net/~jcsackett/launchpad/membership-status-matters-676494/+merge/43303
* jcsackett changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: i'll take it
<jcsackett> bac: it's the stopgap for the TP stuff.
<jcsackett> thanks, bac.
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,jcsackett || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: your new test runs in 1.27 seconds!  dodged that bullet!
<jcsackett> bac: whoo!
<jcsackett> bac: it ran in 1.13 on my machine. you need something faster. :-)
<bac> hmm
<bac> my poor MBP is getting old
<bac> jcsackett: one suggestion:  ACTIVE_STATES is now defined in two places.  how's about moving that into interfaces/teammembership and fixing the two places that use their own copy?
<jcsackett> bac: good suggestion, i'll do that.
<bac> thanks
<bac> jcsackett: this branch has been referred to as a 'stopgap' but i think these changes needed to be made regardless of what happens in the cleanup TP method
<bac> regardless, r=bac
* bac 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
<jcsackett> bac: i think the long term plan is that the query this branch modifies gets replaces by a single way of cleaning up TP.
<jcsackett> bac, thanks for the review. :-)
* sinzui changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> bac: do you have time to review https://code.launchpad.net/~sinzui/launchpad/closed-teams-1/+merge/43375
<bac> sinzui: of course i do.  i was about to grab some lunch though
<sinzui> fab
* adeuring 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
<adeuring> nice weekend everybody
<bac> hi sinzui
<bac> in your MP, for rule 1 you say
<sinzui> hi bac
<bac> Rule 1 means a closed team cannot become open if it is a member of
<bac>          any team
<sinzui> that is wrong
<bac> ok
<bac> it looks wrong
<bac> any (closed) team?
<sinzui> closed teams cannot become open if they are a member of another closed team
<sinzui> I think I discovered that when writing the test...I should have updated the rule in my notes
<bac> that makes more sense
<bac> thanks
<bac> sinzui: on TeamSubPolicyError were you forced to define 'doc' to get around your problem?
<sinzui> yes
<bac> do you know why it happens?  is that explained in 'Invalid'?
<sinzui> The Invalid class has def: doc(): return self.__doc__
<sinzui> That appears to be insane
<bac> i'm not familar with the doc method.  what should it do?
<sinzui> I understand that in the ideal that the cause of  type of error is obvious, but then why do they all permit use pass a message?
<sinzui> bac I really cannot explain the issue well. I cannot think or a good reason why that method (rather than str()) is used to get the description of the issue in the Web UI
<bac> sinzui: odd
<bac> sinzui: i think this test is redundant and should just be 'else:'
<bac> elif policy != TeamSubscriptionPolicy.OPEN:
<bac> you just checked policy == OPEN
<sinzui> I did see that zope wants unicode, translatable docstrings, so it does funky operations like class.__doc__ = _('my translatable error description')
<sinzui> bac; I do not think that is right. The else clause covers safe transitions such as from moderated to restricted
<bac> sinzui: at line 95 you test that the policy == OPEN.  so the next check that policy != OPEN must always be true
<sinzui> I think I should add something to the elif.
<bac> yeah, you need to check what you are coming from
<bac> i think 108 should be team.subscriptionpolicy !=OPEN
<sinzui> elif policy != TeamSubscriptionPolicy.OPEN and team.policy != TeamSubscriptionPolicy.OPEN
<bac> you can eliminate the first part because it is already known to be true
<sinzui> We only want to check the teams when we are certain that the team is transitioning from closed to open
<bac> 108 should test that it is transitioning to closed from another closed
<sinzui> okay I understand
 * sinzui tries a rewrite
<bac> no, i'm wrong.  108 should test going from OPEN to closed, so the test should be old == OPEN, since we know the new is one is not OPEN
<bac> urgh.
<bac> so i think you have a coding mistake
<bac> why is there not a test case that is broken?
<sinzui> elif team.policy != TeamSubscriptionPolicy.OPEN
<bac> team.subscriptionpolicy
<sinzui> +    elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
<sinzui> ^ that makes the tests happy
<bac> sinzui: but the tests were already happy.  i think you may be missing a test case
<sinzui> I do not think so. Do you want a test for moderated to restricted?
<sinzui> the else that has a pass in it?
<bac> i don't know
<bac> i'm just bummed that you had a logic error but no test failure
<bac> doesn't that indicate a lack of coverage?
<sinzui> okay. I will add a test from moderated to restricted
<sinzui> If I had pdb running in that test, I would have found that I never entered the pass block, I looked at the teams
<bac> yeah, i think it'll be an uninteresting test now but would have shown a failure before.
<sinzui> I want to avoid looking at teams
<bac> you can add the test and then shelve your code fix and see if it fails
<sinzui> it would not have failed, I was just making db checks that I intended to avoid
<bac> yes you are right
<bac> the error just caused you to do checks that are not needed in that case.  there is no way it could fail
<bac> sinzui: r=bac with these changes.
<sinzui> almost finished the test
<sinzui> bac: http://pastebin.ubuntu.com/542016/
<bac> sinzui: looks good
* bac changed the topic of #launchpad-reviews to: On call:  - || Reviewing: -,- || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
