#launchpad-reviews 2010-04-19
* jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775,adiroiban(bug-564852), jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> No volunteers for a review today?
<henninge> jtv: Let's swap: https://code.edge.launchpad.net/~henninge/launchpad/bug-516736-templatename/+merge/23659
<jtv> henninge: OK!
<jtv> henninge: s/\<otemplate/potemplate/
<jtv> henninge: also, it may help to introduce a variable in the last paragraph of the diff:
<jtv> potemplate = self.context.potemplate
<jtv> if potemplate is None:
<jtv>     # ...
<jtv> else:
<jtv>     # Do stuff with potemplate, not self.context.potemplate
<henninge> jtv: yeah, nice idea. Although one line lost, one line gained...
<henninge> jtv: how did you "tighten the check a bit more" ?
<jtv> henninge: besides just counting the number of forms, I'm also checking that they are the right forms.  The old code would not detect the problem with nplurals=2 and "n ? 0 : 2" for instance.
<henninge> oic
<jtv> henninge: anyway, I'm approving yours.
<henninge> jtv: is it this?
<henninge> 144	- if pluralforms_count != len(forms):
<henninge> 145	+ found_forms = sorted(forms.keys())
<henninge> 146	+ if found_forms != range(expected_forms):
<jtv> henninge: right
<henninge> cool
<henninge> jtv: thanks
<jtv> It may seem wasteful at first to do a range comparison, but this is only over the number of forms anywayâmaximum of 6
<henninge> jtv: I just have two small things, I'll mention them in the review. r=me
<jtv> henninge: dankeschÃ¶en
<jtv> ahem
<henninge> doppelt gemoppelt
<jtv> Ã¶n
<henninge> ;-)
* henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775,adiroiban(bug-564852)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: review sent
<jtv> thanks
<sinzui> BjornT, ping
<BjornT> hi sinzui
<sinzui> I am not certain from your remarks about the not-packaged-1 branch whether I should try a different name for a new column, or proceed using Packaging
<BjornT> sinzui: it depends on your use case. if you actually want to record that something isn't packaged, and display this information in the ui, go for the Packaging table.
<sinzui> BjornT, regarding the enum, we hide the INCLUDE enum a few months ago because project owner do not understand it. It is used for some Debian and Ubuntu packages. While it is correct for modeling, it does not provide a lot of value for the project or ubuntu contributors
<BjornT> sinzui: if all you want to do is to control whether to ask the user whether it's packaged, go for the column with a different name
<sinzui> I will choose a different name since this is really about a portlet behaviour
<BjornT> sinzui: ok, cool
<adiroiban> danilos: Hi, when you have some time can you please take a look at the current interface proposed for bug 512735 and have a short preimplementation chat?
<mup> Bug #512735: At first diplay only preferred languages for a POFile from a POFileSubset <tech-debt> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/512735>
<danilos> adiroiban, sure
<danilos> adiroiban, should I look at the branch?
<adiroiban> yes
<adiroiban> danilos: there is an IHasLangaugesWithTranslations interface
<danilos> adiroiban, right, looking
<adiroiban> and an implementation of language-statistics view
<danilos> adiroiban, so, I am not sure this is quite enough; this is definitely a step in right direction though
<adiroiban> danilos: what is not enough? the fields from the interface?
<danilos> adiroiban, yeah, I am not sure that's completely enough
<danilos> adiroiban, still looking though :)
<danilos> adiroiban, the biggest concern is performance
<danilos> adiroiban, also, I suspect you based the generic languages-translations-statistics.pt on one of distroseries or productseries templates
<danilos> adiroiban, it would have been much easier to read that if you moved one of them to it
<danilos> adiroiban, also, has_translations and languages_statistics should probably be @properties
<adiroiban> danilos: true. my bad for not moving one of those files. And yes, those should be properties
<adiroiban> danilos: the idea is that there will a lot of changes, so I should start by implementing an interface for distroseries and productseries and commit that one
<adiroiban> and then see what other fields are required by sourcepakage and potemplate and add them later
<danilos> adiroiban, sure, sounds good, that's why I am complaining about the templates, because that'd be the easiest way for me to see if this is indeed enough for the first go :)
<danilos> adiroiban, btw, LanguageStatisticsView should definitely not be in browser/language.py :)
<adiroiban> danilos: agree, I was not sure where to put it. Should I add it in a new file?
<danilos> adiroiban, sure, please do... it's also not really "language_statistics", it's "per-language statistics"
<danilos> adiroiban, so, I think it looks good in general, I don't like the naming for many things though :)
<adiroiban> danilos: why do you think there will be performance issues?
<danilos> adiroiban, well, we have to be careful about how many DB queries are executed and how many Storm objects we instantiate
<adiroiban> is there an easy way of profiling sql query numbers and created storm objects ?
<danilos> adiroiban, so, if we don't have everything we need exposed in the interface and we have to do something like go through all languages to get a total of something, we are a bit screwed (i.e. timeouts will happen)
<adiroiban> yes, but those statistics are already generated
<danilos> adiroiban, eg. imagine we need to get a total of translated and untranslated messages here; with this interface, we'd have to go through all languages and add them up
<adiroiban> is just that there is a lot of duplicate code
<danilos> adiroiban, right, I am just giving an example of what kind of things to watch out for
<danilos> adiroiban, and that's the kind of thing we'd need to watch for when designing the interface
<danilos> adiroiban, I am not pretending to be too smart this late in the day, though, so take that into consideration :)
<danilos> adiroiban, anyway, if we can't think of anything else, I mostly have complaints about the naming :)
<adiroiban> :)
<danilos> adiroiban, not that I can come up with good names right now
<adiroiban> I will try to rename then and will come back with a full implementation for distro and product series
<danilos> adiroiban, sure, sounds good
<danilos> adiroiban, the important distinction is that these are not languages, as I said above, but instead translations... perhaps using "translations" for the property name on the interface would be better
<adiroiban> danilos: true... i was thinking language_translations
<adiroiban> to denote that those translations are per language
<danilos> adiroiban, right, I am struggling with coming up with good naming for this :)
 * jpds has https://code.launchpad.net/~jpds/launchpad/fix_565345-mirror-prober-agent/+merge/23708 up for review.
* jpds changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775,adiroiban(bug-564852),jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-04-20
<abentley> thumper, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/append-distroseries/+merge/23728
<wgrant> abentley: Appending just the distroseries name is very unusual.
<wgrant> Was that was james_w suggested?
<abentley> wgrant, it is.
<wgrant> (normally something like ~series1 is used)
<abentley> wgrant, okay, I may have gotten that bit wrong.
<abentley> wgrant, thumper: changed and pushed.
<wgrant> abentley: I'm not a fan of the s/distroseries/distroseries_name/, but from a packaging PoV that looks good. Thanks.
<thumper> wgrant: please comment on the merge proposal :)
<thumper> wgrant: in the future rather than just IRC
<thumper> wgrant: otherwise we can miss it
<abentley> wgrant, thanks for catching it.  I'd forgotten exactly what james_w suggested.
<wgrant> thumper: True. Sorry.
<thumper> wgrant: consider it practise for being a real reviewer :)
<mwhudson> thumper: this is the real interdiff http://pastebin.ubuntu.com/418954/
<abentley> thumper, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/build-security/+merge/23741
<noodles775> Hey henninge! Will you get a chance to lok at my branch today?
<noodles775> No problem if not, just trying to clear our kanban lane :)
<henninge> oh, it's Tuesday ...
<henninge> noodles775: sure, at some point ... ;)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [noodles775,adiroiban(bug-564852),jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Great, thanks.
<gmb> henninge, Good morning. Mind if I add https://code.edge.launchpad.net/~gmb/launchpad/reschedule-button-qafix-bug-558415/+merge/23698 to your queue?
<henninge> gmb: ah, yes, but I am working on a critical bug atm
<gmb> henninge, Okay. There's no rush; bac's reviewing this afternoon isn't he?
<henninge> gmb: should be, yes
<gmb> henninge, Okay. I'll put it on the queue but I'm not worried if you or bac don't get to it for a while.
* gmb changed the topic of #launchpad-reviews to:  On call: henninge || reviewing: - || queue: [noodles775, adiroiban(bug-564852), jpds, gmb(http://is.gd/bAr7Y)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* mrevell changed the topic of #launchpad-reviews to:  On call: henninge || reviewing: - || queue: [noodles775, adiroiban(bug-564852), jpds, gmb(http://is.gd/bAr7Y)] mrevell (http://is.gd/bAtGL) || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos: when you have some time, can you take a look at the initial commit and have a pre-implementation chat for bug 146178?
<mup> Bug #146178: Add links to latest full and delta language pack exported <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/146178>
* sinzui changed the topic of #launchpad-reviews to:  On call: henninge || reviewing: - || queue: [noodles775, adiroiban(bug-564852), jpds, gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui] || 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: henninge, bac || reviewing: - || queue: [noodles775, adiroiban(bug-564852), jpds, gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * bac looks at the queue.  girds himself for a long day of reviewing.
 * noodles775 feels for bac :/
<bac> don't mind me...just whining a bit
* bac changed the topic of #launchpad-reviews to:  On call: henninge, bac || reviewing: -,noodles || queue: [adiroiban(bug-564852), jpds, gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> noodles775: r=bac.  thanks.
* bac changed the topic of #launchpad-reviews to:  On call: henninge, bac || reviewing: -,adiroiban(bug-564852) || queue: [jpds, gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> bac: Great, thanks.
<danilos> adiroiban, I'm looking at it
<adiroiban> danilos: I have created a preliminary MP for the branch ... if that helps
<danilos> adiroiban, I've commented on the bug there, probably should have done it on MP as well :)
<bac> danilos: are you doing the review?
<adiroiban> danilos: ok. I will copy the commen. I have also commited a working implementation of IPerLanguagesWithStatistics https://code.edge.launchpad.net/~adiroiban/launchpad/512735/+merge/23759
<danilos> bac, no, the preimp chat :)
<danilos> adiroiban, that's the worst naming ever though :) I'll be happy to review that one
<adiroiban> :)
<jtv1> henninge, bac: can I put a small one on the queue?  https://code.launchpad.net/~jtv/launchpad/bug-567035/+merge/23742
<adiroiban> danilos: for the language-packs links text, should we use UTC or user timezone?
* henninge changed the topic of #launchpad-reviews to:  On call: bac || reviewing: adiroiban(bug-564852) || queue: [jpds, gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bac, jtv1: I am sorry, but I won't be able to take any reviews today.
<jtv1> henninge: still on that critical bug?
<henninge> yup
<jtv1> and that looks like a pretty full queue.
<jtv1> So maybe I should wait until tomorrow, when I'm OCR.  :)
<bac> henninge: ok
<jtv> bac, need any help with that queue?
<bac> jtv: i'm not going to say no
<bac> jtv: but i have a long day ahead of me so it'll be ok
<bac> adiroiban: so is your branch for bug 564852 read for review or are you still making changes?
<jtv> bac: I'll see if I can steal a short one then.  :)
<adiroiban> bac: is ready
<bac> adiroiban: ok
<adiroiban> bac: if a MP is not ready i'm setting its status to 'work in progress'
<jtv> bac: I'm looking at the first one on the queue.
<bac> adiroiban: right.  i was just concerned that it was ready for review but you and danilos seemed to be having an active discussion about the branch
<bac> jtv: jdps?  if so, please remove it from the topic
<adiroiban> bac: ah...my discussion with Danilo is about another branch
<bac> adiroiban: ok.  thanks.
* jtv changed the topic of #launchpad-reviews to: On call: bac || reviewing: adiroiban(bug-564852) || queue: [gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> adiroiban: +1 for using lp-send to create your MP.  much appreciated.
<adiroiban> bac: I'm not using lp-send ... just the LP web ui. What are the advantages or using lp-send?
<bac> adiroiban: it provides the skeleton you used in https://code.edge.launchpad.net/~adiroiban/launchpad/bug-564852/+merge/23612 and automatically includes the lint
<bac> adiroiban: much easier than doing it by hand and ensures you don't just forget a section
<adiroiban> bac: I see. Well I have a script that generates that output and copy it into the clipboard, but I will try to install lp-send and try to use it
<bigjools> bac: hi, do you know Twisted?
<bac> bigjools: i do not.
<bigjools> bac: you're not going to like my branch then
<bigjools> you can learn a bit if you want? :)
<bac> bigjools: cool.  put it on the end of the queue...maybe i'll get to it, maybe i won't.  :)
<bigjools> hah :)
<bac> bigjools: i'd like to look at it.  if i have to punt i'm not shy
* bigjools changed the topic of #launchpad-reviews to: On call: bac || reviewing: adiroiban(bug-564852) || queue: [gmb(http://is.gd/bAr7Y), mrevell (http://is.gd/bAtGL), sinzui, sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> bac: it's a complicated change in the buildd-manager
<bigjools> bac: but I'm happy to explain over a call if you want
<bac> bigjools: either way.  if you'd rather get a fellow soyuzian to look at it now i won't be offended
<bigjools> bac: it's not partiocularly Soyuzy apart from the build farm aspect, which is not that soyuzy any more :)
<bigjools> bac: I'm happy to wait
<bigjools> thanks
<bac> np
<jpds> jtv: it was salgado who suggested the sendHeader() trick..
<jtv> jpds: the trick itself is fine, just don't rely on a specific offset in the array!
<jtv> jpds: you could also use an "in" check, but that's fragile because of the whitespace.
<jtv> And since this is key/value pairs anyway...
<bac> adiroiban: in language-views.txt there is the claim that Language.translators is monkey patched but I cannot find where that happens
<adiroiban> there should be someting like "del Language.tranlators"
<adiroiban> line 92
<bac> adiroiban: ok, so that deletes the method.  i expected to see it replaced.
<bac`> adiroiban: nm, i see it now
<adiroiban> bac`: it is replaced at line 153
<adiroiban> it is a property and I was not able to patch the object
<adiroiban> as the property did not defined any "deleter"
<bac> adiroiban: thanks
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: gmb(http://is.gd/bAr7Y) || queue: [mrevell (http://is.gd/bAtGL), sinzui, sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi mrevell
<mrevell> hi bac
<bac> mrevell: i'm looking at your MP and notice two things before starting the review.  one you are linked to a bug that is fix released, which is odd.  and second you're targeted to db-devel, not devel.  is that intended?
<mrevell> bac, The bug -- I'll update, I forgot it was still in that state. I thought the fix had been released but when I looked at the tour it appeared that it hadn't.
<mrevell> bac, As for db-devel, that is not intentional. Thank you for pointing that out.
<bac> mrevell: i was burned by that last week...
<bac> yet another reason i'm continually pimping 'bzr lp-send'
<mrevell> bac, I haven't used lp-send yet, so I'll give that whirl.
<bac> gmb: did you intend to target your MP to db-devel?
<gmb> bac, Yes.
<bac> ok
<gmb> bac, Or... Maybe not, actually
<gmb> Lemme check.
<bac> gmb: i don't see why...
<gmb> bac, Ah, yes. It's because there's a DBItem, BugWatchActivityStatuses, which has one more item on db-devel than on devel.
<bac> oh, ok
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing:mrevell (http://is.gd/bAtGL) || queue: [sinzui, sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> mrevell: did you intentionally omit mercurial from the list?
<mrevell> bac, Yes, jelmer isn't happy for it to be mentioned publicly yet.
<bac> ok
<bac> shhh
<mrevell> :)
<mrevell> Well, let's say he doesn't want a song and dance yet.
<bac> mrevell: is the tour to be written in en_US?
<mrevell> bac, AFAIK the tour is written in en_GB.
<bac> mrevell: alrighty.  i thought we'd decided the other for most things.  i don't care, just as long as it is consistent
<mrevell> bac, The company styleguide requires en_GB these days, so over time I'll make sure everything I'm responsible for follows that. But I'm quite certain the tour was always en_GB.
<bac> mrevell: oh, i didn't know about the styleguide specifying that.  goode to knouw.
<mrevell> Yea verily
<sidnei> bac, if im allowed to jump the queue, there's a trivial one-liner here: https://code.edge.launchpad.net/~sidnei/lazr-js/use-browser-console/+merge/23778
<bac> sidnei: done
<sidnei> bac, thanks!
<bac> mrevell: your review is done.  remember if you use 'ec2 land' it's going to go directly to db-devel, which you don't want
<mrevell> Thanks bac. I'll address the issues you've raised and then use pqm-submit, following a successful ec2 test.
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing lunch|| queue: [sinzui, sinzui, bigjools] || 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 sinzui|| queue: [sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi sinzui
<sinzui> hi bac
<bac> sinzui:  i'm curious why you chose 1 year for the period to not suggest packaging again.  i would've thought six months, matching the ubuntu release period, would make more sense.
 * sinzui is on a call
<bac> o
<bac> k
<bac> no rush
<intellectronica> bac: can i add a branch to the queue?
<bac> sure intellectronica
<intellectronica> bac: great, thanks!
* intellectronica changed the topic of #launchpad-reviews to: On call: bac || reviewing sinzui|| queue: [sinzui, bigjools, intellectronica] || 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 sinzui|| queue: [bigjools, intellectronica] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> could I please have a review for https://code.edge.launchpad.net/~jml/launchpad/better-subunit-attachment/+merge/23786
<jml> bigjools, are you in the queue for https://code.edge.launchpad.net/~julian-edwards/launchpad/builder-reset-fail-bug-563353 ?
<bigjools> jml: yes but I don't mind if you insert
<jml> bigjools, actually, I was just going to review your branch if it's the one we were talking about earlier
<jml> (and if LP ever loads the damn thing)
<bigjools> jml: oh right!
<bigjools> well, thanks :)
<sinzui> bac: It takes more than 6 months for Ubuntu to diverge. Lucid +1 has the same packages as Lucid.
<sinzui> bac: a year is a better time to ask again. I also think that the next time we ask, we should explain how the project can get packages in Ubuntu
<bac> does lucid + 2 == lucid + 1 == lucid ?
<sinzui> s+2 switches out main packages. s+3 adds new packages from the universe. beta (4-6) just add dependencies
<jml> bigjools, done
<bigjools> jml: cheers
* jml changed the topic of #launchpad-reviews to: On call: bac || reviewing sinzui|| queue: [intellectronica, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> and with that, I'm off.
<bigjools> will work on it in the morning, thanks
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing intellectronica || queue: [jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> intellectronica:  r=bac
<intellectronica> bac: thanks!
<bac> intellectronica:  i do hope your new algorithm is doing what you intend.
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing jml || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> bac: yes, i hope too that this last batch of improvements will make heat more useful to more projects
<bac> intellectronica:  previously you saw very little/almost no depreciation of the heat values?
<bac> that's why it was not useful?
<intellectronica> bac: more importantly, currently heat is not sensitive enough to activity. these new changes will make it respond to activity and inactivity in a much greater granularity, so new changes to a bug, or the lack of them, will reflect quickly in the bug's heat value
<intellectronica> bac: our hope is that this will make heat more useful that now have a list of untouched but hot bugs at the top of the list
<bac> intellectronica:  it'll be a nice feature when the kinks are worked out.  this should get you much closer.
<intellectronica> bac: that's mostly a case of reality vs. ideology. intuitively, projects should work on their hot bugs and get them closed, but some don't.
<intellectronica> indeed
* 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
<EdwinGrubbs> bac: can you review this branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-553384-deactivated-project-oops/+merge/23794
<bac> EdwinGrubbs:  sure
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing edwin|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> yeah
<thumper> morning
<thumper> EdwinGrubbs: are you going to UDS
<EdwinGrubbs> thumper: bac was going for the Registry
<thumper> EdwinGrubbs: was, or is?
<EdwinGrubbs> thumper: is
<sinzui> is
<sinzui> bac: is pretty centered. He lives in the here and now. He is
<sinzui> Though his nic implies he lives in the past, back never was, he just is
<thumper> heh
<sinzui> ah evo, how I hate you
<bac> yes i is
<abentley> mwhudson, when you need a break, could you review this? https://code.edge.launchpad.net/~abentley/launchpad/no-builds-recipe/+merge/23744
<mwhudson> abentley: sure
<bac> EdwinGrubbs:  i almost finished your branch but not quite.  i'll try to finish after dinner.
<EdwinGrubbs> ok, thanks
#launchpad-reviews 2010-04-21
<jtv> bac, I take it you're off-duty now?
* 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
<adeuring> jtv: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-333521-allow-mail-to-question-subscribers/+merge/23833 ?
<jtv> adeuring: coming up
<adeuring> jtv: thanks!
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: adeuring || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adeuring: does the test need to do its own login?  Isn't it enough to pass the identity's email address to the TestCaseWithFactory constructor?
<adeuring> jtv: ah, I did not know that
<jtv> adeuring: better make sure that it's really the same, because you want this test to fail if the db permissions aren't enough.
<jtv> Next, I don't think you really need to create product_owner explicitly.  In the interest of test brevity, why not have factory.makeProduct create an owner and use self.product.owner.email in the test comparison?
<jtv> Oh, I see you're doing switchDbUser anyway
<jtv> adeuring: Does this test need Librarian at all?  If not, ZopelessDatabaseLayer may be a better layer for your test.  Faster to set up
<adeuring> right, we don't need the librarian.
<jtv> adeuring: and a definite no-no: don't compare the exact list of notification recipients.  Use self.assertContentEqual to eliminate ordering issues.
<adeuring> jtv: OK, makes sense...
<jtv> adeuring: what is a "sub" in the context of this test?  Not a sandwich?
<jtv> See the docstring.
<adeuring> that's plain nonsense. I'll remove it.
<adeuring> the title should be ""Ensure that question subscribers are notified about bug expiration"
<adeuring> jtv: can you give me an example where the DB user is passed as a constructor parameter to TestCaseWithFactory?
<adeuring> jtv: ah, now I see what you meant...
<adeuring> jtv: I need LPZopelessLayer because of the method switchDBUser() That is not avaliable in ZopelessDatabaseLayer
<adeuring> jtv: current diff: http://paste.ubuntu.com/419731/
<jtv> adeuring: oic
* allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: adeuring, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> Morning jtv :)
<jtv> allenap: morning!
<jtv> adeuring: one thing I don't like about the test is the somewhat arbitrary division between setup and test.  I think it'd help a lot of you could gut setUp, and put as much as possible in methods with meaningful names.
<adeuring> jtv: so... something like a method "createBugProductQuestionAndLinkThem()"?
<jtv> adeuring: that name reveals the problemâthat it's doing too much.
<adeuring> jtv code in the test method is needed there: make a snapshot of a bugtask, change the bugtask, call notify() for the change
<jtv> adeuring: that sounds like a more useful breakdown.
<adeuring> jtv: well... that's what setUp is for, IMHO: create the test data you need to run the tests ;)
<jtv> adeuring: opinion is divided on that subject.  It's easy to create a situation where future generations keep adding tests and depending in different ways on a convoluted setup.
<adeuring> jtv: well, ok... but this is a a unit test for very special situation. I was suprised that it occured so late...
<jtv> adeuring: ideally, the test would read as a series of logical steps very similar to the ones you just named.
<adeuring> so, what about a comment in setUp: "we need a product, a bug and a question linked to the bug in order to..."
<adeuring> ...perhaps enriched by "we want explicit person names so that we have stable test data just in case that the factory methos cahnge in a funny way"
<jtv> adeuring: I don't even see why you need explicit person names, frankly.
<jtv> You need known names, and that's no problem.
<adeuring> jtv: yes, that's cruft. I'll remove it
<adeuring> jtv: new diff
<adeuring> http://paste.ubuntu.com/419745/
<jtv> adeuring: btw I do realize this is annoying...  imagine me having a Rambo Moment here as I am reminded of experiences with convoluted tests.  :)
<jtv> adeuring: in the comment, I wouldn't say "don't clutter the test data with irrelevant notifications" (which would need a capital letter btw) but just something like "Flush pending notifications for question creation."
<adeuring> jtv: sure
<jtv> adeuring: also, I always forget... do you need to commit before switchDbUser?
<jtv> I guess not, otherwise you wouldn't get the notification, right?
<adeuring> jtv: right, otherwise the test wouldn't work...
<jtv> adeuring: I still don't see why you want the fixed email addresses.  I think it's actually clearer in the test to say "[product.owner.preferredemail, subscriber.preferredemail]" than to repeat the email address string.
<adeuring> jtv: ok, no problem...
<jtv> adeuring: then I think I've made it hard enough for you.  :)
<jtv> adeuring: approved.
<adeuring> thanks
* jtv changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, - || queue: [-] || 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: jtv, allenap || reviewing: -, jtv || queue: [-] || 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: jtv, allenap || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> allenap: was that a very fast review?
<allenap> jtv: Yes, it was perfect :)
<jtv> allenap: thanks!  (hope it wasn't perfect in the sense of "the perfect chance to dash someone's hopes before my first coffee" or "at last a review I can type out in two letters using my right hand only")
<wgrant> jtv: What, 'ok'? :P
<jtv> wgrant: ooh hadn't thought of _that_ one...
<jtv> allenap: me be relocatin
<jtv> so copping out for now, sorry to leave you with this madhouse.  :)
* jtv changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> jtv: Heh, no worries :)
<gmb> allenap, Do you have time to review a 931-line branch :D?
<gmb> allenap, It's the first step to refactoring
<allenap> gmb: Sure :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Renaming the existing BugWatchUpdater and moving checkwatches.updater to checkwatches.core.
<gmb> allenap, https://code.edge.launchpad.net/~gmb/launchpad/cw-refactor-create-rename-bwu-567793/+merge/23842
<gmb> allenap, Note, I hate the new name for BugWatchUpdater; feel free to suggest a better one. I just picked it to stop myself bikeshedding
<gmb> allenap, Your branch is a prerequisite; I don't know if the MP will DTRT now.
<allenap> gmb: CheckwatchesTopBanana
<gmb> allenap, How about CheckwatchesYesItWorksNowShutup
<allenap> gmb: Yes, that'll do it.
<allenap> gmb: TBH, I think CheckwatchesMaster is okay.
<gmb> allenap, Fair enough. We can always change it later.
* StevenK changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: My grey matter is now grey blancmange, but r=me.
<gmb> allenap, Cool. Your desserting is important to us.
<gmb> Boom
<gmb> Boom
<allenap> gmb: Oh god.
<gmb> My lame joke know no bounds.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: Edwin || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> StevenK: Hi, I don't see a merge proposal for you in https://code.edge.launchpad.net/launchpad/+activereviews. Can you point me to it?
<StevenK> allenap: Certainly https://code.edge.launchpad.net/~stevenk/launchpad/fixes-bug-451396/+merge/21706
<allenap> StevenK: Okay, I think I'll ask adeuring to follow up on that.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> allenap, StevenK: I'll look
<StevenK> adeuring: Thanks!
<adiroiban> henninge: Hi, any news about the landing of this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ? :)
<adiroiban> also is there anything I need to do to have this branch ready for code review, https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285 ?
<allenap> wgrant: Shall I land lp:~wgrant/launchpad/bug-565739-dont-retry-superseded-builds for you?
<adeuring> StevenK: it seems thet problem still remains that the tests pass when the line "if self.isPPA():" is re-introduced in lib/lp/soyuz/model/queue.py .
<StevenK> adeuring: Right, I'll revert the changes in queue.py and keep checking
<adiroiban> danilos: hi, when you have some time can we have a preimplementation chat for bug 61081? This should be about deciding a range for POTemplate priority. https://code.edge.launchpad.net/~adiroiban/launchpad/bug-61081/+merge/23811
<mup> Bug #61081: PO template +edit form needs better validation for priority field <oops> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/61081>
<wgrant> allenap: Yes please.
<danilos> adiroiban, lookin
<danilos> looking even :)
<danilos> adiroiban, fyi, on staging: http://paste.ubuntu.com/419817/
<gmb> allenap, I just found another reason not to do relative .foo imports: it makes grep-and-sed fragile :)
<adiroiban> danilos: shoud I use those limits? Maybe -100 ... 5000000 ?
<danilos> adiroiban, there is only one potemplate (pyroom) that has this priority, all the others are lower than 10k
<danilos> adiroiban, as for negative ones, that sounds wrong and we should probably just reset them
<danilos> adiroiban, 6 templates have negative priorities
<adiroiban> danilos: what values do you suggest for the priority range?
<danilos> adiroiban, I guess 0-100000 (i.e. 10x10000 to leave some room there)
<adiroiban> danilos: ok. we can reset the values... but I suppose that user will be warned when the want to update again the priority and will have to fix it
<adiroiban> danilos: is there anything on this bug that is worth mentioning or it is ready to code or I should just change the range, write tests and create MP ?
<danilos> adiroiban, yeah, we can notify them as well
<danilos> adiroiban, no, it should be ready to code; I'll add a comment to the bug explaining my "findings"
<bac> hi mrevell-lunch
<adiroiban> danilos: thanks. do you also have time to give a quick feedback based on some screenshots ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/23760 :)
<bac> mrevell-lunch:  i agree with your email wrt line wrapping.  keep it as is.
<bac> adiroiban:  thanks for your reply to my review yesterday.  everything you said makes perfect sense.  sorry for the silly suggestion about using a set.
<danilos> adiroiban, added a comment to the MP: "Looking at the screenshots: any reason to still add 'Current' to the titles on distroseries:+translations page? And don't forget to decapitalize "Base" in "Current Base" on the +language-packs page (not to mention that it'd be nice to find better terminology for these, but I am letting you take care of that with the UI reviewer)"
<danilos> adiroiban, and do NOT use tables there
<danilos> :)
<adiroiban> danilos: I will remove the "Current" and fix the capitalization and try to find better names
<danilos> adiroiban, thanks
<adiroiban> danilos: +language-packs has no table
<danilos> adiroiban, you added one to distroseries-translations
<adiroiban> but for the +translations page
<danilos> adiroiban, right, so don't do that, it's not tabular data
<adiroiban> I added one since this is what it was suggested durring an UI review for another bug
<adiroiban> ok
<danilos> adiroiban, hum, where?
<bac> adiroiban:  if you have no further changes to your branch i'll be happy to land it now.
<adiroiban> danilos: https://bugs.edge.launchpad.net/rosetta/+bug/367844 ... I added this none on the MP
<mup> Bug #367844: Language pack page needs ui improvements. <ui> <Launchpad Translations:Triaged> <https://launchpad.net/bugs/367844>
<adiroiban> note
<danilos> adiroiban, (wondering what review was that so if needed I can complain and resolve it with the UI team :)
<adiroiban> danilos: ah... my bad. looks like the bug is talking about a different data
<adiroiban> .. or not ? not sure :)
<danilos> adiroiban, that bug is obsolete, for the old version of the page :)
<adiroiban> bac: the branch should be ready for testing
<adiroiban> danilos: ok. I will revert the table
<danilos> adiroiban, thanks
<bac> adiroiban:  great.  by 'land' i meant 'ec2 land'...full test and then submit.
<mrevell> thanks bac
<bac> adiroiban: could i get you to resolve the text conflict in your branch and repush?
<adiroiban> bac: sure. I will do it now and add a commnent on the  MP when it is done
<bac> adiroiban: please ping me here too
<adiroiban> bac: done :)
<bac> adiroiban: thank
<bac> s
<bac> adiroiban: submitted to ec2.  in a short 3.5 - 4 hours it'll be landed.  :)
<jtv> allenap: want me to take that one off the queue?
<allenap> jtv: Yes please :)
<allenap> jtv: No, actually, adeuring took that one already.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> ah
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: allenap, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> bac: hi. I received an email with the subject âTest Runner FAILEDâ but the traceback does not give any hints about what test went wrong
<bac> adiroiban: it was a failure in the ec2 script.
<bac> adiroiban: the portion that sends email with the test results.  so, i'm afraid, that run was wasted
<bac> jml: https://code.launchpad.net/~bac/launchpad/bug-567985/+merge/23868
<jml> bac: reviewed.
* adiroiban changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> adiroiban: ec2 has been fixed and i'm resubmitting your branch
<adiroiban> bac: :) thanks!
* adiroiban changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [adiroiban(UI bug-146178), adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-04-22
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [adiroiban(UI bug-146178), adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/fix-ec2-harder-dammit/+merge/23897
* adiroiban changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [adiroiban(UI bug-146178), adiroiban(bug-61081), adiroiban(bug-487137)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/yet-more-api-exports/+merge/23905
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: adiroiban(UI bug-146178) || queue: [adiroiban(bug-61081), adiroiban(bug-487137)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* ctrlsoft changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: adiroiban(UI bug-146178) || queue: [adiroiban(bug-61081), adiroiban(bug-487137)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi ctrlsoft :)
<jelmer> moin noodles :-)
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: adiroiban(UI bug-146178) || queue: [adiroiban(bug-61081), adiroiban(bug-487137), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: Hi! are you after more UI reviews? If not, I'll do it now, just let me know what you prefer (for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/23760)
<noodles775> wgrant: hi! do you want me to land this one as is? https://code.edge.launchpad.net/~wgrant/launchpad/emailauthentication.txt-2.6-fix/+merge/23449
<wgrant> noodles775: Yeah, I'm a bit busy until Wednesday, so it'd probably be a good idea.
<wgrant> I'll track down the real bug then.
<noodles775> wgrant: ok, will do.
<wgrant> Thanks.
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: adiroiban(bug-61081) || queue: [adiroiban(UI bug-146178), adiroiban(bug-487137), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775, jelmer: I have a CP-candidate that could use some high-priority treatment. Can either of you take that?
<jelmer> henninge: I'll have a look
<jelmer> henninge: where's the MP?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-565294-nplurals/+merge/23861
<henninge> jelmer: thanks
<henninge> jelmer: it looks large but a lot of is was me removing lint.
<henninge> I probably should not have done that in this branch ...
<jelmer> henninge: it looks like the lint changes are mostly removing assignments?
<henninge> jelmer: yes, unused variables
<jelmer> henninge: is it correct that test_header_pluralform_western uses 'ja'?
<jelmer> (which is Japanese I guess?)
<henninge> jelmer: yes, it is about the case were the western standard formula is found in the file for a language were that should not be.
<henninge> and a language that has less plural forms than 2 - which is 1 ...
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: adiroiban(bug-487137),henninge || queue: [adiroiban(UI bug-146178), adiroiban(bug-487137), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: adiroiban(bug-487137),henninge || queue: [adiroiban(UI bug-146178), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer> noodles775: thanks
<noodles775> np, just updating what I'm reviewing currently :)
<henninge> jelmer: the actual logic behind that is based on Danilo and me looking at the current data in the database.
<henninge> jelmer: and we found some po files for asian languges that report western plural expressions.
<henninge> in many templates for POFile headers, the western style is used as the default and often translators just don't change that before importing the file to Launchpad.
<jelmer> henninge: ah, ok - thanks
<StevenK> adeuring: Hi! Do you have a moment to look at https://code.edge.launchpad.net/~stevenk/launchpad/fixes-bug-451396/+merge/21706 again?
<adeuring> StevenK: sure
<StevenK> adeuring: Ignore the 'import urllib', please, I'm just pushing the branch that gets rid of that
<adeuring> StevenK: ok ;)
<intellectronica> noodles775, jelmer: can one of you review a small branch for me?
<noodles775> Suer!
<jelmer> intellectronica: Sure, if you give me 15m :-)
<intellectronica> jelmer: great, thanks. just creating an MP.
<intellectronica> jelmer: https://code.edge.launchpad.net/~intellectronica/launchpad/bug-heat-days-active/+merge/23920
<jelmer> intellectronica: k
<intellectronica> thankyou
<noodles775> intellectronica: not sure if you saw my message before, but let me know if you are after UI reviews, otherwise I'll just do Adi's UI review now.
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: -,henninge || queue: [adiroiban(UI bug-146178), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: i must have missed your message. i'm happy to do a ui review. which mp?
<noodles775> intellectronica: I just wasn't sure if you were trying to collect UI reviews atm. (as a mentat), otherwise I'm happy to do it... it's this one: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/23760
<intellectronica> noodles775: do you know if adi will be back today for an interactive review, or shall i just reply in the mp?
<noodles775> intellectronica: I haven't heard back from him today after doing two code reviews, I'd do it via the mp for now.
<intellectronica> oright
<noodles775> jelmer: have you started intellectronica's? If not, can I swap you and you can review mine? :)
<jelmer> noodles775: I haven't started on his yet
<jelmer> noodles775: So swapping would be fine :-)
<noodles775> Great, although I guess I'll need to ask someone else to do the second review on mine :)
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: intellectronica,henninge || queue: [adiroiban(UI bug-146178), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<StevenK> adeuring: No news is good news? :-P
<adeuring> StevenK: nah, things look fine :)
<intellectronica> jelmer, noodles775: whoever is reviewing, please wait a sec, i just realised the diff is too much, because it contains some changes from a previous branch i thought had already been merged.
<noodles775> intellectronica: ok, np.
<adeuring> StevenK: I have just one remaining nitpick: could you remove the trailing space in the line "Leaving the PGP signature on a package uploaded " ?
<adeuring> we try to avoid them so that we don't get sprurious diff lines when somebody adds/removes trailing spaces.
<intellectronica> can i add a prequisite branch to the mp after the fact, or do i have to recreate it?
<StevenK> adeuring: Okay, done.
<noodles775> intellectronica: I can't see any way to do so without recreating either.
* bigjools changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: intellectronica,henninge || queue: [adiroiban(UI bug-146178), noodles, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> nice branch for you guys :)
<adeuring> StevenK: cool. r=me. I assume I should run your branch  through ec2, or can you do that?
<jelmer> noodles775: so I guess I should get somebody else to review my review of your code?
<StevenK> adeuring: I can do that. Can you fiddle the MP?
<noodles775> jelmer: yeah, shouldn't be a problem though. But maybe grab bigjools' for the moment, as at least we can do that one together?
<adeuring> StevenK: done
<intellectronica> noodles775: oright, the new mp is https://code.edge.launchpad.net/~intellectronica/launchpad/bug-heat-days-active/+merge/23921 .   sorry for the inconvenience.
<noodles775> np.
<StevenK> adeuring: Thanks! I'm throwing it through EC2 now.
<noodles775> intellectronica: I can't see that days_since_last_activity is being used for anything?
<noodles775> Did you mean to do (max_heat * 0.25 / (days_since_created - days_since_last_activity)) or something similar, looking at the comment?
<noodles775> Ah, or looking at the related bug title, I'm guessing it should be s/dasy_since_created/days_since_last_activity on line 21 on the MP diff?
<henninge> jelmer: thanks for the review, I agree with you nitpicks ;)
<jelmer> henninge: sorry it took so long, I haven't really looked at the translations code before :-)
<henninge> jelmer: that's ok. Ask about whatever is unclear.
<henninge> actually, I will go to lunch now ...
<henninge> ;)
<intellectronica> noodles775: no, i think that's a cut-n-paste error.
<henninge> jelmer: will you pass it on to noodles775 for mentoring?
<jelmer> henninge: Yeah, I've just requested a review from him as well.
<henninge> jelmer: cool, thanks
<danilos> henninge, jelmer: it's very important to get this CPed before 2200 UTC today
 * noodles775 switches to it.
<danilos> noodles775, thanks
<noodles775> intellectronica: so should it be divided by the number of days *since* the bugs last activity (as stated in the bug title), or divided by the difference between the days since the bug was created and it's last activity (as the comment seems to suggest?)
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, jelmer || reviewing: henninge,henninge || queue: [adiroiban(UI bug-146178), noodles, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: it should be divided by the days since the bug's creation
<noodles775> intellectronica: so the bug 567439 title is wrong then, ok.
<mup> Bug #567439: Add MAX_HEAT / 4 / days since last activity to bug heat <story-bug-heat> <Launchpad Bugs:In Progress by intellectronica> <https://launchpad.net/bugs/567439>
<intellectronica> noodles775: oh, right, it is. there's another bug for a calculation based on time since last activity, i must have confused them.
<noodles775> henninge: why not s/language_forms/number_plural_forms on line 57 and do away with the extra variable? (line 73 sets number_plural_forms to the same value?)
<noodles775> Ah, right, as the language.plural forms can be overwritten with None... nm.
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [adiroiban(UI bug-146178), noodles, bigjools] || 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: jelmer || reviewing: - || queue: [adiroiban(UI bug-146178), noodles, bigjools, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-04-23
* adiroiban changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [adiroiban(UI bug-146178), noodles, bigjools, sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/new-code-import-owner/+merge/23974
* thumper 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> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-index-redux/+merge/23976
<abentley> thumper, could you please review https://code.edge.launchpad.net/~abentley/launchpad/build-from-recipe-api/+merge/23981 ?
<thumper> were we too noisy?
<abentley> thumper, no, my bowels were too full.
<noodles775> adiroiban: do you want me to land your bug-487137 now?
<adiroiban> noodles775: only if you have time :)
<noodles775> adiroiban: heh, it takes all of 10 seconds ;), and the same for 61081 I assume?
<noodles775> Thanks for the updates!
<adiroiban> noodles775: yes. they need to be tested
<adiroiban> noodles775: can you please also send this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ?
<noodles775> Sure.
* 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
<noodles775> adeuring: can I get you to take a look at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-build-farm-job-model/+merge/23913
<adeuring> noodles775: sure
<noodles775> Thanks!
 * noodles775 has another one which will follow too.
* noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: hi, you seem to have removed my branch from the queue, unless you reviewed it?
<adeuring> bigjools:sorry... I think the queue was empty when I joined the channel... I even wasn't aware that you asked for a reveiw...
<adeuring> bigjools: will look at your branch when I've finished noodles'
<bigjools> adeuring: ah ok, dunno what reset the topic then, it was there earlier!
<bigjools> thanks
<noodles775> bigjools: when I logged in this morning at 7:00UTC, the queue was empty :/
<bigjools> oh :/
<bigjools> it was there last night, prob what I am thinking of
<noodles775> yeah, I was surprised too.
<adeuring> noodles775: r=me, a few nitpicks
<wgrant> 5 items were mysteriously dropped from the queue 9 hours ago.
<adeuring> bigjools: i assume this is your branch: https://code.edge.launchpad.net/~julian-edwards/launchpad/delete-ppa-part2/+merge/23967 ?
<noodles775> Thanks adeuring
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bigjools || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> wgrant: Thanks! Could you pop them back in so other people don't get annoyed? :)
* wgrant changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bigjools || queue: [noodles, adiroiban(UI bug-146178), sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: yes, thanks
<adeuring> bigjools: a naive question: can't you use "if os.path.exists(root_dir)" instead of catching two exceptions?
<noodles775> Hi adeuring, I think you forgot to include your comment when you reviewed my branch (instead using the MP description)?
<bigjools> adeuring: I prefer the exceptions, something else may go wrong, like permissions being wrong etc.
<bigjools> adeuring: it seems more robust
<bigjools> but happy to be persuaded otherwise?
<adeuring> noodles775: sorry... stupid copy&paste mistake. I've added a comment
<adeuring> bigjools: OK. But there may be other errors, like odd permissiono settings preventing the deletion. And now we don't see any specific reason why the deletion failed.
<bigjools> adeuring: it prints the reason in the log
<adeuring> bigjools: argh, yes, you are right. I did not see the little "e)" at the end of the log.warning() call. Sorry
 * adeuring really needs better glasses
<bigjools> hehe, np, easily missed
<adeuring> bigjools: so, r=me
<bigjools> adeuring: great, thanks very much
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles || queue: [adiroiban(UI bug-146178), sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: r=me
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: [lunch] || queue: [adiroiban(UI bug-146178), sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks adeuring
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: adiroiban(UI bug-146178) || queue: [, sinzui, adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * adeuring does not do ui reviews... 
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: sinzui,  || queue: [adiroiban(UI bug-146178), adiroiban(bug-402235)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: adiroiban(bug-402235)  || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> adeuring, thanks for seeing the bug I added to the code. I will reply with a fix soon
<sinzui> noodles775, I have an awesome (and I mean that in the historical context) branch that updates CSS. I hope you can find time to review it
<noodles775> sinzui: great!
<sinzui> adeuring, I replied to your review
 * adeuring is looking
<adeuring> sinzui: approved
<sinzui> adeuring, thanks
<noodles775> sinzui: In your MP text, you mention removing the duplicate css for ul/li.language, but afaics MP diff just shows those as moved?
<noodles775> Wow... the re-org. looks excellent.
<sinzui> noodles775, it was defined twice!
<noodles775> sinzui: but on your MP, I can see it added (from a move) on lines 340ff, removed on lines 1578ff, ah, right, then removed again at 3228. OK.
<sinzui> noodles775, I really hope that future additions/changes to the css will be easier with this reorganisation. I lost my evening to an obsessive need to understand all the rules we are using
<noodles775> sinzui: I think it definitely will... and yes, looking at the organisation I can see why it took an evening!
<sinzui> noodles775, have you used the W3C validator? I was looking for a tool in apt and could not find anything as good as the website
<henninge> noodles775, jelmer: Can you please re-affirm your review for my branch? I want to merge it into devel.
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-565294-devel/+merge/24020
<noodles775> sinzui: not for a few years, I'll give it a whirl next time I do some css work.
<noodles775> sinzui: another (build) tool I've used in the past for nicer stylesheet organisation is http://sass-lang.com/
<sinzui> that looks practical and is compatible with mars' ideas
<noodles775> I did mention it a while back, but I think the fact that it's ruby killed the discussion (yet more developer dependencies).
<sinzui> ruby needs another 5 years to evolve into python
<noodles775> lol
<sinzui> It wanted to be perl done right, but soon learned python done right is even better
<sinzui> I like ruby's symbol feature. I hope python steals that idea
<noodles775> henninge: done.
<henninge> cheers
<noodles775> mrevell: just a heads-up that I assume your branch will conflict with sinzui's. You may want to wait until his lands and then remerge devel.
<mrevell> Thanks noodles775, I'll leave it until after the weekend in that case.
<sinzui> mrevell, is your branch styles for help?
<mrevell> sinzui, Yes
<sinzui> mrevell, you get your own section now
<mrevell> sinzui, or, rather, it updates the help style help
<mrevell> sinzui, https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-heat-help-bug-544799/+merge/24016
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: -  || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> mrevell, you may cry. my branch is an extensive reorg. You will may want to update/add the styles by hand.
<noodles775> It should just be adding the one line for a.help.icon.
<mrevell> sinzui, Heh, no tears here :) As noodles775 says, it was just a one line change.
<sinzui> mrevell, you certainly wont have a problem finding your classes in the css.
<noodles775> :)
<mrevell> Cool :)
* adeuring changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi sinzui, can you take a look at this small chunk i separated out:  https://code.edge.launchpad.net/~bac/launchpad/bug-569101/+merge/24034
 * sinzui looks
<bac> sinzui: oops: s/The Launchpad/The Launchpad object/
<sinzui> Too late. I just approved it
<sinzui> now you have to land it as it is :)
<bac> bah
<bac> thanks, though
<bac> sinzui: have time for another gem?  https://code.edge.launchpad.net/~bac/launchpad/lplib-testing/+merge/24038
<bac> a melange, if you will
<bac> a cornucopia of code snippets
<sinzui> oh, I have reviewed the build_doctest_suite before. I glad the work will see the light of day
<sinzui> bac: I cannot review your proposal. It is read-only
<bac> well that's dumb
<sinzui> bac: you are listed as the only person who can review it
<bac> weird
<bac> sinzui: and now?
<bac> sinzui: i tried to make it depend on my previous branch but it wants to merge into it
<bac> let me blow away that MP and try again
<sinzui> oh
<sinzui> I was just reviewing it
<sinzui> bac: sorry you had fixed it
<sinzui> bac: my only remark about the code was that I think this line is missing punctuation: # Add doctests using default setup/teardown
<bac> sinzui: sorry, can you try again with https://code.edge.launchpad.net/~bac/launchpad/lplib-testing/+merge/24040
 * sinzui reviews the next proposal before a vengeful bac kills it
<bac> i think the first one was doomed
<sinzui> bac: I want to rename your kanban card  to be about refactoring lplib tests.
<bac> ok
<sinzui> bac: is your branch done with my review? Can I move the card to Review/Done?
<bac> yes
<Edwin-lunch> sinzui, bac: do either one of you want to review my branch. After I took out an assertion, the size of the branch collapsed. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-248518-setPreferredEmail/+merge/24041
<sinzui> I will
 * sinzui claims it before bac deletes it
<EdwinGrubbs> sinzui: btw, I blame pipeline for the completely unnecessary prereq branch.
<sinzui> EdwinGrubbs, I see you found an elegant fix in the factory
<sinzui> EdwinGrubbs, I am surprised there is not comparable test for account.activate() in doc/account.
<sinzui> I think your test is the only documentation we have
<EdwinGrubbs> sinzui: do you want me to add a test to doc/account?
<sinzui> no. I think there is a unit test for it. From launchpad's perspective, we care about the person.
<sinzui> EdwinGrubbs, My initial though was to move your test, but I think your decision was right. I think your tests tells other launchpad developer how to get a person's account activated
<sinzui> EdwinGrubbs, I approved your branch.
<EdwinGrubbs> sinzui: thanks
