#launchpad-reviews 2010-01-25
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [jpds] || 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 [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * henninge can't stand the quiet .... ;)
* henninge 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
* 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
<gary_poster> hey gmb.  For the release-critical request for https://code.edge.launchpad.net/~gmb/launchpad/remove-bug-heat-from-garbo-daily-bug-511283/+merge/17912 , I'm trying for the guideline of "if it would have been a CP, it can be a release-critical".  The problem you say it addresses does sound like it would be a CP candidate under non-release circumstances, but could you verify before I pursue further?
<sinzui> gary_poster: I have a bug that I just tagged as a release blocker. Maybe I was wrong: https://bugs.edge.launchpad.net/launchpad-registry/+bug/512182
<mup> Bug #512182: Karma and bug activity missing when bugs are updated by creating a release <current-rollout-blocker> <Launchpad Registry:In Progress by sinzui> <https://launchpad.net/bugs/512182>
<gary_poster> looking
<gary_poster> sinzui: my evaluation is that it is CP candidate, and therefore I will approve it for RC if it is available, but it doesn't sound like a release blocker to me because the two symptoms are not critical (that is, I value an on-time release higher than those two symptoms).  Are you OK with that evaluation, or would you like to talk about it more?
<sinzui> gary_poster: yes
<gary_poster> sinzui: cool thanks
<sinzui> EdwinGrubbs: can you review my branch at https://code.edge.launchpad.net/~sinzui/launchpad/release-bugtask-bug-512182/+merge/17989
<EdwinGrubbs> sinzui: sure
<EdwinGrubbs> sinzui: is it really 13,000 lines, or should it be merged into db-devel instead of devel?
<sinzui> oh bugger
<sinzui> EdwinGrubbs: I will remake the merge request to db-devel, The diff is less that 100 lines
<EdwinGrubbs> ok, cool
<sinzui> EdwinGrubbs: https://code.edge.launchpad.net/~sinzui/launchpad/release-bugtask-bug-512182/+merge/17999
<intellectronica> abentley: can i ask you to review a small branch of mine, an r-c candidate to fix those timeouts in bug listings?
<abentley> intellectronica: okay
<intellectronica> abentley: thanks. https://code.edge.launchpad.net/~intellectronica/launchpad/patch-badge-performance/+merge/18000
<abentley> intellectronica: BugTaskImageDisplayAPI already exposed has_patch?
<intellectronica> abentley: yes. i've added it in a previous branch, but didn't realise it's not being used because it's not included in the decorator
<abentley> intellectronica: What decorator do you mean?
<intellectronica> i mean BugTaskListingItem
<abentley> intellectronica: I thought BugTaskImageDisplayAPI was an Interface because of its name and comment string.  Now I understand that BugTaskListingItemImageDisplayAPI is a subclass of it.
<abentley> intellectronica: I don't see any call site updates to match the new BugTaskListingItem.__init__.  Where is it constructed?
<abentley> intellectronica: never mind.
<intellectronica> abentley: found it? they're passed as a dict to BugListingBatchNavigator which is why you missed them
<abentley> intellectronica: Okay, approved.
<intellectronica> abentley: thanks!
<intellectronica> abentley: can i ask for an extra line for review? the same needed to be added in milestone views, since they also use BugTaskListingItem. see http://pastebin.ubuntu.com/362666/
<abentley> intellectronica: r=me
<intellectronica> cheers
<sinzui> EdwinGrubbs: Do you have time for that review? Should I seek someone else?
<EdwinGrubbs> sinzui: I think this error is unrelated to your branch, but it is making it difficult for me to test things. When I run "make", I get ImportError: No module named builder.recipe
<EdwinGrubbs> sinzui: do you get that error?
<sinzui> I have not gotten that error.
<EdwinGrubbs> sinzui: I think rocketfuel-get will solve it. I've gotten so used to running "bzr up download-cache" that I hardly ever run it.
<sinzui> EdwinGrubbs:  I certainly would have noticed that error. I had trouble with the code...the test was right, but I forgot to use notify so I ran my test many times without issue to understand my incompetence.
<EdwinGrubbs> sinzui: make works now. Running the tests.
<EdwinGrubbs> sinzui: it looks like Bug.setStatus() calls BugTask.transitionToStatus() and sends an event. you could use that with bug.setStatus(bugtask.target, status, user)
<sinzui> bugtask.editView uses the approach I took
<EdwinGrubbs> sinzui: ok, r=me
<sinzui> EdwinGrubbs: how would you test the use of bugtask.bug.setStatus(self.series_target, BugTaskStatus.FIXCOMMITTED, user)
<sinzui> Can I keep the same test and just update the code?
<EdwinGrubbs> sinzui: I think so.
<sinzui> let me try, The method you found has conjoined rules that I never ever want to write again
<sinzui> Oh, I cannot
<sinzui> EdwinGrubbs: I need 5 more minutes, the target is not as obvious as I thought because of source packages
<sinzui> EdwinGrubbs: Thanks. I got rid of a lot of cruft: http://pastebin.ubuntu.com/362757/
<sinzui> The test was fine
<EdwinGrubbs> sinzui: that looks good to go.
<sinzui> thanks you very much. bug.setStatus works as I thought transitionToStatus() worked.
<sinzui> Hi Gary. I am officially asking for an RC for this branch that we discussed a few hours ago: https://code.edge.launchpad.net/~sinzui/launchpad/release-bugtask-bug-512182/+merge/17999
<gary_poster_> ack, sincui, approving.
<gary_poster_> sinzui: 
<sinzui> gary_poster: ^ If we landed it, you can create a release and all the foundations bugs will be updated for you for free!
<gary_poster_> heh, cool
<gary_poster_> sinzui, the web interface is pretty slow, but I approved
<sinzui> beuno: Do you have time to do a follow up review of https://code.edge.launchpad.net/~sinzui/launchpad/needs-packaging-bug-509848/+merge/17848
<beuno> sinzui, yes
<bac> hi beuno
<beuno> hi bac 
<bac> beuno: on friday you noted some padding issues on the radio buttons as shown on http://people.canonical.com/~bac/linkup-multiple.png
<beuno> yes
<bac> beuno: i'm just using a vertical LaunchpadRadioWidget...so i don't see how the padding on it could be different than elsewhere
<beuno> those lines are all squished
<bac> beuno: but it does look cramped
<beuno> off the top of my head, I can't think fo why that is
<bac> i don't know fo why either
<beuno> bac, are they in li's?  divs?
<beuno> or just br's?
<bac> beuno: the widget does the rendering based on the vocab.  i haven't looked at the implementation of it to see how it is done.
<beuno> sinzui, doing your review
<beuno> I think it's mostly ok
<leonardr> gary: https://code.edge.launchpad.net/~leonardr/lazr.restful/generate-multiversion-operations/+merge/18032
<beuno> sinzui, done
<sinzui> thank beuno
<sinzui> beuno: I can make the link changes. This changes will appears on all pages and in the page title
<sinzui> s/this/those/
<beuno> sinzui, I think that's fine
<sinzui> I hope so, I only want one set of terms referring to these pages
<sinzui> beuno: That hardest part about developing a portlet to show the status of information that communities need to work with the porject is that we do not have a name for it
<beuno> sinzui, yaeh, it's more about "tell us how the community works"
<sinzui> The information we show on the source package should be presented the same way on the project page: https://dev.launchpad.net/Registry/UbuntuLinkUpstream?action=AttachFile&do=get&target=linked-package.png
<sinzui> I have been looking for a term that uses contributor or community for project version of this portlet.
<sinzui> Contributor Connections is the best term I have, and I do not like it
<beuno> hm
<beuno> it's a tricky one
<sinzui> oh, this information cooresponds to the Participation portlet, accept that it contains all the hosted and remote services
<beuno> I'll think about it while I write a long email
<sinzui> thanks
<bac> beuno: have a look at http://people.canonical.com/~bac/linkup-multiple-ff.png when you have a sec
<beuno> bac, much better
<beuno> but the "Upstream project" label is too far from the question  :)
<bac> beuno: i didn't do anything.  :)  FF vs. webkit
<beuno> you could, of course, drop that label
<beuno> it's meaningless to most people I think
<bac> beuno: that label is the 'title' for the widget.  i tried omitting it and just got a free-standing colon.
<beuno> garrrrrrrr
<leonardr> gary: if you want to warm up before reviewing my huge branch, here's an extremely short one:
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/missing-field-error/+merge/18037
<beuno> bac, can the label be the question?
<gary_poster> :-) ok
<bac> beuno: if we want the question to end with ":"
<beuno> double garrrrrrrrrrrrrr
<bac> beuno: so the spacing issue is webkit only.  i propose to ignore it for this branch and find/file a bug to address it in a later branch
<beuno> bac, I'll stamp it if you target it   :)
<bac> que?
 * bac curses his internet connection today
<beuno> bac, the bug for the webkit thingie
<bac> beuno: ok.  i'm still confused by stamp and target.  target where?
<bac> sorry if i'm being dense
<beuno> bac, ah, sorry for confusing you
<beuno> I'll approve the MP if you target the bug to be fixed in the next cycle
<beuno> these are the kind of bugs that almost never get fixed  :)
<bac> beuno: sure i'll do that
<bac> beuno: but, of course, i'll mark it as foundations...
<beuno> ha
<beuno> well played
<gary_poster> :-P
<gary_poster> leonardr: since you already looked at an earlier version of the file, could you review https://code.edge.launchpad.net/~gary/launchpad/bug_475371/+merge/18039 ?  I think this should just be comparing it to Tom's description in that bug report, as you already did, so I'm hopeful that this would be easy.
<leonardr> gary, sure
<gary_poster> thank you
<bac> sinzui: my MP is up if you're interested:  https://code.edge.launchpad.net/~bac/launchpad/bug-490518-link-suggestion/+merge/18040
<sinzui> bac: Thanks, I'll start it shortly
#launchpad-reviews 2010-01-26
<rockstar> thumper, do you think you have time to review an RC candidate?
<thumper> rockstar: shoot
<rockstar> thumper, great, thanks.
<rockstar> I just need to run it through one more tests.
<thumper> ack
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/upgrade-branch-rc/+merge/18049
<rockstar> gary_poster, ping
<gary_poster> rockstar: yo
<rockstar> gary_poster, got an rc candidate for you: https://code.edge.launchpad.net/~rockstar/launchpad/upgrade-branch-rc/+merge/18049
<rockstar> gary_poster, it's playing in ec2 now.
<gary_poster> cool, looking
<gary_poster> rockstar: approved
<rockstar> gary_poster, great, thanks.  I'll land as soon as the ec2 run is done.
<gary_poster> rockstar: sounds great, thank you
<al-maisan> Good morning!
<bac> good morning
<bac> gmb, you reviewing today?
* bac 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
<gmb> Argh
<gmb> bac: Yes, though no-one's asked so far.
<bac> as i'd expect...
* gmb changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: -, - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> i'm going to relocate but will be back in 12 minutes +/-
<noodles775> Hi guys, non-urgent MP ready here: https://code.launchpad.net/~michael.nelson/launchpad/510331-syncsources-latest-pub/+merge/18070
<jtv> hi bac, hi gmb!
<gmb> Hi jtv
<gmb> noodles775: I'll see if I can get to your mp this afternoon; depends on bugs QA atm
<noodles775> gmb: thanks, no rush.
<jtv> gmb: that sounds like you're busy... which is a shame because I've built up a bit of a backlog
<gmb> jtv: Well, I've only got a few more items to do and bac's here too, so we'll see what we can get through
<jtv> yay :)
<gmb> jtv, noodles775: Queue 'em up gentlemen and we'll start rolling shortly.
* noodles775 changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: -, - || queue [sinzui,noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> gmb: I'm not asking for priority, mind you; just pointing out there's stuff on activereviews.  :)
<gmb> :)
* bac changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: sinzui, - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> morning jtv, noodles.  we'll get to your reviews shortly
<jtv> bac: good morning!  thanks :)
<bac> sinzui:  you were on the review queue but i don't see anything on +activereviews.  stale request?
* bac changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: jtv, - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> Hey can I get a review on this what's new branch? it needs to go to gary for an RC soonish -- https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnews-10dot1/+merge/18071
<gary_poster> mrevell: yes.  Looking.  It's actually only because we have had a bit of a problem with the release that we can still land it, so we're lucky.
<mrevell> thanks gary_poster
* gmb changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: jtv, noodles775 || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> mrevell: approved https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnews-10dot1/+merge/18071 .  Please land asap
<mrevell> thanks gary_poster, it's going through the test-suite atm. It started 3 hours ago so should be done soon.
* bac changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: jtv-2, noodles775 || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775: r=me
* gmb changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: jtv, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb
<gmb> noodles775: One minor nitpick about a comment but nothing major
<jtv> hi bac, thanks for the review.   Like the 365/2; maybe I'll make it a leap year so we can avoid fractions.  :-)
<bac> jtv....uh, ok
<noodles775> gmb: great. Did you have any thoughts about a better way to test the private method (as mentioned in the MP)?
<gmb> noodles775: Argh, must've skipped that bit of the mp. Sorry. Hang on...
<gmb> noodles775: So, no, I can't think of a better way (which is why I never balked at the use of rSP()). The only way I could think of is to test it indirectly, but I'd rather have the direct test myself.
<noodles775> Great, thanks for that.
<leonardr> gary, given that you're busy with the release, would you like me to put generate-multiversion-operations in the general review queue?
<gary_poster> leonardr: yes, sorry.
<leonardr> gary, np
<gary_poster> salgado: also, feel free to do the same with yours, unless you want me to review (or you have a hard time getting someone to agree to the length)
<leonardr> bac or gmb, can you take a look at https://code.edge.launchpad.net/~leonardr/lazr.restful/generate-multiversion-operations/+merge/18032 ?
<leonardr> it's slightly over 800 lines
<leonardr> i'm available to chat about it in any level of detail
<bac> leonardr:  i can look at it in a bit
<leonardr> cool
<bigjools> bac, do we use camelCase or under_score for methods on the API?
<leonardr> bigjools, are you asking him to check on what i said?
<leonardr> bac, gmb: another branch for you, this one is much easier to review: https://code.edge.launchpad.net/~leonardr/lazr.restful/test-multiversion-collection/+merge/18089
<gmb> leonardr: I'll take it
<bigjools> leonardr: yes - don't take it personally :)  I've not seen that said before, that's all.
* gmb changed the topic of #launchpad-reviews to: on-call: bac, gmb || reviewing: jtv, leonardr || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> bigjools, np
<leonardr> let me try to find a doc on one of our many wikis
<leonardr> bigjools: i would expect to see it on https://dev.launchpad.net/API/ImplementingAPIs but it's not explicitly there
<leonardr> you will however see createProduct exported as 'create_product'
<leonardr> the general rule is that we want to follow pep 8 in the interfaces we expose the the public
<bigjools> leonardr: there's a load of stuff exported in camelCase as well :/
<leonardr> bigjools: yes, because people don't know about the coding standard
<bigjools> yay, standards, there's so many to choose from
<leonardr> if someone (maybe gary) can confirm this i'll add it to the wiki
<bigjools> yeah that'd be good to clarify, and bring up in a reviewers' meeting
<leonardr> once we have multiversion there will be a lot of renaming of methods
<gmb> leonardr: r=me
* gmb changed the topic of #launchpad-reviews to: on-call: bac || reviewing: jtv, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb calls it a day
<jtv> gmb: have a good one
 * gmb will have a sleepy one, but the thought is appreciated
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: leonardr || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> leonardr: we prefer initialCamelCase for methods, since the project adopted that before PEP 8 preferred underlines.  https://dev.launchpad.net/PythonStyleGuide#Naming
<leonardr> gary: i'm talking about the names we present to the outside world via the web service, not the names we use internally
<leonardr> i think flacoste would be a definitive source
<leonardr> but what i believe happened was
<gary_poster> leonardr: ah.  I'm not familiar with a standard there.  I'd be inclined to stick with the same standard as our Python classes.  This is actually more of a reviewers meeting sort of thing IMO, and barry quite possibly would have been the tie-breaker in olden days
<leonardr> i guess at this point we might as well go with the de facto default
<leonardr> gary: the rationale is that people are going to be using launchpadlib from their third-party python apps that conform to pep8
<bac> leonardr, gary_poster: i can confirm that the decision was made when we started exporting things to be PEP8.  i'm surprised it isn't documented anywhere.
<leonardr> bac: i can't find it
<bac> it will be a good topic for the reviewer's meeting
<gary_poster> bac: ok, cool, I'm good with that.
<gary_poster> thanks
 * bac notes there is a lot of inconsistency, though.  :(
<gary_poster> bac, yeah, I think it might be nice for leonardr to provide some third-party consistency, going through the webservice after completing versioning and lower-level collections, while he verifies that what he has done is usable.
<salgado> bac, I have a branch which is about 1100 lines up for review, but should be an easy review.  any chance you can do take it?
<bac> salgado: is that the one gary_poster started?
<salgado> bac, yep, that's it
<bac> salgado: and gary_poster isn't going to finish it?
<bac> salgado: i'll be glad to take it later this afternoon if that is the case.
<salgado> bac, not today and probably not tomorrow as he's busy with RM tasks
<salgado> bac, that'd be great, thanks a lot!
<bac> cool.  just didn't want to duplicate effort
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [salgado] || 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: bigjools || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: i'm going to bump bigjools in front of you since his MP is short
<salgado> bac, fair enough
<sinzui> bac: I reviewed your branch. The bulk of my remarks are conversational. Do not panic at all the remarks
 * bac pulls hair out and sets it on fire
* bac 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 2010-01-27
<bac> salgado-afk: i tried to review your branch but didn't get far.  i tried running the branch referenced in the MP but +login-openid does not work on that branch.  I then tried grabbing your loom branch but got bzr errors.
<salgado-afk> bac, you should be able to try it out by merging from bzr+ssh://bazaar.launchpad.net/~salgado/launchpad/lp-as-openid-rp-for-ec2
<salgado-afk> in that branch the +login page uses OpenID, so +login-openid doesn't exist there
<bac> salgado-afk: got the branch
<bac> salgado-afk: running the new branch i still don't see +login-openid
<bac> salgado-afk: so, i'm confused as to what your two different branches do.
<salgado-afk> bac, +login-openid doesn't exist anymore -- in that branch I've already switched the +login page to use OpenID
<salgado> the branch you're reviewing just adds the OpenID provider, and the other one changes +login to use the OpenID provider
<bac> salgado: and neither support the URL you listed in the MP -- +login-openid?
<salgado> bac, the latter one used to, but I moved it over the existing +login
<bac> salgado: ok, i think i understand now.
<bac> +login on the branch you posted a second ago tries to work.  i need to add testopenid.lp.dev to /etc/hosts now to see it really work
<salgado> yeah, forgot to mention that, sorry
<bac> no biggie
<bac> salgado: missing cert
* 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
<jtv> Who's free to review an RC/CP branch?  https://code.edge.launchpad.net/~jtv/launchpad/bug-512698/+merge/18124
<jtv> flacoste, are you free to review a re-roll candidate?  It's just backing out another branch.  https://code.edge.launchpad.net/~jtv/launchpad/bug-512698/+merge/18124
<jtv> I've never backed out a branch before, so would like to be sure I get it right.
<flacoste> jtv: looking
<jtv> thanks!
<flacoste> jtv: looks good
<jtv> flacoste: thanks...  I can't explain the conflict, so you'll notice that's the one part I didn't back out.  Seemed safer; somebody else may have started using the same JS module.
<al-maisan> jtv: hello there! Are you still reviewing?
* al-maisan changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue [al-maisan<http://tinyurl.com/yag7j48>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* al-maisan changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [al-maisan<http://tinyurl.com/yag7j48>] || 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: Edwin || reviewing: - || queue [al-maisan<http://tinyurl.com/yag7j48>] || 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: Edwin, allenap || reviewing: -, - || queue [al-maisan<http://tinyurl.com/yag7j48>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> hello EdwinGrubbs or allenap : would either of you be willing to review the branch listed in the topic?
<allenap> al-maisan: I'll do it.
* allenap changed the topic of #launchpad-reviews to: on-call: Edwin, allenap || reviewing: -, al-maisan<http://tinyurl.com/yag7j48> || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> allenap: thank you!
<al-maisan> back in a few minutes..
* jamalta changed the topic of #launchpad-reviews to: on-call: Edwin, allenap || reviewing: -, al-maisan<http://tinyurl.com/yag7j48> || queue [jamalta] || This channel is logged:           http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> EdwinGrubbs, allenap: Let me know when you have a chance, just want to have a short pre-implementation chat about a trivial bug
<EdwinGrubbs> jamalta: I'm available
<jamalta> EdwinGrubbs: I'm looking at bug #118609
<mup> Bug #118609: "List related bugs" doesn't work on hosts other than bugs.launchpad.net <trivial> <ui> <Launchpad Bugs:Confirmed> <https://launchpad.net/bugs/118609>
<jamalta> First, I wanted to see what the suggested implementation would be. Adding +bugs to the link works as expected, but would that be the best solution? Or should the link go to the bugs sub-domain instead?
 * EdwinGrubbs checking
<jamalta> Secondly, would a test for that bug just check if the link for "List related bugs" is pointing to +bugs or the bugs subdomain (depending on the implementation taken)?
<jamalta> Only issue I see with using +bugs is that the link in the side menu is not active
<EdwinGrubbs> jamalta: hmm, it seems that the root of the problem is that going to http://edge.launchpad.net/~bjornt/+assignedbugs does not redirect you to bugs.edge.launchpad.net so the canonical_url() creates a link to edge.launchpad.net instead of bugs.edge.launchpad.net
<jamalta> EdwinGrubbs: So the solution should be to instead redirect the original URL to bugs.launchpad.net?
<jamalta> I had skipped that as a solution as it resolved correctly :) But seems fair enough
<EdwinGrubbs> jamalta: I think so. I'm looking up where the link is defined now.
<jamalta> EdwinGrubbs: Well the sidebar menu is in lib/lp/registry/browser/person.py
<jamalta> But the only way I know to get to that page is by clicking the "Bugs" tab which correctly moves you to bugs.launchpad.net, in which case all links will work correctly.
<jamalta> I was assuming that the use-case would be if the user typed the URL manually instead of following a link, although I could be very wrong.
<jtv> al-maisan: hi! sorry, long break, but yes.
<al-maisan> jtv: no problem, allenap was so kind to take a look at the branch in question.
<jtv> allenap: sorry for leaving you alone for so long.  There was nothing in my earlier day... what needs doing?
<al-maisan> jtv: do you mind if I review yours tomorrow morning .. my day is drawing to a close.
<jtv> al-maisan: not at all!
<al-maisan> jtv: thanks!
<allenap> jtv: Don't worry, I started after Edwin today; I was out this morning.
<EdwinGrubbs> jamalta: it looks like the best fix will be to change Link('', text, summary) to Link('', site='bugs', text, summary)
<jamalta> EdwinGrubbs: Ah, okay
<jamalta> EdwinGrubbs: That makes a lot of sense, I should add site='bugs' to all of the bugs link, though, right?
<jamalta> All the links within that class
<EdwinGrubbs> jamalta: It's not absolutely necessary, since those urls don't overlap with urls in other domains, but it would make it more consistent.
<jamalta> EdwinGrubbs: Right, that's why I wanted to consider that
<jamalta> EdwinGrubbs: Last question would be about testing
<jamalta> I'm assuming I need to write a test for this bug, right?
<jamalta> If so, should I simply pull out the link and make sure the URL is "bugs.launchpad.dev..."?
<al-maisan> allenap: sorry for bothering you but I corrected two comments; this may make it easier to read the branch: http://pastebin.ubuntu.com/363970/
<jamalta> I can't find any tests that deal with that link specifically
<allenap> al-maisan: Cool, thanks for letting me know.
<EdwinGrubbs> jamalta: that should be sufficient. You can add that test to lib/lp/registry/stories/xx-person-bugs.txt
<jamalta> EdwinGrubbs: thanks so much! I'll get working on that then :)
<allenap> barry: Out of interest, do you know if argument unpacking - i.e. def foo(a (b, c)): ... - is deprecated, generally frowned upon, or jolly good stuff?
<barry> allenap: generally frowned upon, surprising to many users, and (not sure about this) maybe gone in python 3?
<allenap> barry: Cool, thanks.
<barry> np!
<allenap> barry: Yes, actually, it's a SyntaxError in 3.
<barry> allenap: cool, that's what i thought.  thanks for checking it!
<jamalta> allenap: since Edwin is now out to lunch, would you care to review an MP for me? It'll be fun, I promise! :)
<allenap> jamalta: I'd love to, but I won't get to it until tomorrow; it's nearly my end-of-day here. I can take it, or you could ask tomorrow's OCR.
<jamalta> allenap: alright that sounds good.. thanks :)
<allenap> jamalta: Which one? OCR or me? :)
<jamalta> allenap: waiting for another reviewer
<jamalta> or waiting for you
<jamalta> whichever comes first :)
<jamalta> I have no problem waiting, heh
<allenap> jamalta: Okay, cool. Just to be clear, it's for bug 118609 right?
<mup> Bug #118609: "List related bugs" doesn't work on hosts other than bugs.launchpad.net <trivial> <ui> <Launchpad Bugs:Confirmed for jamalta> <https://launchpad.net/bugs/118609>
<jamalta> allenap: correct
<allenap> jamalta: Actually, it's short, I'll do it now.
<al-maisan> allenap: are you done with the other branch by any chance?
<jamalta> allenap: Alright, thanks!
<jamalta> It is pretty short, maybe 10 lines changed ;)
<allenap> al-maisan: I hope to have it done in the next 15 minutes.
<allenap> jamalta: Do you need someone to land that branch for you?
<al-maisan> allenap: great, thanks!
<jamalta> allenap: well, yeah i guess so :)
<jamalta> i always get confused at that question.. 
<jamalta> do you ask because some people land their own branches?
<jamalta> i'll fix the description in the test, thanks
<allenap> jamalta: Well, only some people have permission to submit the branches to PQM, which is what actually lands the branches on devel. I can land it for you. Can you either email me when it's ready, or ping me in the morning?
<jamalta> allenap: ah ok, i get it
<jamalta> now is ok with me, as long as you have time for it
<jamalta> i fixed the doctest per your suggestion
<allenap> al-maisan: Wow, your branch has a lot in it. I'm not finished yet, but http://pastebin.ubuntu.com/364018/ has my review so far. I've just got to TestMultiArchJobDelayEstimation, and I'll finish it in the morning.
<al-maisan> allenap: OK .. that's fine .. have a nice evening.
<allenap> al-maisan: And you, and sorry for not getting it all done.
<al-maisan> allenap: no problem .. I understand, it's quite a big branch.
<leonardr> edwin or allenap, can you review https://code.edge.launchpad.net/~leonardr/lazr.restful/generate-multiversion-collections/+merge/18153 ?
* sinzui changed the topic of #launchpad-reviews to: on-call: Edwin, allenap || reviewing: -, al-maisan<http://tinyurl.com/yag7j48> || queue [sinzui, jamalta] || This channel is logged:           http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> Edwin-lunch: allenap: I am jumping the queue because I have a branch that address the timeouts when creating a release
<EdwinGrubbs> sinzui: is it the release-timeout-bug-513321 you want me to review
<sinzui> yes
<EdwinGrubbs> sinzui: r=me. I wonder why we don't have an architecture for optionally queueing up notify() calls and processing them later.
<EdwinGrubbs> jamalta, leonardr: do you still need a review?
<sinzui> EdwinGrubbs: you mean "What happened to rabbitMQ"? I agree this is needed. This was a very rude surprise for me
<leonardr> edwingrubbs: yes, please
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/generate-multiversion-collections/+merge/18153
<EdwinGrubbs> sinzui: well, these are all internal messages, so they could easily be queued up in a db table by pickling the objects involved.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin, allenap || reviewing: leonardr, al-maisan<http://tinyurl.com/yag7j48> || queue [jamalta] || This channel is logged:           http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> EdwinGrubbs: no allenap reviewed the branch, i believe it has to be landed still, though.
<jamalta> i think allenap ran short on time, i'm not sure where that is at this point
<EdwinGrubbs> jamalta: since there was just a rollout, non-critical branches probably won't be able to be landed until Monday.
<jamalta> EdwinGrubbs: sounds good to me, thanks
<EdwinGrubbs> jamalta: I can do it for you, but you should probably ping me on Monday so I don't forget.
<jamalta> EdwinGrubbs: will jot that down on my calendar, thanks :)
<jamalta> and thanks for the help earlier
<jamalta> it's fun to learn so much from such trivial bugs :)
<sinzui> EdwinGrubbs: correct: Our struggle is that since we do not have that infrastucture, I need to proposed schema changes, land model changes, and isolate these on staging. Regardless of the labour, this this a 5 week fix and few users will see it for me to get good feedback :/
<sinzui> EdwinGrubbs: Can I resubmit my proposal against the correct branch? for an RC I should make it clear that I want to merge into db-devel
<sinzui> EdwinGrubbs: and the diff will be very small
<EdwinGrubbs> sinzui: yes.
<sinzui> EdwinGrubbs: https://code.edge.launchpad.net/~sinzui/launchpad/release-timeout-bug-513321/+merge/18160
<EdwinGrubbs> sinzui: done
<sidnei> an, there we go
<sidnei> anyone up for a lazr-js review?
<intellectronica> sidnei: sure, i'm happy to take it
<sidnei> intellectronica, awesome. it's pretty trivial actually. let me find the url
<sidnei> intellectronica, https://code.edge.launchpad.net/~sidnei/lazr-js/module-config-uses-min
<intellectronica> sidnei: i'd start the tuple on line 20 of the diff on the next line. your call but i find this a bit weird to read
<sidnei> intellectronica, good catch.
<intellectronica> sidnei: no other comment. patch looks great
<sidnei> intellectronica, cool. is lazr-js still using pqm?
<intellectronica> sidnei: i think so, yes
<sidnei> intellectronica, ok. i have to find the majik incantation again *wink*
<intellectronica> sidnei: and confusingly the trunk branch is called 'toolchain'
<intellectronica> sidnei: also, i think you'll have to wait until launchpad's pqm is open again. it's still in release-critical mode after the release
<sidnei> intellectronica, man, that really shouldn't block lazr-js
 * sidnei holds up on the angrymail
<sidnei> intellectronica, is this what you mean? https://pastebin.canonical.com/27105/
<intellectronica> i totally agree. time is high for us to start including lazr-js as a separate package and test it independently
<intellectronica> sidnei: yes, exactly
<intellectronica> makes it a bit clearer on first pass that you are concatenating tuples
<sidnei> yeah. it wasn't intentional. i refactored that line a couple times.
<EdwinGrubbs> leonardr: I got errors in multiversion.txt and field.txt tests.
<leonardr> edwingrubbs, can you paste the errors?
<EdwinGrubbs> leonardr: http://pastebin.ubuntu.com/364149/
<leonardr> edwin: i think you have a different version of something
<leonardr> edwingrubbs: yes, i believe you have an old version of simplejson
<leonardr> edwingrubbs: i have 2.0.9. what do you have?
<EdwinGrubbs> leonardr: you're right, I have an old version.
<EdwinGrubbs> leonardr: do you get an oops when you reload the mp? https://code.edge.launchpad.net/~leonardr/lazr.restful/generate-multiversion-collections/+merge/18153
#launchpad-reviews 2010-01-28
<leonardr> edwingrubbs: fwiw, yes
 * leonardr heading out
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: al-maisan<http://tinyurl.com/yag7j48> || queue [] || 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: - || reviewing: al-maisan<http://tinyurl.com/yag7j48> || queue [adiroiban(bug-509252)] || This channel is logged:           http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan || reviewing: jtv || queue [adiroiban(bug-509252)] || This channel is logged:           http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> hi al-maisan!  Thanks.
<jtv> al-maisan: want me to do adi's branch?
<al-maisan> Good morning jtv 
<al-maisan> jtv: if you like, I guess noodles775 will start reviewing at some point as well..
<noodles775> yep, I'll be there soon.
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan || reviewing: jtv || queue [adiroiban(bug-509252)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * jtv backs off
<al-maisan> hello jtv, errm, just wanted to let you know that a branch of mine (currently reviewed by allenap) does away with `IBuildFarmJobDispatchEstimation` altogether
<al-maisan> i.e. your branch that adds an implementation of that interface is not needed any more
 * jtv is silent as face progresses through a wild mix of moods and expressions
<al-maisan> sorry for the extra work you have put in but ..
<jtv> oh well :)
<al-maisan> .. a solution that allows us to remove `IBuildFarmJobDispatchEstimation` became obvious only recently
<jtv> That is good news though.  Did you go with the simulation as Tim suggested?
<al-maisan> jtv: not really but we now have enough data in the BuildQueue table to estimate dispatch times w/o having to look beyond that
<jtv> that's great... do you think you'll be needing me to provide more info for that data?
<al-maisan> jtv: not really as long as you set BuildQueue.processor, BuildQueue.virtualized and BuildQueue.estimated_duration
<jtv> al-maisan: I'm not sure I do though
<al-maisan> well, the defaults for BuildQueue.processor and BuildQueue.virtualized are NULL, so that should be fine
<al-maisan> However, the BuildQueue.estimated_duration property is required. Wasn't henninge working on that?
<jtv> No, I think I implemented that one as a hard-coded constant.
<jtv> I think he looked at it and found that the actual job there took essentially zero time.
<al-maisan> jtv: as long as the constant is close enough to reality that would be fine as well :)
<al-maisan> hmm .. so what is the constant he chose? Zero?
<jtv> I think I picked one minute...
<jtv> ...which is probably high.
<al-maisan> jtv: hmm .. that's a pretty conservative guesstimate
<al-maisan> jtv: are you storing the actual running time after the translation import job completes?
<jtv> I was thinking we could leave it at that unless and until we find that it's really affecting the reliability of estimates.
<al-maisan> jtv: I have no problem with that .. let's keep it for the time being.
<jtv> Isn't storing the running time something that's done in the generic buildmaster code?
 * al-maisan looks
<allenap> al-maisan: What does EXTRACT(EPOCH FROM ...) do?
<al-maisan> allenap: it converts the interval to seconds
<allenap> al-maisan: Cool.
<al-maisan> allenap: http://www.postgresql.org/docs/8.1/static/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
<allenap> al-maisan: Awesome, thank you.
<al-maisan> you are welcome :)
<al-maisan> jtv: you were right, the actual running time of the job is stored in BuildBase.storeBuildInfo()
<jtv> al-maisan: common sense, design-wise, to keep that generic.  :)
<al-maisan> jtv: yup, but common sense is not at all that common ;)
<jtv> true, alas
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan || reviewing: - || queue [adiroiban(bug-509252)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> adiroiban: your mp is reviewed & approved already (https://code.edge.launchpad.net/~adiroiban/launchpad/bug-340662-take-2/+merge/17598)
<al-maisan> is that correct?
<al-maisan> adiroiban: can I remove you from the queue?
<adiroiban> hm
<adiroiban> let me check
<adiroiban> it is for bug 509252
<mup> Bug #509252: Remove AdminPoTemplateSubset from security.py <cleanup> <Launchpad Translations:New for adiroiban> <https://launchpad.net/bugs/509252>
<al-maisan> adiroiban: sorry .. I clicked on the wrong link
<adiroiban> np
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan || reviewing: adiroiban || queue [] || 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: al-maisan, noodles775 || reviewing: adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<al-maisan> adiroiban: looks good, r=me
* al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> al-maisan: thanks! Do you have time to land it or should I ask someone else?
<al-maisan> adiroiban: I believe we are in RC mode .. but I can land it once 10.02 opens up
<adiroiban> al-maisan: sure. I was refering to land it then the queue will be open... probably it would be best to ask you when the queue is open
<al-maisan> adiroiban: thanks!
<allenap> al-maisan: I've finally finished your review. Lots of minor suggestions, nothing major. Looks good :)
<al-maisan> allenap: great, thanks!
* adiroiban changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: -,- || queue [adiroiban(bug-462013-try-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adiroiban: I can take a look at that branch, or did you want someone specific to look at it?
<adiroiban> noodles775: hi. nope. you can review it
* noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: -,adiroiban(bug-462013-try-2) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adiroiban: hi, your new test seems to pass even when I revert the change to product.py?
<noodles775> (BTW: thanks for adding extra tests too!)
<adiroiban> noodles775: uh... yes. I forget to test âobsolete is _not_ in extracted_textâ
<adiroiban> noodles775: I pushed the missing test
<noodles775> adiroiban: thanks... I'm actually looking at the pagetest, and thinking that most of it is really testing the *view* functionality and should really be in a view/browser test...
<noodles775> I mean, you're really testing the untranslatable_series property... what do you think?
<adiroiban> noodles775: yes. I will move them in browser/tests
<noodles775> adiroiban: So perhaps just add one series in the pagetest and verify that it appears, then yes, the rest can go in the view test.
<adiroiban> noodles775: or 2...one active and one obsolete
<noodles775> adiroiban: also, I (personally) think it would be worth sorting the results of untranslatable_series - random order is not good. What do you think?
<noodles775> adiroiban: well, the fact that the obsolete series won't be included should be in the view test (as it's view code).
<adiroiban> noodles775: ok. alphabetically sorted?
<noodles775> adiroiban: if that make the most sense, yes (another option would be by the status, but you'll know more about which makes most sense :)
<noodles775> adiroiban: So I'll make this as needs-fixing for the moment. Just ping me when you're done and I'll go through the code in detail. Thanks!
<adiroiban> noodles775: ok. thanks
<beuno> kfogel, any magic trickery available to load the sample data?
<beuno> ro should I just clean the db and run the SQL?
<kfogel> beuno: in standup, will answer after (inability to multitask, is all)
<beuno> thanks
<kfogel> beuno: most ppl in #launchpad-dev should know, though; I recall it's pretty easy to load -- abel has shown me how before
<kfogel> beuno: of course, abel is in this standup with me :-)
* al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: adiroiban(bug-462013-try-2) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<kfogel> beuno: okay, so I *think* you just do 'cd database/schema; cp current-dev.sql MY_SAFE_current-dev.sql; cp newsampledata-dev.sql current-dev.sql'
<kfogel> beuno: and then a 'make schema' may be necessary
<beuno> kfogel, sounds like what I want, thanks, I'll give it a go
<kfogel> beuno: might want to check with someone that that last step is both necessary and safe
<beuno> kfogel, it's ok, I have a helmet on
<kfogel> beuno: that is, there's also the "current.sql" file; I'm not sure what it is or whether 'make schema' blows it away, though I doubt 'make schema' is destructive to any file on disk (its whole point is to be destructive to the db)
<kfogel> ok
<beuno> kfogel, no padding between the borders and the content of the tooltips!  :)
<adiroiban> noodles775: I'we pushed the changes. Hope you got the latests diff in your mailbox :)
<noodles775> Thanks adiroiban, I'll take a look shortly.
<kfogel> beuno: need padding?
<beuno> kfogel, the content is squished against the border
<intellectronica> beuno, kfogel: b.t.w don't forget that there's a lazr widget wanting to happen there. we didn't take this route to being with because we thought it's better to first have a concrete implementation, but that's already the second implementation in LP and i'm sure other apps would benefit from this too
<intellectronica> i'm talking about the fancy tooltip b.t.w
<kfogel> intellectronica: I *had* forgotten, actually, thank you for reminding me.  IOW, we want to abstract out the tooltip functionality?
<intellectronica> yes, we want to abstract the tooltip functionality using a YUI widget, and put that in lazr-js
<intellectronica> it will be hard to find resources to take on this task, but just recording it for now will already be a step forward
<kfogel> intellectronica: would an XXX comment with a bug be appropriate?  (I could put the XXX comment both in lib/lp/bugs/templates/bugtarget-patches.pt and in the appropriate place for the other spot we're using a fancy tooltip right now).
<beuno> kfogel, change div.popupTitle{padding:0 1em;} to padding:0.5em 1em
<intellectronica> kfogel: yes, i think an XXX with a bug would be appropriate. the bug should probably be on lazr-js (rather than launchpad)
<kfogel> beuno: I was just about to ask you how to do that, thank you :-)
<kfogel> intellectronica: *nod*
<kfogel> beuno: trying it now
<kfogel> beuno: http://paste.ubuntu.com/364621/
<kfogel> beuno: something like that?
<kfogel> beuno: (it appears to have the desired effect in my launchpad.dev instance)
<beuno> kfogel, yes, but it's better if you do that on the css file
<kfogel> beuno: oh, right -- popupTitle is a shared class
<kfogel> one sec
<kfogel> beuno: I did this http://paste.ubuntu.com/364623/, but is there some way to do a page reload that will reload the CSS file?  I'm not seeing the new padding now.
<beuno> kfogel, IIRC, you need to run make again (yuk!)
<kfogel> beuno: oh, to make combo.css?
<beuno> yeah
<kfogel> right, forgot about that
<kfogel> cool, doing now
<kfogel> beuno: while that's going on, any other UI tweaks?
<beuno> kfogel, triple tasking, so I can't say for sure
<beuno> the drop down looks odd
<kfogel> beuno: ...can you be more specific?  Should we get rid of the little two-headed lizard icon? :-)
<beuno> kfogel, I haven't gotten to the suggestion part
<beuno> it just floats there
<beuno> I'm trying to think what to do with it while I explain to the team I'm leaving what I did this morning  :)
<kfogel> beuno: no rush -- come back to this later if triple-casking is killing you.  Excuse me, I mean "triple-tasking" (cough, cough).
<beuno> :)
<beuno> sinzui, how's the TL call?  I miss everyone
<sinzui> beuno:  join in 
<beuno> about to!
<beuno> kfogel, bug icons don't match the importance
<beuno> and lastly, I'd add an "Order: " label to the ordering drop down
<beuno> I'll add all these things to the MP
<kfogel> beuno: oh, that's right, bug icons get colored according to importance, I totally forgot.
<kfogel> Thank you.
<kfogel> I'm not actually color-blind, but I might as well be -- I never notice that stuff.
<beuno> heh, it gets better when looking at other peoples' stuff
* noodles775 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
* rockstar 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
<leonardr> rockstar, i have a fun branch for you
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/enforce-version-order/+merge/18223
<leonardr> ("if it was fun to write, it should be fun to understand")
<leonardr> i'm taking my wife out to lunch, so if you run into problems and need my input, just kick the branch back to me. but it should be pretty self-explanatory
<rockstar> leonardr, okay.  I'm also heading out to run an errand.  I'll get to it when I get back.
<leonardr> all right
<henninge> Hey jtv, I hope you didn't find my review too harsh ... ;)
<henninge> jtv: Also, what are you doing up this time of night?
<jtv> henninge: wrapping up a few things I couldn't do in my own timezone.  :-)
<jtv> henninge: no worries about the review... I would say the reason this is (slightly) translations-specific is that we specifically check for read-only mode and make decisions based on it, in this case leading to an assertion failure.  Other requests will mostly just fail with a "transaction is read-only" database error, which is easy enough to filter out.
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> rockstar, fwiw, i'm back
<rockstar> leonardr, cool.  Looking at your branch now.
* leonardr_ 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
#launchpad-reviews 2010-01-29
<stub> I want to land https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/18198 today so I can turn on the new logging on edge on the weekend.
<stub> rockstar: ^^
<stub> jtv1: And I even used real ISO8601 date formats without a / to be found!
<jtv1> stub: I'm so proud
<jtv1> But I suppose this is your way of asking for a review.
<stub> I'm trying subtlety before resorting to abuse and tantrums.
<jtv1> Hmm... actually I think I'll hold off.  I'd like to see the tantrums.
<jtv1> stub: I haven't reviewed code from you for a while...  the difference in coding style is like night and day.
<jtv1> stub: a few small things though: line 60, in the list of date/time formats, we require a trailing comma.
<stub> That is either coffee intake, or that this is greenfields rather than extending existing code. Most of the time I seems to be wading about the guts of something horrible.
<stub> (ie. code I wrote 5 years ago)
<jtv1> Ah yes... as I keep trying to tell people, we can argue all we want about the intricacies of deterministic coding style, but existing code is a much stronger persuader than guideline #1728.317 sub b.
<jtv1> I once tried to implement my own optparser types as well, though wasn't very successful at it... it'd be really nice to have our own little library of option types, e.g. DistroSeries.
<stub> That code is pretty much verbatim from the Python reference
<jtv1> ah so
<stub> There is an example under 'extending optparse'
<jtv1> On an unrelated note: in pageperformancereport.py, everything needs docstrings.
<stub> Can I leave them until later? Main goal of this particular landing is to get the URL logged in the trace logs
<stub> (I can land just that change if necessary).
<jtv1> Later never comes, does it?
<stub> I'm still tweaking that script - just added support for compressed logs and malformed lines.
<stub> TypeError: A() takes exactly 4 arguments (149 given)
<stub> hehe...
<jtv1> Very helpful...
<jtv1> The docstrings can be quite short, as long as they place things in context.  F'r instance, Category could do with a one-liner "Category of <whatever it is you're categorizing>."
<stub> Ok. I'll add docstrings.
<jtv1> Thanks.  Also makes reviewing easier.
<jtv1> Another thing: scripts don't normally follow the entire python file template AFAIK...  for one, they start out by importing _pythonpath.
<jtv1> BTW is it worth reporting the number of malformed lines, to help detect problems in recognizing record types?
<jtv1> Line 365, instead of "for i in range(0, len(histograms)): histogram = histograms[i]" use "for i, histogram in enumerate(histograms)"
<stub> k
* adeuring 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
<jtv1> stub: in "a list of Category, in report order", what does "in report order" mean?
<stub> The order we want things displayed in the report.
<jtv1> stub: so that's why you wanted an order-preserving config?
<stub> Yes. I need to add an order= parameter to the config or a different config format entirely so we can put important stuff first.
<stub> And group similar areas etc.
<jtv1> I see... good call on leaving it out, I guess
<jtv1> stub: I think parse() would be more manageable if at least the loop for reading one file was in a separate function...  less indentation, but also you won't need the "continue # skip to next line" and "break # skip to next file" stuff
<stub> jtv1: Then I lose an entry that spans two log files
<stub> (although maybe that doesn't matter for statistical analysis)
<jtv1> you can have entries that span two log files!?
<jtv1> Frankly though I don't see how you'd lose anything at all by doing it that way
<jtv1> Unless you've got some loop-carried variable definitions hidden in there, which would be icky.
<stub> Your right. I don't lose.
<jtv1> btw I now see that "requests" only holds ongoing requests...  it'd have helped to make that clear.
<jtv1> stub: what does the "$(" before the JS do?  I don't think you're closing that parenthesis.
<stub> jtv1: Some jquery thing I don't understand. The basic structure was cargo culted from examples.
<jtv1> stub: I'm getting old.  I find myself tempted to ask "wouldn't it be easier to do this with XSLT?"
<stub> The answer to that question is almost always 'no'. And when you are on a team where XSLT knowledge is nearly zero, it is almost certainly 'no'.
<jtv1> Maybe we're all getting old.
<jtv1> Anyway, please make sure that parenthesis is closed.
<jtv1> Oww, you're importing main into a script elsewhere.  In that case, it should've been in the __all__ where you export it.
<stub> k
<stub> I need to replace parsedt with something that isn't broken (it fails when there are no milliseconds)
 * jtv1 says Hail Mary in penance for assuming that it'd be called where it was defined.
<jtv1> You need to test stuff as well.  We live in Thailand; we don't want to fill our even virtual toolboxes with utensils that fall apart when we finally need them.
 * jtv1 had another "most solid-looking in the shop" piece of crap spontaneously fall apart after about a week
<jtv1> Made out of solid, chromed steelâexcept the moving parts which are chromed plastic.
<jtv1> stub: about the config... that's for production only?
<stub> Its an example for now. The report is still a work in progress really. People will use their own configs for what reports they want to generate.
<stub> But we need the logging bit landed so we can get some real data to test the report script against. 
<stub> Even without the URLs, I'm finding lots of issues with the parser I'm having to fix now.
<stub> (using the existing trace files, sans URLs with virtual host information)
<jtv1> stub: I have to go do stuff now... should give you some time to do that.  :-)
<stub> I won't have time to sort that today. I want to land the logging change (the important bit I want to land so I have logs to test against on monday, allowing me to complete the report script)
* salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> salgado: https://code.edge.launchpad.net/~stub/launchpad/trivial/+merge/18258 (trivial)
<jtv> stub: re-doing the whole thing?
<stub> The report will change enough. Changed a bit already.
<stub> The report isn't the important thing though since it is a work in progress
<stub> And there is little point polishing it until we can run some real data through it. I'm just working on memory issues now.
<jtv> rockstar: "nag"
<bac> beuno: we talked about this new portlet earlier in the week and it is my understanding you were happy with it.  could you mark a UI review on it?  https://code.edge.launchpad.net/~bac/launchpad/bug-490518-link-suggestion/+merge/18040
<beuno> bac, sure, ine sec
<bac> beuno: got a sec yet?  :)
<beuno> bac, ah
<beuno> it seems I lied
<beuno> really opening up the MP this time
<bac> stretched the truth
<bac> had good intentions
<beuno> bac, any screenshots I can look at?
<bac> beuno: just the same old ones from earlier.  let me dig them up
<bac> http://people.canonical.com/~bac/linkup-multiple-ff.png
<bac> http://people.canonical.com/~bac/linkup-none.png
<bac> http://people.canonical.com/~bac/linkup-one.png
<bac> beuno: recall the spacing issues are a webkit problem and a bug has been filed for it
<beuno> ah
<beuno> I remember now
<beuno> approved\
<bac> you the rock
<beuno> I think it's more of a balad today
* salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> salgado, A review. I am needing one.
<salgado> rockstar, how big is it?  I'll need to leave soon
<rockstar> salgado, it's not very big, but is a big priority.
<salgado> rockstar, ok, where is it?
<rockstar> salgado, it's the branch to fix the problems detailed in abentley's Root Cause Analysis email.
<rockstar> salgado, finishing letter now.
<rockstar> salgado, https://code.edge.launchpad.net/~rockstar/launchpad/scanner-events/+merge/18281
<salgado> rockstar, care to tell me a bit about IBranchScanJobSource and how it is tied together to the rest of the branch scanning system?
<rockstar> salgado, so there is the IBranchScanJob and IBranchScanJobSource - I'm not sure why we have two interfaces, but one is for instances usually, and the other is for classmethods.
<rockstar> IBranchScanJobSource specifically gives you all the methods you need to get all the BranchScanJobs, etc.
<abentley> rockstar: The two interfaces are because stateful things don't really work as utilities.
<deryck> salgado, I have one for you, as you have time, please.
* deryck changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> deryck, I won't have time today, sorry.  reviewing one for rockstar now and I'll have to leave after that
<deryck> salgado, no worries.
<deryck> salgado, thanks anyway!
<rockstar> All BranchJob types have a wrapper around them that provides extra information.  BranchScanJob is the class in this case, and it has no extra information needed.
* deryck changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> yo rockstar !
<rockstar> deryck, I was just about to offer.
<rockstar> :)
<deryck> :-)
<deryck> you da man!
<deryck> rockstar, https://code.launchpad.net/~deryck/launchpad/not-implemented-errors/+merge/18283
* salgado 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
<salgado> rockstar, ISTM that .run() now expects self.server to be not-None, but do we have anything to make sure contextManager() is called before run(), to make sure self.server is set?
<rockstar> salgado, it's actually cls.server, but yes, the JobCronScript deals with all of that.
<rockstar> salgado, it even uses the context manager stuff that barry posted to the list about.
<salgado> rockstar, would it be worth having an "assert self.server is not None" in run(), just to make it clear?
<rockstar> salgado, well, run is called in a with statement, so it would bomb out before then.
<salgado> also, what do you think of a test showing that all the expected subscribers are actually registered for the event?  the existing one only shows (implicitly) that the subscriber for generating diffs is registered, but not the others
<rockstar> AIUI, the with is what handles the contextManager call, and knows how to clean up after itself.
<rockstar> salgado, well, I don't think this fix is the way forward.  abentley, thumper, and I are all in agreement there.  It's the how we're not quite sure of.
<abentley> salgado: This isn't a nice fix, this is a quick, minimally-invasive fix.
<salgado> ok, maybe that'll be clear for someone reading the whole thing and not just the diff, but maybe a comment explaining how we know self.server will be set before run() is called would be nice
<rockstar> salgado, maybe, but this isn't the only place that uses the contextManager.
<salgado> fair enough, I leave that up to you
<rockstar> If you're implementing JobCronScript, you need to know about contextManager and how it works.
<salgado> but if you're just reading the code, it might take you longer than necessary to understand it
<rockstar> deryck, r=me
<deryck> rockstar, you rock!  thanks
<salgado> rockstar, well, since this is more like a quick fix, then, I guess I'm happy with it.  my only real concern was with the test, but I see you guys plan to address that
<salgado> r=me
<rockstar> salgado, yes, we definitely plan to address it.  We'll need to chat with gary about it though.  I'm sure he'll have some good insights.
 * gary_poster likes chatting :-)
<abentley> gary_poster: I think this is what jml wanted to talk with you about, too.  Basically, we want to make sure we're using Zope events the best way for us.
<rockstar> gary_poster, you're probably also hating packing right now.  :)
<gary_poster> abentley: gotcha.  rockstar: eh, I'll throw whatever in a suitcase.  More worried that the predicted 10 inches of snow will bring NC to a standstill tomorrow. :-)
<rockstar> gary_poster, 10 in is NC? You bet it will.
<rockstar> Maybe you can convince bac to give you a ride in one of his Land Rovers.  :)
<gary_poster> yeah.  I'm hoping it won't really do that.  If it does, I may have an extra day at home
<gary_poster> heh
<bac> gary_poster: i'll come get you!
<bac> rockstar: you taunt.  you know only one works...
<rockstar> bac, yea, and it sounds like the floor might be suspect.  :)
<bac> rockstar: nah, the floor is mostly fine.  it's the frame.  :(
<gary_poster> bac, ha, don't offer too often or I might take you up on it ;-)  I've been wondering if there were a we'll-drive-in-snow taxi service around here.  Depending on how bad things look I might look into it.  Not keen on piling the family in to a NC snow drive to the airport if things look bad.
<bac> ah, i forgot you have to fly out
<bac> gary_poster: the new terminal looks very comfortable for spending the night if you get snowed in
<gary_poster> bac, thanks ;-)
