#launchpad-reviews 2009-12-21
<stub> Can I get a review for https://pastebin.canonical.com/25990/ ? It is a minor refactoring to a previously reviewed branch to get all the tests passing - the bulk is shuffling some useful code to a better location.
<mwhudson> stub: doesn't look like the remove argument to kill_by_pidfile is ever used
<stub> mwhudson: It isn't - just there to make it obvious that it gets removed by default and a bit of overengineering for future uses.
<mwhudson> stub: also the docstring seems to lie about it
<stub> I can remove it if you think that is better
<mwhudson> stub: i think it is a bit better yes
<stub> I supposed it is always success since we don't check if the -9 produced a zombie of some sort.
<mwhudson> ok, maybe not a lie, but not super clear
<stub> mwhudson: Remove the parameter entirely or just remove 'on success' from the docstring?
<mwhudson> stub: remove the parameter entirely would get my vote
<stub> def kill_by_pidfile(pidfile_path):
<stub>     """Kill a process identified by the pid stored in a file.
<stub>     The pid file is removed from disk.
<stub>     """
<mwhudson> stub: +1
<stub> (With a blank line between the two lines that the IRC client stripped
<mwhudson> stub: otherwise, it looks fine
<stub> Thanks :)
<mwhudson> apart from the usual "anything that touches lib/canonical/testing/layers.py makes me want to cry a little"
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165)] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> salgado: Hi! Do you have time to land this branch? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994 Thanks!
<salgado> adiroiban, sure, I'll do it in a minute
<al-maisan> hello allenap, I just replied to your review email from last Wednesday (sorry for the delay) -- could you please take a quick look before I land the branch?
<allenap> al-maisan: Sure.
<allenap> al-maisan: I have a team stand-up call now, but I'll look at it straight after that.
<al-maisan> allenap: that's greatly appreciated :)
<allenap> al-maisan: Is it very expensive to calculate changes_file_url?
<al-maisan> allenap: it is somewhat expensive and it is calculated for each SourcePackagePublishingHistory on the LP API -- whether you're interested in it or not.
<allenap> al-maisan: By making it a method, Launchpad must field an extra HTTP request for every SPPH. Maybe that's a bigger overhead? Or do you expect that it will be called rarely?
<al-maisan> allenap: it would seem that it's not a datum of general interest .. there are some LP API apps that use it: e.g. code imports or scripts written by kees
<al-maisan> but my feeling is that it's data that's not necessarily needed whenever we access a SPPH instance
<al-maisan> .. and calculating the 'changes_file_url' property now leads the Soyuz OOPS top-10 by a big margin
<allenap> al-maisan: Okay, sounds like you have a handle on it. It can be changed later if it turns out to be otherwise, so I'm not too worried.
<allenap> al-maisan: Timeout or error?
<al-maisan> allenap: time out
<allenap> al-maisan: Wow.
<al-maisan> yeah
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165),adiroiban(bug-359180)] ||  This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> al-maisan: Okay, one other thing. The test_changesFileLFA method is misnamed; I think it should be test_getChangesFileLFA(). Everything else looks fine.
<al-maisan> allenap: ah, good point. Will fix that.
<al-maisan> allenap: thanks for taking another look at this :)
<allenap> al-maisan: I'll add the conversation as a comment in the mp.
<allenap> al-maisan: Welcome :)
<al-maisan> allenap: great, thanks!
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165),adiroiban(bug-359180),wgrant] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: we don't hear your echo
<henninge> adiroiban: Hi!
<adiroiban> henninge: hi
<henninge> adiroiban: it's time for Christmas goodies: I am finishing your review now! ;-)
<henninge> adiroiban: This is bug 435165
<mup> Bug #435165: Make it easier to navigate to the full list of templates in source packages <trivial> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/435165>
<henninge> adiroiban: what does "request/URL/5" do?
<adiroiban> get the URL up the the fifth segment
<adiroiban> maybe I should replace it with an object URL formater
<henninge> well, relying on the format of the url in this manner is liable to break.
<henninge> so, yeah, using a formatter is the right thing but I have another suggestion.
<adiroiban> al-maisan: Hi. Do you have time to land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375/+merge/16279 ? Thanks!
<henninge> adiroiban: I am also not happy with using more_templates_by_source as a condition. We always test explicitely in Python code (i.e. more_templates_by_source != 0) and I think TAL should not be different.
<henninge> adiroiban: my suggestion to circumvent this is to construct the whole fragment in the view code.
<adiroiban> henninge: we also have  <p tal:condition="context/relatives_by_source" id="potemplate-relatives">
<henninge> adiroiban: true, maybe that is not so bad...
<henninge> adiroiban: Maybe it is just my test that I like to unclutter the tal an create fragments in the view, even call tal formatters from the view.
<adiroiban> henninge: I can create the whole text in the view...as I only need to append "and" 
<al-maisan> adiroiban: can do.
<henninge> adiroiban: as I said, that is what I would do but I acknowledge that it is not the only way tod do it.
<henninge> I leave it up to you, important thing is to get rid of the request/URL/5 and use a fmt:url instead (or canonical_url in view code).
<adiroiban> henninge: for the condition http://paste.ubuntu.com/344078/
<henninge> adiroiban: what I meant would make the TAL look like this http://paste.ubuntu.com/344080/
<adiroiban> henninge: for the fmt:url ... I am not sure how to create it. Is there a url formater for sourcepackage name? (I could not find it) Or I should implement it?
<henninge> and more_templates_string simply returns "and <a href="...">17 more templates</a>"
<adiroiban> henninge: got it. Thanks. I'll implement it that way.
<henninge> adiroiban: or it returns an empty string, so no need for a condition in TAL.
<adiroiban> henninge: yep
<henninge> adiroiban: I think what you are looking for is canonical_url(related_template), right? 
<adiroiban> henninge: I don't know. Please allow me to take a look :)
<henninge> adiroiban: btw, I just demoed that on launchpad.dev by hacking the database and simply moving some of the existing templates to evolution.
<henninge> adiroiban: What do you want to link to? The logical place would be +templates, I guess.
<adiroiban> henninge: ok. I think there is also a pagetest for that.
<henninge> adiroiban: I know, I saw it!
<adiroiban> henninge: https://translations.edge.launchpad.net/ubuntu/karmic/+source/kdelibs/+translations
<henninge> but to try it out locally i your branch I need a product with more than 5 templtes.
<henninge> adiroiban: doesn't work that way for a product it looks
<henninge> adiroiban: https://translations.edge.launchpad.net/wiserearth/trunk/+translations
<henninge> danilos: do you know why +translations works differntly on products and packages?
<adiroiban> henninge: rebuilding schema. I'm taking a look now
<danilos> henninge, because we didn't have time to make sourcepackages use the same approach (i.e. group by languages, not by templates)
<henninge> bad news
<henninge> so we don't have a place to link to that lists are templates for a source package, once +translations for source packages works like products.
<henninge> s/are/all/
<al-maisan> adiroiban: your branch (lp:~adiroiban/launchpad/bug-492375) is running in an ec2 instance now and will be submitted to pqm (in approx. 4 hours) if all goes well.
<adiroiban> al-maisan: thanks!
<al-maisan> you are welcome 
<adiroiban> henninge: I will look to see what can be done fro productseries
<henninge> adiroiban: it looks like the best bet is to link to +templates for productseries and +trantlations for packages atm.
<henninge> but this will need to be fixed in another branch.
<danilos> henninge, adiroiban: we should use the same views/pages for +templates and +translations on all of productseries, sourcepackage, distroseries, project (with focused series being shown for each product)
<danilos> henninge, adiroiban: do not do that right away though :)
<henninge> what I meant.
<henninge> ;)
<adiroiban> henninge: what needs to be fixed in another branch?
<adiroiban> danilos: ok. just give me the bug number 
<adiroiban> just kidding :)
<adiroiban> but still, it would be nice to have a bug ;)
<danilos> adiroiban, it's not a bug per se, and it'd involve cleaning up a few interfaces/implementations first :)
<adiroiban> henninge: I don't know what you mean with canonical_url(related_template) . Since I'm in a potemplate I assume I should look the context for self.context.distroseries
<henninge> adiroiban: yes, taht was wrong
<henninge> adiroiban: It will be canonical_url(series)+"/+translations", I guess.
<henninge> or canonical_url(context) ?
<adiroiban> henninge: nope
<adiroiban> context is the potemplate
<adiroiban> context.distroseries is ubuntu/hoary
<adiroiban> i need the sourcepackage name
<adiroiban> maybe I need to create a url formater
<adiroiban> as I could not find one
<henninge> potemplate also has a sourcepackagename, doesn't it?
<henninge> adiroiban: but don't do that for this branch. Just create the html in view.
<henninge> 'and <a href="%s">%d more templates</a>' % (url, num_of_templates)
<henninge> constructing the url string is the hard part.
<henninge> adiroiban: you will have to check whether the template is from a product series or from a sourcepackage and construct the url accordingly.
<adiroiban> henninge: yep. but there is no sourcepackagename url formater
<henninge> adiroiban: I need to go now, I am sorry.
<adiroiban> henninge: np. I should be able to handle it from now on
<adiroiban> thanks
<henninge> adiroiban: yes, you will need to create the url in your code.
<henninge> good luck, I will look at it tomorrow.
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165),adiroiban(bug-359180),wgrant] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-359180),wgrant] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: Hi. Do you have time to land this branch? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-496352-series-status-refactor/+merge/16108 Thanks!
<abentley> adiroiban: Has it passed a full test suite run?
<adiroiban> abentley: I don't know. I have just started a new one now.
<abentley> adiroiban: Okay, let me know when it passes, and I'll land it.
<adiroiban> abentley: sorry for that. I've only tested with -m translations, and for my other branches the reviewers have started the full test.
<adiroiban> abentley: do I need to merge with devel ?
<abentley> adiroiban: It is a good idea to merge with devel, but not strictly required.
<sinzui> danilos: bac: Can either of your review https://code.edge.launchpad.net/~sinzui/launchpad/webkit-css/+merge/16388
<bac> sinzui: i can.  is abentley unavailable?
<bac> or is his queue just too long?  anyway, i'll grab it now.
<sinzui> bac: I think you would appreciate a layout fix for safari more than most people.
<bac> sinzui: i should point out i never *really* saw the issue last week.  the problems i saw on staging were fixed when they did a 'make clean'
<sinzui> bac: lpnet and edge are very broken in epiphany. The the sidebar is rarely on the side. Most of the time it is below the content
<abentley> adiroiban: was there a pre-implementation discussion for your bug-497438 branch?
<adiroiban> abentley: yes.
<bac> sinzui: can you paste an URL that is currently broken on production in epiphany so i can see the problem?
<sinzui> bac:
<sinzui> https://edge.launchpad.net/launchpad-registry/+milestone/3.1.13
<sinzui> https://edge.launchpad.net/launchpad-registry
<sinzui> https://edge.launchpad.net/~bac
<bac> sinzui: ah, i see it if i have a super-wide browser
<sinzui> bac: I have a narrow browser
<abentley> adiroiban: It's really helpful to mention it.  There's supposed to be a section in your cover letter between "Proposed fix" and "Implementation details" called "Pre-implementation notes".
<abentley> adiroiban: https://dev.launchpad.net/QuickCoverLetterTemplate
<adiroiban> abentley: sorry for not including that part. The preimplementation chat was with Danilo and the summary is part of Implementation details
<abentley> adiroiban: In lib/canonical/launchpad/security.py, you have "Disribution" rather than "Distribution" twice in the docstrings.
<adiroiban> abentley: thanks. fixed.
<abentley> adiroiban: UTC always makes me think of Universal Coordinated Time.  Is there another name you could use for utc_browser?
<adiroiban> dtc ?
<adiroiban> distribution translations coordinator
<abentley> adiroiban: works for me.
<adiroiban> abentley: ok. I'll rename it
<abentley> adiroiban: Thanks.  I think that will save confusion in the future.
* jelmer_ changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-359180),wgrant,jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> BjornT_, since you're living Windmill these days, could I get a Windmill fixer-upper review? :)
<abentley> adiroiban: It seems like you create utg_member in order to specify their email and password, so that you can pass known values to setupBrowser.  Is that correct?
<adiroiban> abentley: yes
<BjornT_> deryck: maybe :)
<deryck> BjornT_, heh.  It's a small fix.  Trying to make inline subscriber test less fragile.
<abentley> adiroiban: I think it would be useful to provide an alternative to setupBrowser that accepted an arbitrary Person, but what you've done is also fine.
<BjornT_> deryck: sure
<deryck> BjornT_, https://code.edge.launchpad.net/~deryck/launchpad/fix-subscriber-windmill-fragility-497112/+merge/16431
<deryck> BjornT_, thanks!
<adiroiban> abentley: now that it is called DTC, it would have make some sense to pass the distribution as argument
<BjornT_> deryck: what does css_name look like? is it unique for each subscription?
<deryck> BjornT_, it's the form "subscriber-XXX" where XXX is the db id for the subscribed user.
<deryck> BjornT_, so yeah, should be unique.
<BjornT_> deryck: ok, approved
<deryck> BjornT_, thanks!
<abentley> adiroiban: (didn't notice I was disconnected) Why are you assigning to ubuntu.translationgroup rather than making utg_member a member of it?
<adiroiban> abentley: now that it is called DTC, it would have make some sense to pass the distribution as argument
<adiroiban> abentley: I don't know if ubuntu.translationgroup is already created
<abentley> adiroiban: Sure, that would be fine.  But this function has side-effects that aren't apparent from the name.  Especially, calling it twice will make the first return value invalid.
<adiroiban> abentley: true. I also tried joining utg_member to translationgroup
<adiroiban> but I got an Attribute now allowed
<adiroiban> or something like that
<adiroiban> I also tried to login(ubuntu.translationsgroup.owner)
<adiroiban> and then add the new member
<adiroiban> but with no luck
<abentley> adiroiban: Okay, I'm glad you explored other options.  I think this is acceptable in this case.
<adiroiban> abentley: I think I need to look for a implementation that will also work for calling that function twice
<abentley> adiroiban: I think that would be an improvement, but I can't think of a use case for calling it twice.
<abentley> adiroiban: If you get the "Attribute not allowed" message again, I can help you look into it.  It's important to distinguish between "forbidden" (never allowed) and "unauthorized" (not allowed in this context).
<adiroiban> abentley: OK. Thanks!
<abentley> adiroiban: I'm going to summarize our discussion and change it from "needs review" to "work in progress" for now.
<adiroiban> abentley: sure.
<abentley> adiroiban: Since I assume you're already working on stuff, I'll skip your other review for now.
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: wgrant || queue [adiroiban(bug-359180),jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: there is no hurry :)
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: || queue [adiroiban(bug-359180),jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: can you please take a look at this implementation http://paste.ubuntu.com/344161/ ? Thanks!
<abentley> adiroiban: Sorry, on phone.  With you shortly.
<abentley> adiroiban: What would raise NameAlreadyTaken?
<adiroiban> abentley: calling the function twice
<adiroiban> abentley: here is the code that also accepts a person http://paste.ubuntu.com/344170/
<adiroiban> another person
<salgado> adiroiban, did you get a success/error message for that branch you asked me to land?
<adiroiban> salgado: success :)
<salgado> adiroiban, but it didn't land on devel, did it?
<adiroiban> salgado: nope, as it was not a RC
<abentley> adiroiban: calling which function twice?
<salgado> adiroiban, but pqm is open
<adiroiban> salgado: it was closed last week
<adiroiban> abentley: makePerson
<adiroiban> abentley: or setupDTCBrowser
<salgado> adiroiban, oh, I meant to ask if you got a success email today.  I've submitted your branch in the morning and it's not on ec2 anymore
<adiroiban> salgado: ah... no email
<abentley> adiroiban: AFACT, calling setupDTCBrowser will not raise NameAlreadyTaken, because you don't re-raise it in the except block.
<adiroiban> abentley: yes. I wanted to say that makePerson from setupDTCBrowser
<adiroiban> can raise that exception
<al-maisan> adiroiban: your lp:~adiroiban/launchpad/bug-492375 branch fails a test, please see: http://pastebin.ubuntu.com/344177/
<abentley> adiroiban: Your code is still unsafe if you call it twice with an explicit Person, and if you call it with an explicit person, it doesn't use that person to set up the browser.
<adiroiban> abentley: true
<abentley> adiroiban: So either we need to make the handling of person more robust, or we should remove it.
<abentley> adiroiban: I am happy either way.
<adiroiban> the browser is not setup. But why it will fail when called twice?
<abentley> adiroiban: If there is an explicit person, you will reassign ubuntu.translationgroup unconditionally.
<adiroiban> abentley: yes. I'm creating a new translationgroup having that person as an owner
<abentley> adiroiban: That will make the return value from the previous call unusable.
<adiroiban> abentley: then I can so something like this http://paste.ubuntu.com/344183/
<abentley> adiroiban: That would solve the double-call issue.  I would prefer a try/except/else, like this: http://paste.ubuntu.com/344186/
<abentley> adiroiban: This ensures that you won't catch NameAlreadyTaken from other methods and assume they came from makePerson.
<adiroiban> abentley: yes. thanks
<abentley> adiroiban: With that change, I'd be happy to approve the review.
<abentley> salgado: Thanks!
<abentley> adiroiban: Going offline for a sec.
<adiroiban> al-maisan: do you know how I can call that failing test?
<al-maisan> just a minute
<adiroiban> al-maisan: I tried with bin/test -m translations and everhing was ok.
<adiroiban> also bin/test -t rosetta-potemplates is not starting that test
<adiroiban> same for bin/test -t -t "stories/translations/.*"
<al-maisan> adiroiban: please try "bin/test -vv -t stories/translations"
<adiroiban> al-maisan: that did the trick. Thanks!
<al-maisan> adiroiban: you are welcome 
<adiroiban> abentley: I've just pushed the new code
<abentley> adiroiban: r=abentley
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: adiroiban || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> beuno: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325
<EdwinGrubbs> beuno: The last comment in the mp contains a link to screenshots.
<beuno> EdwinGrubbs, I can
<beuno> EdwinGrubbs, I need to grab lunch or I will die
<beuno> is it ok to get back to you in ~1 hour?
<beuno> also, is there a spinner and such?
<EdwinGrubbs> beuno: yes, there is a spinner. I can probably put together a video if that would be helpful.
<beuno> EdwinGrubbs, I trust you that it works as everything else
<beuno> be back in an hour!
<beuno> EdwinGrubbs, hi
<beuno> still around?
<EdwinGrubbs> beuno: hello
<beuno> EdwinGrubbs, I see you've introduced an informational message in the overlay
<beuno> or is that when you try to add somebody who's already been added?
<EdwinGrubbs> beuno: yes, salgado didn't like me using the error message overlay to let the user know that someone has already been added.
<beuno> EdwinGrubbs, could we go a step further and not let you add people who are already part of the team?
<beuno> have them grayed out on the results?
<EdwinGrubbs> beuno: that is a much bigger project, since it will involve changing the way we extract data from the vocabulary as JSON, and we need to have a standard way of specifying what values we are filtering.
<beuno> EdwinGrubbs, gotcha. Could we file a bug for that?  I think that's the right way to go
<EdwinGrubbs> beuno: yes, I'll file one
<beuno> EdwinGrubbs, thanks. I have a few comments on the layout of the "dialog", I'll add them to the MP
<beuno> that's all really, hope it's a quick change
<EdwinGrubbs> beuno: thanks
<beuno> EdwinGrubbs, we'll need to make foundations aware of what we want to change in the vocabularies, but I guess they'll learn about it when they triage the bug  :)
<beuno> EdwinGrubbs, done
<abentley> adiroiban: If tabindex_chain don't affect tabbing, can you call it something else?
<adiroiban> abentley: fields_chain?
<abentley> adiroiban: Maybe something like navigation_order?
<adiroiban> abentley: sure
<adiroiban> or translations_order?
<abentley> adiroiban: Sure.
<abentley> adiroiban: On line 344-345 of the patch, shouldn't you be deleting a semi-colon, not adding a space?
<abentley> in lib/lp/translations/templates/translations-macros.pt, why is the whole thing conditional on autofocus_html_id?
<adiroiban> abentley: line 344 should be left untouched. the two semicolon are valid as the first one is excaping the javascript semicolon
<abentley> adiroiban: Okay.
<adiroiban> abentley: autofocus is returning the first field and if we dont't have any field there is no need  for that code
<abentley> adiroiban: It looks like Y.lp.pofile.initializeKeyBindings is being called unconditionally and is assuming tabindex is set.  Am I reading it wrong?
<adiroiban> abentley: nope.
<abentley> So that would cause javascript errors if autofocus_html_id is unset?
<adiroiban> abentley: yes
<adiroiban> abentley: i will add empty variables
<abentley> adiroiban: Sounds good.
<EdwinGrubbs> beuno: the error message is currently shown only after someone clicks on one of the results, which hides the picker. Are you saying in the review that you want me to re-show the picker so that I can display the error message in that?
<beuno> EdwinGrubbs, well, you are re-showing something, right?
<beuno> we can do one or the other
<beuno> either make it look more like a dialog
<beuno> or do it within the picker itself
<beuno> we should give a user a way out other than clicking on the small x  :)
<EdwinGrubbs> beuno: I'll just add the OK button to dismiss the current dialog.
<beuno> EdwinGrubbs, but that doesn't let the user recover, no?
<EdwinGrubbs> beuno: they would only want to recover if they are adding multiple people as members. If they try to add multiple users quickly, we might end up in the situation where they start typing in the new search text, and the error message shows up in the picker, even though the error is related to the last usage of it.
<beuno> EdwinGrubbs, good point
<EdwinGrubbs> rockstar: ping
<rockstar> EdwinGrubbs, I'm technically on holiday right now, but would be happy to oblige small requests.
<rockstar> (I got Warcraft III working in Wine last night, so am currently doing nothing useful)
<EdwinGrubbs> rockstar: that's awesome. I'll try to find someone else to review a windmill test first.
<rockstar> EdwinGrubbs, okay.  If you don't find anyone, I can take it.
<EdwinGrubbs> deryck: ping
<EdwinGrubbs> jtv, mars: ping
<jtv> EdwinGrubbs: zzz
<EdwinGrubbs> jtv: sorry, go back to sleep. It's all just a bad dream.
<jtv> <snore>
<wgrant> abentley: Can you please land that branch for me?
<abentley> wgrant: Has it passed the whole test suite?
<wgrant> abentley: Not for two and a half months, but it looks like it should still pass, and I thought you normally ec2-landed things.
<abentley> wgrant: I don't ever ec2-land things.
<deryck> EdwinGrubbs, hi.  I really need to EOD for kids being home.  Was on phone.  Can we take it up tomorrow?
<wgrant> abentley: Ah. I guess I'll ec2test it myself.
<EdwinGrubbs> deryck: I'll find someone else to do a short review.
<abentley> wgrant: When it passes, ping me and I'll be happy to land it.
<deryck> EdwinGrubbs, ah, ok.  Sorry I couldn't help.
<EdwinGrubbs> salgado-afk, beuno: I have updated the mp. Can you re-review it?
<EdwinGrubbs> sinzui: do you feel comfortable reviewing a windmill test, since salgado passed on that?
<sinzui> yes
<EdwinGrubbs> sinzui: here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325
<beuno> EdwinGrubbs, I can. I'm bringing in your branch to play with it and fully sign off
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<beuno> EdwinGrubbs, still around?
<beuno> when trying to add someone to the spanish team
<beuno> I get:
<beuno> The following errors were encountered:
<beuno>     * Unexpected status: foo
<beuno> (and it got added)
<EdwinGrubbs> beuno: I left in some debugging code on accident.
<EdwinGrubbs> beuno: it's a one line fix. I can push it if you want.
<beuno> EdwinGrubbs, no, it's ok
<beuno> EdwinGrubbs, in general, I think we show the persons' name on the list when saving
<beuno> grayed out
<beuno> and flash that green/red
<beuno> no?
<beuno> like subscribing to bugs
<beuno> also, the alignemnt on the overlay is a bit wonky
 * EdwinGrubbs is checking
<beuno> the text could ideally be top-aligned with the info icon
<EdwinGrubbs> beuno: I had forgotton about the grayed out name and spinner next to the name. I can look into that.
<beuno> EdwinGrubbs, thanks. Sorry I didn't catch that earlier, I should of branched from the start
<beuno> EdwinGrubbs, commented exactly this on the MP
<beuno> and now, I'm off to buy my brother a last minute bday gifty
<beuno> *gift
#launchpad-reviews 2009-12-22
<mwhudson> anyone here?
<mwhudson> jml: maybe you?
<mwhudson> rockstar even?
<rockstar> mwhudson, hello sir.
<mwhudson> rockstar: can you do a trivial review?
<mwhudson> i know you're not really here...
<rockstar> mwhudson, sure, send it on over.
<mwhudson> heh, it's https://bugs.edge.launchpad.net/bugs/392155 reappearing 100k bugs later
<mup> Bug #392155: make run_all is broken <Launchpad Foundations:Fix Released by jml> <https://launchpad.net/bugs/392155>
<mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/unshag-make-run_all/+merge/16461
<mwhudson> man there's a lot of typing to do for a trivial fix :(
<rockstar> mwhudson, r=me
<mwhudson> rockstar: ta
<jml> mwhudson, I'm back.
<mwhudson> jml: hello
<mwhudson> jml: could you maybe look at https://code.edge.launchpad.net/~mwhudson/launchpad/log-noop-puller-runs/+merge/16460 ?
<mwhudson> jml: it involves gathering data, you'll like it 
<mwhudson> :-p
 * mwhudson .oO(maybe i need to graph it too for full jml baiting...)
<jml> hahaha
<jml> mwhudson, any ideas for a graph?
<mwhudson> jml: dunno
<mwhudson> jml: puller runs per day by type?
<mwhudson> branch type i mean
<mwhudson> would be easier after i finish off that branch that schedules puller runs with the job table
<mwhudson> sadly the last time i looked at that branch i started throwing things around the room
<jml> mwhudson, well, you're moving out soon right?
<mwhudson> :)
<mwhudson> jml: i think i need to abandon my attempt to be minimal and actually think about it again
<mwhudson> jml: could i ask you to ec2 land https://code.edge.launchpad.net/~mwhudson/launchpad/unshag-make-run_all/+merge/16461 ?
<mwhudson> i would, but i need to pack up for a bit here
<jml> mwhudson, sure.
<mwhudson> ta
<mwhudson> mind you
<mwhudson> maybe ec2 land is excessive for that branch
<jml> mwhudson, do you want to just pqm-submit it?
<mwhudson> jml: yeah ok
 * mwhudson uses ec2 land to figure out the commit message
<jml> mwhudson, that's good. because I am late for a thing.
<mwhudson> jml: :)
<mwhudson> jml: enjoy your thing
<noodles775> mwhudson: can you r or rs a testfix reversion of r10066 (which enabled inclusion of windmill tests by default) http://pastebin.ubuntu.com/344576/
<noodles775> ^^^ or anyone else who is keen.
<mwhudson> noodles775: did you consider disabling the memcached layer tests?
<mwhudson> as it seems to be some interaction there?
<mwhudson> otoh, this is easier i guess
<noodles775> mwhudson: I mentioned in my email,...
<mwhudson> noodles775: ah okay
<mwhudson> noodles775: r=me then
<noodles775> safer... (we can disable the memcached tests and reenable these in another branch that goes through ec2)
<noodles775> thanks
<henninge> adiroiban: the final diff looks good. Let's land this!
<henninge> adiroiban: Your last devel merge has been a few days. Can you please do that once more? I will land the branch then.
<henninge> adiroiban: Thanks for your work.
<adiroiban> henninge: sure
<adiroiban> henninge: do I need any special setup for running soyuz or packagearchiver tests?
<henninge> adiroiban: not that I know of. They are part of the normal test suite AFAIK
<adiroiban> i have started a full test for LP and some tests get stuck
<henninge> shouldn't. Hm...
<henninge> adiroiban: some tests take a bit longer, I think.
<adiroiban> well for ex this one was running for 8 hours lp.archiveuploader.tests.test_ppauploadprocessor.TestPPAUploadProcessor.testPPAUploadResultingInNoBuilds
<adiroiban> henninge: sorry for the delay. got some problem with the new css stuff. The merged branch should be pushed
<henninge> adiroiban: I just realised: the dot "." in "and <a href=\"%s\">%d other templates</a>." should probably be placed in the template so that it appears at the end of the sentence even if this part is empty.
<henninge> adiroiban: cool
<henninge> np
<adiroiban> henninge: that was my first implementation... but this will brake some tests
<henninge> ;)
<henninge> never mind, then.
<adiroiban> in the previous implementation there was no dot
<henninge> which I'd consider a bug since this is a full English sentence. But just a very small one :-)
<adiroiban> henninge: let me fix it
<adiroiban> and change the tests
<henninge> adiroiban: if you wish. The branch is approved for landing as it is ... ;)
<henninge> thanks a lot!
<adiroiban> well, I will fix it...as it's now or never
<jtv1> oh, hi henninge, hi adiroiban!
<henninge> Hi jtv1, 2, and 3 ... ;-)
<henninge> jtv: had enough of cloning?
<jtv> henninge: yes, what's the fun in reproducing if you have to do it this way?
<henninge> :)
<jtv> Nobody on call today?
<jtv> I just fixed a regression to my own fix... the translations branch approver was still missing a permission.
<adiroiban> jtv: hi
<adiroiban> :)
<henninge> jtv: are you saying it's a small branch?
<jtv> adiroiban: not gone for the holidays yet?  Or will you be here even more during the holidays?  :-)
<jtv> henninge: yes
<jtv> henninge: https://code.edge.launchpad.net/~jtv/launchpad/son-of-bug-487447/+merge/16480
<jtv> almost all test change
<jtv> and most of the diff is adding 1 line of code to each test in the test case.
<jtv> (lots of context lines)
<henninge> jtv: r=me
<jtv> henninge: kuhl, danke!
<henninge> lol
<stub> Pretty trivial review at https://code.edge.launchpad.net/~stub/launchpad/librarian-report/+merge/16463, maybe for jtv.
<jtv> stub: having a look...
<jtv> stub: why use Popen instead of our nice run_script wrapper?
<stub> We have a nice run_script wrapper? This time I'm just cargo culting what I landed last week. Last week I didn't know about run_script nor if it is suitable in this case.
<stub> (this test is cut & paste the preceding test, with a few different arguments)
<jtv> stub: and shame, shame, SHAME on you (or whoever is responsible) for using slashes in dates...
<stub> jtv: Anything PG will parse unambiguously will work - you might notice the lack of validation...
<stub> I think - works too
<stub> For the slash deficient amongst us
<jtv> <slash?  I'll give you a slash!  hah!>
<stub> I think we only hire losas in locales which use / though so it is fine.
<adiroiban> jtv: yep. still here to annoy Henning  with my branches
 * jtv bares teeth at stub
<jtv> adiroiban: annoy?  not very likely :)
<adiroiban> :)
<jtv> stub: slashes in dates are a symptom of inbreeding
 * stub goes to take a slash
<henninge> adiroiban: cool dot! ;-)
<henninge> adiroiban: I'll initiate the landing ...
<henninge> forward thrusters ... check ... landing gear ... check ... parachutes .... ch...check!
<henninge> adiroiban: we're in descent
<jtv> stub: I'm also slightly curious what happened to the line of test output right above your real change... did you just pick a different line of the output to compare against?
<stub> jtv: Yes. I wanted the tests to demonstrate the output changes with the different command line arguments.
<jtv> stub: makes sense
<jtv> stub: afaict you could get rid of all existing imports and most of the code in this test if you use run_script
<stub> k
<stub> One import - I still have to assemble the patch etc.
<adiroiban> henninge: successfully landed. Thanks!
<adiroiban> do you think you can land this branch ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-497438/+merge/16335
<adiroiban> it fixed the lucid translation not found problem
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> good morning
<noodles775> hi bac, I'm just heading out for an hour, but I've got a lazr-js branch ready for review if you've time later:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/lazr-js/491337-error-style-try-2/+merge/16423
<bac> ok, noodles775.  i'll have a look
<noodles775> ta 
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [jelmer, noodlesXXX] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adiroiban: the branch did not land yet, there must be some misunderstanding.
<adiroiban> henninge: ah. I got an email from buildbot 
<adiroiban> maybe is for another branch...
<henninge> adiroiban: yes, must be.
<adiroiban> as I can not access the link from that email
<henninge> adiroiban: what's the link
<henninge> ?
<adiroiban> https://lpbuildbot.canonical.com/builders/lp/builds/445
<henninge> adiroiban: http://paste.ubuntu.com/344718/
<henninge> that's what's landed
<adiroiban> thanks
<jtv> stub: any news on the branch?
<stub> News?
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jelmer || queue [noodlesXXX] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adiroiban: can you set a commit message on bug-497438, please?
<stub> jtv: What am I doing to what branch?
<jtv> stub: I thought you were trying out run_script, hence my silence
<stub> jtv: No, I was waiting on your review. I can change it to run_script easily enough - no real gain though on clarity or line count that I can see but whatever.
<adiroiban> henninge: done
<jtv> stub: oic... you meant "I still have to assemble the path" but wrote "I still have to assemble the patch."  What a difference a letter makes!
<stub> oops
<jtv> what I was getting at was that you don't have to compose the path any more
<jtv> we do seem to have a bunch of run_script definitions though
<jtv> stub: you'd do (retval, out, err) = run_script('scripts/', ['--from=2005-01-01', '--until=2005-12-31']) and that's all it takes to run the script
<stub> Ok - so don't need the absolute path.
<jtv> ahem, 'scripts/librarian-report.py' that is
<jtv> right
<stub> sounds like good change then.
<jtv> 's what I thought too :)
<bac> hi jelmer
<bac> hi jelmer_
 * bac tries all permutations
<jelmer> hi bac
<henninge> adiroiban: it's landing
<bac> hi jelmer -- was going to talk to you about interactive testing of your branch with launchpadlib but i just included the comments in the review.  let me know if you have questions.
<jelmer> bac: will do - thanks for the review!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: noodles || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: on call: bac || reviewing: noodles || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> hi bac.  can you do a trivial review for me?
<bac> salgado: sure
 * salgado suspects his m-p submission failed.  it's been almost 20 minutes since he sent it and lp doesn't seem to have done anything with it
<salgado> bac: https://code.edge.launchpad.net/~salgado/launchpad/bug-422552/+merge/16483
<adiroiban> al-maisan: Hi! Do you have time for landing this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375/+merge/16279 ?  Thanks! I've done a new full test and it was ok. 
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: salgado || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi salgado
<salgado> hi bac
<bac> salgado: what if you just did request.split() instead of request.split(' ')?  the former seems to handle mutliple space characters
<salgado> does it?
<bac> salgado: and doesn't require normalizing
<bac> >>> 'a b   c'.split()
<bac> ['a', 'b', 'c']
<bac> >>> 'a b   c'.split(' ')
<bac> ['a', 'b', '', '', 'c']
<salgado> If sep
<salgado>     is not specified or is None, any whitespace string is a separator.
<salgado> yeah, looks good to me
<bac> salgado: but i'm glad we have a test now for the case
<salgado> indeed
<bac> salgado: r=bac
* 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
<salgado> bac, do you think a comment would be desirable explaining why we use split() instead of split(' ')?  (I don't think it would)
<bac> salgado: nah, since it DTRT i don't think it adds anything
<salgado> cool
<salgado> thanks bac!
<noodles775> Thanks for the review bac (I dropped off earlier, so sorry if I missed any questions)
<bac> noodles775: nope.  just a fleeting moment of confusion when i realized i hadn't run 'make build' in the branch and that's why it did nothing.
<noodles775> :)
<EdwinGrubbs> beuno: do you have any thoughts on how I should resolve the issue I commented on in the mp? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325
<beuno> EdwinGrubbs, hey
<beuno> let me actually read your comment
<beuno> hrm
<beuno> I'm torn here
<EdwinGrubbs> yep, me too
<beuno> we need a good mechanism for this type of behavior
<beuno> this branch is probably not the time to do it
<beuno> EdwinGrubbs, how about this
<beuno> lets land this branch
<beuno> and you brainstorm a bit on the list how to solve this problem in the future, in a more general way
<EdwinGrubbs> beuno: sounds good to me
<beuno> EdwinGrubbs, cool, I'll make it official
<EdwinGrubbs> thanks
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> bac: Thank you for the review. You saved my bacon :) I'll go and fix it now.
<bac> ummm, bacon
<bac> allenap: i think gmb gets credit for all of those great XXX comments
<allenap> bac: Yes :) And I get idiot points for not using grep!
<allenap> bac: I'm still working on fixing those other XXXs, but it's getting late here, so I'll finish it tomorrow.
#launchpad-reviews 2009-12-23
<stub> bac: https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/16486 (cherry pick candidate addressing critical bug)
<jtv> hi bac!  is that topic line still current?
<mwhudson> i doubt it
<mwhudson> stub: how did that librarian-gc bug happen?
<mwhudson> jtv: i can do a review for you if you want
<jtv> mwhudson: very kind, but only asking
<jtv> don't need anything reviewed
<stub> It died with abel's recent refactoring (dropping LibraryFileContent.deleted, replacing with a NULLable LibraryFileAlias.content)
<mwhudson> stub: ah ok
<stub> Previously we just set the LibraryFIleContent.deleted for expired files if there are no other unexpired references to that content. That code was removed, and not replaced.
<mwhudson> stub: would it be possible to write that query using storm's query builder?
<mwhudson> stub: would it be possible to write that query using storm's query builder?
<mwhudson> stub: also, i think a comment in the tests about the timescale of "recent" would be nice
<stub> Yes, but I doubt it would be more readable and the rest of the file uses raw SQL
<mwhudson> ok
<stub> I didn't want to infect the tests with definitions for recent etc. because that can change
<mwhudson> well
<mwhudson> when i read the test i had no idea if a day ago was recent
<stub> I suspect we will be turning it down from 1 week to 1 or 2 days shortly based on elmo's comments maintaining debian archives
<stub> ok
<mwhudson> i guess it would be better if both the code and the tests keyed off a config value
<stub> I'll add comments to that effect (1 day old is certainly recent, 30 days is certainly well expired, and values in between depend on the current settings in the garbage collector but are not really that important)
<mwhudson> stub: that would be great, thanks
<stub> Yer - nobody ever gets around to adding it as a config value ;)
<stub> This is a cherry pick so I have an excuse this time ;)
<mwhudson> well, there's no time like the present!
<mwhudson> heh, darn, foiled
 * mwhudson reviews in the webapp
<stub> Ahh.... I should use self.recent_past actually
<stub> And add the comment there.
<mwhudson> heh
<mwhudson> yes please :)
<stub> https://code.edge.launchpad.net/~stub/launchpad/librarian-report/+merge/16522
<stub> (not at all urgent)
<stub> (but short)
* stub 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
<stub> jtv: https://code.edge.launchpad.net/~stub/launchpad/librarian-report/+merge/16522 still needs its bureaucracy dealt with.
<jtv> stub: oh, I thought bac approved that one?
<jtv> stub: I still hate you for using slashes in dates, but given that it's a matter of "whatever the db accepts"...
<stub> click the button and stop wining like a bitch
<jtv> stub: I already did, nigga
<stub> Thank you my good man.
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> https://code.edge.launchpad.net/~jpds/launchpad/fix_489363/+merge/16531 - is good to go. :)
<jpds> jtv: Maybe you could take a look?^ :)
<jtv> jpds: I'm OCR today, so that sounds reasonable.  :-)
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: jpds || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> jpds, a pattern I've found massively useful to know about: when you find yourself retrieving the current time, isolate it in a method that your test can override.
<jtv> jpds: for instance, I think your test will fail after december 31st, 9999 and you can avoid that by dictating the time.
<jtv> (hurry!)
<jtv> jpds: I'd suggest two separate testsâone with the real time to ensure that logging doesn't break due to differences between "naÃ¯ve" and "timestamp-aware" timestamps and such, but doesn't really care about the output; and another that fakes the time and looks for an exact match.
<henninge> jtv: fancy a review of the intltool detection code?
* henninge changed the topic of #launchpad-reviews to: on call: jtv || reviewing: jpds || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge: cool!
<henninge> jtv: actually, the biggest part is translation domain extraction now ...
<jtv> jpds: I do like your logging mixin, and it makes it very easy to get this testing right
<jtv> jpds: taking a step back though, why not the standard logging module?  The log files from our scripts do have timestamps.
<jpds> jtv: Those are interesting senarios, will look into them.
<jpds> jtv: Which module is that?
<jtv> jpds: "import logging"
<jtv> I'm sure it has a facility somewhere for directing to a file as well.
<jtv> it seems the sort of wheel you're doomed to reinvent here
<jpds> jtv: Hmm, well, the changes I've made are the ones sinzui suggested.
<jtv> jpds: then I defer to his wisdom... he's not the sort to miss a thing like that
<jtv> jpds: I'd definitely try to move log_file into LoggingMixin entirely, so ArchiveMirrorProberCallbacks doesn't need to know about it; and make the test just pass a StringIO in its place.
<jtv> Then afaics you don't need to create that fake logging class for your test
<jpds> jtv: OK, I'll look into that.
<jtv> jpds: meanwhile I'll go have my team standup :-)
<allenap> jtv: Oops, I just remembered I'm OCR today too.
<bac> hi henninge
<henninge> bac:  in call atm
<jtv> allenap: the more the merrier... I can start work on henning's branch soon though
<bac> henninge: ok.  i looked at your 'deactivate a user' branch yesterday but it still had test failures
<allenap> jtv: Okay, cool. I think it makes more sense for you to do his branch in any case.
<jtv> y
<henninge> bac: Hi!
<henninge> bac: yes, I am aware of that.
<jtv> jtv à¸«à¸´à¸§
<henninge> bac: I had meant to pass the branch on to intellectronica but never got around to it.
<henninge> bac: We just agreed that I will fix and get this done today, so it can land.
<bac> henninge: ok, cool.  if you need me to look at it tomorrow i'd be happy to.
<bac> (i'm off today)
<henninge> bac: great! I feared I might not get a reviewer tomorrow ... ;) Although, we may not have much overlap since I am off in the afternoon.
<bac> henninge: i can review it and if it needs no revisions could even land it for you
<henninge> bac: cool, thanks.
<jtv> henninge: I find check_potfiles_in a bit hard to read... why not:
<jtv>     command = (
<jtv>         "cd '%s' %% rm -f missing notexist && intltool-update -m" % path)
<jtv> ?
<jtv> (Although with variables on command lines in gnu systems it's usually a good habit to add a "--" to avoid accidental injection of command-line options)
<jtv> Speaking of which, I suppose path needs escaping.
<jtv> henninge: did you get my note about check_potfiles earlier?
<jtv> jpds: any progress?
<jpds> jtv: Sorry, popped out for lunch.
<jtv> jpds: I'm asking since I won't be sticking around for too much longer
<jpds> jtv: So far I have http://paste.ubuntu.com/345345/ - now I *think* I have to create something like DummyProductReleaseFinder in lib/lp/registry/tests/test_prf_finder.py /
<jtv> jpds: the method looks fine... why the DummyProductReleaseFinder?
<jpds> jtv: So I can make _getTime return a predetermined datetime?
<jtv> jpds: in the tests?  You can just re-assign the method as if it were a member variable.
<jpds> jtv: That's what I meant, like: http://pastebin.ubuntu.com/345349/
<jtv> jpds: why not instantiate a normal LoggingMixin in the test (no special class) and insert a fake _getTime there?
<jtv> jpds: same for setting log_file (and btw I don't see why the logfile variable is needed there)
<jpds> jtv: special class> Can you give me an example?
<jpds> jtv: logfile> Copied that from what Curtis suggested.
<jtv> jpds: something like prober = LogginMixin() ; prober.log_file = StringIO() ; prober._getTime = fake_gettime"
<jtv> ...where fake_gettime is a function that you create in your tests, which returns a fixed datetime.
<jtv> jpds: as mentioned, it'd be nice if the log_file could be passed to the LoggingMixin as a constructor argumentâthat'd also remove ugliness from the test.
<henninge> jtv: I am back
<henninge> jtv: saw your note, let me look at it
<jtv> henninge: I'm working on a reply in the MP right now
<henninge> jtv: I copied that call to intltool-update from danilo's damned lies code and he was using dicts with format strings all the time.
<jtv> henninge: it can be a nice thing to do, but in this case in _my_ opinion it harms readability more than it helps.
<henninge> jtv: but I have no problem writing it the way you suggested, except for the single vs. double quotes. I don't think they are as equal as they are in python.
<jtv> jpds: note fake_gettime is private, so better start its name with an underscore
<jtv> henninge: no they aren't...  double-quotes allow the variable to pull a lot more tricks.  That's why I usually use single quotes in shell scripts.
<henninge> I see
<jtv> but ultimately the only way to make it safe is to escape the variable
<jpds> jtv: Correct fake_gettime.
<jpds> corrected*
<jtv> jpds: what was the problem?
<jtv> you said something about it getting overridden?
<jpds> jtv: Oh, just the underscore thing.
<jtv> ah
<henninge> jtv: hm, do what do we have to do the escaping. I am a bit stumped atm.
<jtv> henninge: good question :/
<jtv> I'm running out of time here, but you could google for shell escaping in python
<henninge> jtv: that's fine.
<jtv> or alternatively, prove to me that what's in that variable isn't going to be a problem in our use case :-)
<henninge> jtv: it isn't because the paths are provided from another function that gets them from the file system.
<henninge> (that sentence should probably be using singulars to be clearer)
<jtv> henninge: perhaps... I don't see how it implies that the string is safe
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: jpds, - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: because it is not entered by a user but derived from a system call.
<henninge> jtv: the top-level function only works on the current directory, no chance for someone to get in  a malicious parameter.
<jtv> jpds: in your test, you could now verify that the log message is what you expect using a simple self.assertEqual(expected_value, actual_value)
<jpds> jtv: Ah, I just took a self.failUnlessEqual() and am running the tests now. :)
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: jpds, - || queue [henninge, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge: the path comes from a parameter... I don't see anything here that tells us what that parameter might be
<jtv> jpds: that's fine too
<henninge> jtv: you were talking about our particular use case ...
<jtv> henninge: yes, but I still don't know why a package couldn't have a directory "\" ; rm -rf / ; echo \""
<jpds> jtv: The test passes. :)
<jtv> jpds: \o/
<jtv> now, with these tricks you sometimes get subtle little type mismatches the fake you injected isn't quite compatible with what the real call would have given you.  So you'll also want a test without the fake, and without testing the output in detail, just to make sure using a real time doesn't break.
 * jtv left out a word there
<jtv> you sometimes get subtle little type mismatches _where_ the fake you injected isn't quite compatible
<sinzui> Was the word "waffle"?
<jpds> jtv: OK, and what should I do about the 31/12/9999 thing?
<jtv> jpds: I don't serious think we need to worry about the Y10K problem just yet.  :-)  In fact, that second test should be very lightweightâit really just needs to break when the datetime trick you pulled in your current test is wrong.
<jtv> *seriously
 * jtv must be very tired
<sinzui> jpds: The date should be formatted at 2011-01-31
<sinzui> dates are bigendian in reality
<allenap> sinzui: Are you reviewing https://code.edge.launchpad.net/~jpds/launchpad/fix_489363/+merge/16531, just passing through, or talking about something else? In either of the last two cases, I'll review it.
<jtv> allenap: I think that's the one I'm looking at
<jtv> yes, it is
<allenap> jtv: Okay, I'll leave it alone then.
<sinzui> allenap: I am passing through. I saw a confusing date and had to remark on it
<allenap> sinzui: Cool, thanks.
<jtv> jpds: sinzui was right about the date btw...  slashes in dates are pure, unmitigated, 7th-circle-of-hell evil
<jpds> sinzui, jtv: I'm not doing slashes in dates, I'm just doing: "Wed Oct 20 12:00:00 2004".
<jtv> jpds: I think he was just commenting on how you wrote it in here.  :-)
<sinzui> I was
<allenap> jtv: Why are slashes so bad in dates? Honest question :)
<jtv> allenap: what date is 08/09/10 ?
<allenap> jtv: But what date is 08-09-10?
<jtv> allenap: at least the convention there is pretty unambiguous: September 8 of next year
<henninge> jtv: do you like this better? http://paste.ubuntu.com/345393/
<henninge> jtv: it avoids the shell completely
<jtv> henninge: wow!
<henninge> that can mean anything
<jtv> henninge: nonsense...  it can not mean, for instance, "the 3rd decimal of pi is 1"
<henninge> ok, but apart from that ...
<allenap> jtv: I suppose so. I hoped everyone learnt to use the full year about a decade ago. As for month and day ordering, I think there's only the Americans to blame. Alas, 300 million people are difficult to ignore.
<jtv> henninge: in this case I mean to express admiration at your solution.
<henninge> "Wow, this is great!" or "Wow, I have never seen such rubbish!"
<henninge> option 1 it is
<henninge> jtv: I am glad you llike it ;)
<jtv> allenap: that's it in a nutshell... ostracizing them would be difficult and could be called discrimination, so the easiest way out is to ostracize the slash instead
<jtv> henninge: and "call" looks a lot simpler than Popen too
<henninge> a little googling and RTFM
<henninge> jtv: did you have any other points to raise about the branch?
<jtv> henninge: yes, still working on that reply in the mp
<henninge> jtv: anything we can short-cut here?
<jtv> henninge: I'm almost through, hang on
<jtv> henninge: I've posted a reply
<jtv> henninge: feel free to point out where I get nonsensical...  I seem to be making a lot of mistakes tonight
<sinzui> allenap: jtv: I have a CP candidate that will appear in reviews in a few minutes
<allenap> sinzui: I can take it.
<jtv> allenap: I was about to ask.  Thanks.  :)
<sinzui> flacoste will check it after it clears review and we verity it on staging
 * henninge checks Bangkok time
<henninge> jtv! Go to bed!
<jtv> henninge: soon!
<henninge> jtv: today! (your today)
<jtv> henninge: that'd be hard, unless I sleep in the office
<jtv> jpds: test_logMessage_ensure_message looks good...  in test_logMessage_fake_time you can also use a local variable instead of a class member; then everything referring to self.prober can go away and you'll end up with just 3 methods.
<jtv> _fake_gettime, test_logMessage_fake_time, and test_logMessage_ensure_message
<jtv> jpds: I wouldn't call the middle one test_logMessage_fake_time though; that's how you test, but you want to say what behaviour you're testingâmaybe test_logMessage_output because you're actually verifying the output
<jtv> jpds: in a similar vein, maybe test_logMessage_integration for test_logMessage_ensure_message since you're testing the integration.  (A bit far-fetched perhaps; but it seems weird to say both "test" and "ensure" for the same thing in the same identifier)
* allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: jpds, sinzui || queue [henninge, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: {jpds, henninge}, sinzui || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: that's a nice corner I've painted you into...
<allenap> jtv: Hehehe :)
<allenap> allenap: r=me, it's Christmas after all.
<jpds> jtv: All done and pushed.
<al-maisan> jtv: not sure what time it is on your end but the branch we discussed earlier is now ready for review: https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550
<jtv> allenap: wait... you can _do_ that!?
<jtv> al-maisan: far too late for another review!
<al-maisan> jtv: understood ..or maybe allenap could take a look at it?
<al-maisan> if it's not too late for him
<jtv> jpds: and since I'm being difficult... LogginMixin & its methods need to have docstrings.  :-)
<allenap> al-maisan: I very much doubt I'll get to it. It's my last day today and I have some other things to finish off after doing sinzui's branch and before going afk.
<al-maisan> allenap: I understand .. never mind .. have a nice break!
<allenap> al-maisan: Thanks :)
<al-maisan> jtv: are you around tomorrow by any chance?
<jtv> al-maisan: try bugging me about it tomorrow
<jtv> heh
<al-maisan> jtv: he he :)
<jpds> jtv: Sure, done.
<jtv> thanks
<jtv> jpds: approved.  You can now "ec2 land https://code.edge.launchpad.net/~jpds/launchpad/fix_489363/+merge/16531"
<jpds> jtv: I don't have access to PQM.
<jtv> jpds: nm I'll do it then
<jpds> jtv: Thanks!
* jtv changed the topic of #launchpad-reviews to: on call: allenap || reviewing: sinzui || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: that's me bowing out :)
<jtv> henninge: continue on your mp tomorrow?
<allenap> jtv: Cheerio, until 2010 :)
<henninge> jtv: yes, I'll have something ready for you when you start.
<henninge> jtv: thanks for hanging around this long.
<jpds> jtv: Have a great holiday!
<jtv> allenap: oh, you're not here tomorrow?  then happy 2553 BE!
<allenap> jtv: Thanks :)
<jtv> jpds: thanks, same to you!
<jtv> henninge: great, I like tag-team development
<jtv> oh, it's midnight... no more trains.  :)
<allenap> sinzui: All done, r=me.
<sinzui> thanks. Have a lovely holiday
* allenap 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
#launchpad-reviews 2009-12-24
<jml> hello!
<jml> fun & short patch here: https://code.edge.launchpad.net/~jml/launchpad/import-fascist-tweaks/+merge/16560
<al-maisan> hello jtv, bugging you re. https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550 as requuested :)
<al-maisan> hello jml, could we trade reviews please?
<al-maisan> jml: I am looking at your import fascist branch right now :)
<jml> al-maisan, thanks.
<jml> al-maisan, which branch do you want me to review?
<al-maisan> jml: https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550
<jml> al-maisan, will do.
<al-maisan> jml: thanking you.
<wgrant> Does anybody know what happened about bug #500018?
<al-maisan> wgrant: I guess nothing so far
<wgrant> al-maisan: OK, there was some discussion about finding somebody to cowboy it.
<al-maisan> wgrant: I'll need to touch base with jelmer and/or get the hole plugged myself
<al-maisan> wgrant: I will start on it in 20 minutes or so.
<al-maisan> jml: the imports code you removed from lib/lp/soyuz/model/archive.py : were they just superfluous?
 * jml looks
<jml> al-maisan, they were, after I deleted the two lines like:
<jml> -        source_package_name = getUtility(ISourcePackageNameSet)[source_name]
<jml> al-maisan, those lines assigned to an unused local variable
<jml> al-maisan, and have no obvious side-effects
<al-maisan> jml: I see, thanks!
<al-maisan> jml: the changes in lib/lp/scripts/utilities/importfascist.py look reasonable, did you test them in any way?
<jml> al-maisan, the fascist itself doesn't have tests that I can find.
<jml> al-maisan, I've been running the fascist locally.
<al-maisan> jml: but I guess running anything will exercise it
<jml> al-maisan, correct.
<jml> al-maisan, I've hammered & sent out an email reply to your review
<jml> al-maisan, to your patch, rather.
<jml> al-maisan, I'm going to get some food now, but I'll be back in a bit to handle round #2
<al-maisan> jml: thanks, I am just running "bin/test -vv -t mail -t archive" in your branch to be on the safe side
<jml> al-maisan, heh. thanks. though I was certainly going to submit it via ec2 test in any case.
<al-maisan> jml: FYI, the test on line 2407 in doc/archive.txt is failing
<al-maisan> http://pastebin.ubuntu.com/345778/
<jml> al-maisan, I guess those lines do have side effects!
<al-maisan> jml: yeah .. either deliberate or inadvertent ones :P
<jml> al-maisan, I see what's going on....
<jml> al-maisan, getting the sourcepackagename object there gives a good error for those functions if it doesn't exist
<jml> but that's all it does
<jml> I'll remove the local assignment to keep pyflakes happy and add a comment saying why we do something that appears useless at first glance.
<al-maisan> jml: ah, OK, it's a deliberate point of failure.
<jml> al-maisan, http://pastebin.ubuntu.com/345781/
 * jml really dining now.
<al-maisan> jml: with the change in the pastebin above: r=me
<al-maisan> hmm .. clicking on "Claim review" on https://code.edge.launchpad.net/~jml/launchpad/import-fascist-tweaks/+merge/16560 results in an OOPS
<al-maisan> .. a reproducible one
<al-maisan> http://pastebin.ubuntu.com/345785/
<jml> al-maisan, I've heard mention of a similar bug...
<al-maisan> jml: after refreshing the page, the review would appear to be claimed by me and I was able to finish the review approval workflow.
<jml> al-maisan, hmm. I can't find a record of that in the bug tracker.
<al-maisan> jml: would you like me to add a bug?
<jml> al-maisan, yes please. assign it to thumper, I reckon.
<al-maisan> jml: will do.
<jml> al-maisan, I'm just about to sign off for the year. Do you want to go one more round on the buildJobQuery branch first?
<al-maisan> jml: ideally yes, I am about to reply to your review email. It will take another 5 minutes or so.
<jml> al-maisan, no worries.
<al-maisan> jml: reply sent.
<jml> al-maisan, thanks. 
<adiroiban> al-maisan: hi. Do you have time for landing this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375/+merge/16279 ? Thanks! I have done a full test and everthing was ok.
<al-maisan> adiroiban: yes, I'll do it in the course of the next hour or so..
<al-maisan> jml: fixes to the branch under review so far: http://pastebin.ubuntu.com/345813/
<jml> al-maisan, replied.
<jml> al-maisan, if the classProvides / interface thing is too much, I guess adding a XXX would be OK, but I think that for this particular area of work, we should be trying very hard to avoid tech debt.
<jml> al-maisan, (he says by way of a postfix)
<jml> al-maisan, good night. I'll see you in the new year.
<al-maisan> jml: thanks for looking at this, have a great break!
<jtv> henninge: I'm working on an MP for our buildfarm work... you up for a review?
<jtv> al-maisan: which reminds me... did you find a reviewer for yours?
<henninge> jtv: sorry no, my year is almost done.
<al-maisan> hello jtv, yes, jml reviewed the branch but it would be good if you could rubber-stamp it once I have the changes he requested in place.
<al-maisan> jtv: how long will you be available today?
<jtv> al-maisan: ok, I'll be around for a few more hours
<al-maisan> jtv: I should have something in the next hour or so..
<jtv> I'm just putting up my MP, though if you land an extension to IBuildFarmJob first, it will break my test.  :-)
<al-maisan> henninge: how about you? Are you working normal hours today?
<al-maisan> jtv: that's a concern .. why would it break the tests though ?
<al-maisan> there is a minimal implementation of the 3 methods added to the interface
<jtv> al-maisan: because my test verifies that my IBuildFarmJob implementation really does provide all of the interface's API
<jtv> oh, if there is a minimal implementation in BuildFarmJobBase then it won't break things
<henninge> al-maisan: no, got the afternoon off.
<al-maisan> OK
<jtv> al-maisan: my mp is up
<jtv> al-maisan: want to review each other mutually?
<al-maisan> jtv: I can review your branch but mine is not ready yet
<jtv> al-maisan: then you can't review mine either, because you've got other things to do.  :-)
<al-maisan> jtv: if you give me 30 minutes it may work out for both of us.. 
<jtv> \o/
<al-maisan> jtv: OK .. ready now .. please note that jml already approved of the branch (https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550) ..
<jtv> al-maisan: I was just reading up on the preceding review thread
<al-maisan> .. provided I fix a few things
<al-maisan> the fixes are here: http://pastebin.ubuntu.com/345918/
<al-maisan> .. and I am just pushing the branch
<jtv> al-maisan: btw what exactly does virtualization mean here?  That the slave must be in a vm?
<al-maisan> yes
<jtv> al-maisan: that'll become relevant when I update the branch I just put up for review to match the one you're having reviewed.  :-)
<al-maisan> jtv: as I said before: if your branch lands first I will add a perfunctory getPendingJobsQuery() to your class in my branch to avoid breakage
<jtv> oh, I read that differently earlier... thanks.
<jtv> al-maisan: "get" as a bit generic and over-used as an opening verb for method names...  how about composePendingJobsQuery to reflect that it puts together a textual query?
<jtv> (This is a verb I use a lot in these situations)
<al-maisan> jtv: OK .. that's fine.
<jtv> thanks
<jtv> al-maisan: and in its docstring, the docs for min_score and processor end without punctuation.
<al-maisan> jtv: will fix that.
<jtv> thx
<jtv> al-maisan: in the test you use processor='386'...  not 'i386'?
<al-maisan> yes, you can see it in the psql excerpt I have in the test as well
<jtv> oh well, not our problem
<al-maisan> jtv: line 232 for example
<jtv> al-maisan: here's my mp btw: https://code.edge.launchpad.net/~jtv/launchpad/db-bug-499404/+merge/16564
<al-maisan> jtv: thanks -- I am free to look at it now .. sorry for the delays
<jtv> stop apologizing!
 * al-maisan apologizes for apologizing ;P
 * jtv glares back
<al-maisan> jtv: the changes you suggested: http://pastebin.ubuntu.com/345928/
<jtv> looks fine
<al-maisan> jtv: thanks!
<jtv> The comment you added in TestBuildPackageJob.setUp chains two "so that"s
<al-maisan> Oh!
<jtv> And I see that elsewhere you use x86 instead of 386 (or i386 or IA32 or 80x86...)
<jtv> But not worth changing that last bit.
<al-maisan> hmm .. ok
<jtv> I didn't know about classProvides btw; it's a bit meta but it does look useful
<jtv> Wonder how we test for it.
<al-maisan> jtv: the tests in the branch exercise the method specified in the interface mentioned in classProvides()
<jtv> al-maisan: I was thinking of "make a test verify that the class really provides the complete interface"
<al-maisan> jtv: it's only one method in this case
<jtv> True...  sometimes though I come across methods whose interface definitions and model-class definitions have grown apart
<jtv> Imagine that you add a parameter to that one method: it'd be nice if all classes had tests verifying that they support the new parameter.
<al-maisan> jtv: hmm ... I see what you mean .. /me starts grep'ing for classProvides in tests
 * al-maisan finds from canonical.launchpad.webapp.testing import verifyObject
<jtv> Who knows, it might work on classes and classProvides as well as objects and implements
<al-maisan> hmm..
<al-maisan>     def test_providesInterface(self):
<al-maisan>         """Ensure that BranchDiffJob implements IBranchDiffJob."""
<al-maisan>         verifyObject(
<al-maisan>             IBranchDiffJob, BranchDiffJob.create(1, '0', '1'))
<jtv> Yes, you normally pass it an interface and an object
<jtv> I've been looking for mentions of interfaces that this is used for in tests, but zip so far
<jtv> ah! lib/lp/code/model/tests/test_diff.py
<jtv> verifyObject(IMyInterface, MyClass)
<al-maisan> yep
<al-maisan> jtv: the changes discussed: http://pastebin.ubuntu.com/345943/
<jtv> al-maisan: perfect, thanks
<al-maisan> jtv: thank *you*
<jtv> I'm about ready to give that rubber stamp now.  :-)
<al-maisan> jtv: you are one picky reviewer ;)
<jtv> yup
 * al-maisan feels compelled to type "meine Rache wird bitter sein" for some reason :-)
<jtv> Hmm... nicht sÃ¼Ã?
<al-maisan> jtv: na ja, das auch :)
<jtv> I've gone through enough horrible crap code to be really, really grateful and environmentally conscious about this stuff...
<jtv> one of my favourites:
<al-maisan> jtv: fully agree :)
<jtv> for (i=0; i < result.size(); ++i) {
<jtv> switch (i % 3) {
<jtv> case 0:
<jtv> blablah();
<jtv> break;
<jtv> case 1:
<jtv> foofoo();
<jtv> break;
<jtv> case 2:
<al-maisan> all specimen of finest code :)
<jtv> barbar();
<jtv> break;
<jtv> case 3:
<jtv> foofoo();
<jtv> barbar();
<jtv> break;
<jtv> }
<jtv> return;
<jtv> }
<jtv> ah, there was an "if" as well
<al-maisan> jtv: no goto?
 * al-maisan is slightly disappointed :)
<jtv> ah! no goto, but I forget the "default:" case
<al-maisan> :-)
<jtv> Note how the "case 3:" won't be exercised except in cases where you have much, much worse things to worry about
<al-maisan> like broken CPUs?
<jtv> or broken compilers
<jtv> and this one was broken... oh God was it broken
<al-maisan> jtv: if you continue like this your branch will never be reviewed ..
<al-maisan> :)
 * jtv swallows some great anecdotes about Microsoft's compiler
<al-maisan> we can chat next year :-)
<jtv> ok :)
<al-maisan> in NZ
<jtv> ok
<jtv> al-maisan: I added my approval
<al-maisan> jtv: thank you!
<jtv> np
<EdwinGrubbs> sinzui: ping
<sinzui> Hi EdwinGrubbs
<EdwinGrubbs> sinzui: are you working today?
<sinzui> yes and no
<sinzui> EdwinGrubbs: I am hacking on some critical items, but I am also om my day of
<sinzui> off
<sinzui> EdwinGrubbs: How can I help you?
<EdwinGrubbs> sinzui: just wondering if you wanted to review a 2810 line branch that only has 731 lines which are not formulaic changes, but that is not a good way to spend your day off.
<EdwinGrubbs> sinzui: bye now. Have a good holiday.
<jtv> al-maisan: any remarks from the review so far?
<al-maisan> jtv: none so far .. 
<al-maisan> jtv: thanks for cleaning up the doc strings for some of the IBuildFarmJob methods!
<jtv> nearly removed them for fear of causing conflicts with your work :-)
<al-maisan> jtv: why the "pylint: disable-msg=E0211,E0213" on line 119?
<jtv> al-maisan: IIRC that's the warnings about "hey this method doesn't take a 'self' argument"
<jtv> lemme check
<al-maisan> thanks
<jtv> al-maisan: actually, it seems I only need to suppress 213
<jtv> so I'll remove the other one
<al-maisan> thanks again
<al-maisan> my first thought re. `IBuildFarmJobBehavior`: do we really have to impose an implementation of this interface upon each job class? It seems pretty generic.
<jtv> Yes, I was wondering about that too... but then again the best way to find out is to duplicate and then unify
<al-maisan> jtv: yeah .. I guess there's no way around it.
<jtv> I think it won't be too hard to factor most of it away, but first we'll have to find out what differences we need...
<al-maisan> hmm..
<al-maisan> jtv: I like your '4010' job score :)
<jtv> al-maisan: we may want to improve on it later, but right now I just statically computed what seemed the closest equivalent for package build jobs
<al-maisan> as you said in the comment: if the duration is short it may not matter much ..
<al-maisan> ..except if you flood the build farm with gazillions of these jobs 
<jtv> ...at which point we'll probably just want to tweak the score statically to keep them down a bit.  :-)
<al-maisan> jtv: what would happen if we dropped one of these on the build farm as it stands? Would TranslationTemplatesBuildBehavior.dispatchBuildToSlave() work?
<jtv> al-maisan: my impression was that it wouldn't, because BuildQueue isn't really ready for that
<jtv> (at least until your branch lands...)
<al-maisan> Well, there's a much more fundamental build farm piece that needs adjustment: candidate job selection
<jtv> that, tooâthough I guess that would not affect this particular method
<al-maisan> the branch you approved previously is just a part of the work that needs to get done in order to have job dispatch time estimation working across different job types
<al-maisan> right.
<jtv> one problem is that I don't really know what affects what there; it's actually not unthinkable that the method might work as-is
<al-maisan> jtv: OK .. so, I did not look at the db part .. the code part looks reasonable .. but if I approve it what have we gained .. not quite sure where we stand..?
<jtv> [Nice: D'Addario colour-codes its guitar strings... no individually labeled packaging needed]
<jtv> al-maisan: we'll have the BuildFarmJob part for our type of job basically ready.  Next I add testing for the dispatch method; implement the method you just defined; and register the new job type in specific_job_types.
<jtv> (Of course I'll also get a db review)
<al-maisan> jtv: OK .. makes sense .. do you want me to approve the branch in that context?
<jtv> al-maisan: not sure I understand the question
<al-maisan> jtv: re. "register the new job type in specific_job_types": this part is in doubt, there are some other ideas floating around how this can be implemented in a cleaner way in BuildQueue
<jtv> al-maisan: I understand... all the more reason why I didn't implement those in-flux parts yet
<jtv> my impression though was that we have some pretty stable interfaces already, and those parts it seems to make sense to land
<al-maisan> jtv: I have no qualms about the ITranslationTemplatesBuildJob* part
<al-maisan> the build behaviour part though is more obscure and untested
<al-maisan> just a minute please
 * jtv strings up another string
 * al-maisan looks for any tests for dispatchBuildToSlave()
<al-maisan> jtv: this is still very much work in progress and as such looks good, r=me (code)
<jtv> al-maisan: wonderful, thanks
<al-maisan> jtv: thank you!
<jtv> I don't like going in with code that's not complete enough to be operational, but it's better than letting it bit-rot unmerged
<al-maisan> jtv: yeah .. I understand
<jtv> [Some things I do miss in this guitar: the monkey grip and having the tuning knobs all on one side of the head]
<al-maisan> jtv: enjoy your break!
<jtv> thanks, same to you too!  I believe there's some custom-made egg-nog waiting for me
<al-maisan> ..and thanks for bearing with me for such a protracted period of time ;)
<jtv> no worries... kinda peaceful here tonight.  Good night, happy holidays, & see you in 2553 BE!
 * al-maisan is being called to the dinner table
 * al-maisan will google "2553 BE" later :P
<bac> jelmer: you can submit the *Publishing fix with rs=bac if you want, assuming there is nothing else in it.
<jelmer> bac: Thanks!
<bac> jelmer: please reply to the email on the launchpad list about the buildbot failure stating you've submitted a fix.  otherwise someone else may waste time investigating.
<jelmer> bac: ok
<bac> jelmer: do you know anything about the new flood of import warnings spewed by bin/test?
<jelmer> bac: No, sorry
<bac> jelmer: jml's branch 10090 changed the importfascist and caused all of the new warnings.
<al-maisan> bac: yep, http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/10090
<bac> al-maisan: i haven't looked into the warnings to see if they are accurate or not...
<al-maisan> same here, 
<al-maisan> but jml's branch looked good.
 * al-maisan needs to go
<al-maisan> have a nice vacation!
#launchpad-reviews 2010-12-30
<jelmer> jml: How hard would it be to avoid the patch() call in test_translationtemplatesbuildmanager.py ?
<jml> jelmer: good question.
<jml> I think I'd need to change some stuff to not do "print "
<jml> jelmer: I think I'd have to change lib/canonical/buildd/debian.py
<jml> jelmer: I really didn't want to do that
<jml> brb
<jml> back
<jelmer> re
<jelmer> jml: I think that should be doable.
<jelmer> jml: My worry is that "eating" all of the stdout output might hide some useful messages printed out by other bits of the code.
<jml> jelmer: ok. I'll give it a go.
<jelmer> jml: If you like, I'd be happy to +1 the changes without the patch() call for now?
<jml> jelmer: sounds good. I'll revert the patch() change and land.
