#launchpad-reviews 2010-05-17
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [noodles775,wgrant,wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi intellectronica! The first one in the queue there for me is: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-5/+merge/25239 when you've time.
<noodles775> (just see the note about conflicts with db-devel on the MP... you can branch it rather than merge to run the tests).
<intellectronica> noodles775: yes, looking at it already
<noodles775> intellectronica: great, thanks.
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: noodles775 || queue: [wgrant,wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: r=me
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [wgrant,wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks intellectronica
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: wgrant || queue: [wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: wgrant || queue: [noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> hi intellectronica
<jelmer_> intellectronica: Can I add another branch to the queue?
<intellectronica> jelmer_: sure
* jelmer_ changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: wgrant || queue: [noodles775, sinzui, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> thanks :-)
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: sinzui || queue: [noodles775, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [noodles775, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: so the next in line is 567922-binarypackagebuild-new-table-6 ?
<noodles775> intellectronica: indeed, thanks.
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: noodles775 || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge: hi. was this branch landed https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285 ?
<henninge> adiroiban: no, don't think so. That was probably due to ec2 problems we were (are?) having.
<henninge> adiroiban: can you merge the current devel, please, before I try again?
<adiroiban> henninge: yep. I will let you know when the merge was pushed.
<henninge> thanks
<adiroiban> henninge: merge pushed. no conflicts.
<henninge> adiroiban: cool, I'll land it
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> gimme some more soyuz branches. ---- yeah!
<jelmer_> :-)
<jelmer_> it's more registry than soyuz I think
<intellectronica> yes. looks fine too.
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || 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: intellectronica || reviewing: - || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos, henninge, jtv : I have created a MP for the first step in having a POTemplate API. Maybe you would like to take a look https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525371/+merge/25423 :)
<jtv> adiroiban: I can take that if you like
<adiroiban> jtv: feel free to to it :)
<henninge> jtv: Adi and I had a pre-imp discussion about this at UDS.
<adiroiban> adeuring: hi. Is there anything I need to do for having this branch approved and landed https://code.edge.launchpad.net/~adiroiban/launchpad/bug-402235/+merge/23973 ?
<adeuring> adiroiban: nothing than reminding me. Sorry, I tried to get the branch in for the last release, but the ec3 test hung unfortunately. And I was on holidays last week, was conpletel offline for a few days. I'll stat the ec2 test now again.
<adiroiban> adeuring: don't worry :) it is not a critical bug.
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -,- || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos: are there any plans for allowing project maitainers to add custom language codes? this can help with fixing bug 487137
<mup> Bug #487137: Allow Rosetta admins to create custom language codes <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/487137>
<danilos> adiroiban, not at this time, no... I am not sure that's a good idea yet, but it's what we should aim for in the future (exposing people to more and more unfinished features would make LP only harder to use)
<adiroiban> danilos: ok
<cody-somerville> Anyone interested in doing a review? :-)
<cody-somerville> https://code.edge.launchpad.net/~cody-somerville/launchpad/create-ppa-via-api/+merge/25319
<cody-somerville> abentley, intellectronica: ^^ :-)
<intellectronica> cody-somerville: otp, but can look at it after that
<cody-somerville> kk
<danilos> bigjools, this is logging I want to add to our jobs on the build slave, any comments? https://code.edge.launchpad.net/~danilo/launchpad/bug-580345/+merge/25416
<danilos> jtv, henninge: ^^
<intellectronica> cody-somerville: oright, the ppa api branch?
<cody-somerville> create-ppa-via-api, yup
<intellectronica> cody-somerville: is it really ok to create a ppa with just a name?
<intellectronica> i mean, is it the same in the web ui?
<bigjools> no it's not
<cody-somerville> displayname looks optional
<intellectronica> cody-somerville: there you go
<intellectronica> looks?
<henninge> bigjools: huh?
<bigjools> henninge: huh?
<henninge> bigjools: what question are you replying to?
<henninge> danilos: looks good to me.
 * bigjools sees danilos' message and looks
<cody-somerville> bigjools, are you says its not okay to create a PPA with just a name?
<cody-somerville> *saying
<bigjools> cody-somerville: ideally no
<intellectronica> cody-somerville: i would rename the ubuntu variable to distribution. it's only a cosmetic change, but it means that if you ever want to start handling other distros it will be more natural. you can put the xxx comment near where you assign ubuntu to the distribution variable
<bigjools> otherwise the +ppas page looks crap
<cody-somerville> bigjools, It doesn't use the name if displayname is not set?
<intellectronica> cody-somerville: i'm a bit concerned that this will be open to misuse. how expensive is it to create new ppas? should we maybe export the method for removing them, so that if someone creates too many they can delete them easily?
<intellectronica> bigjools: consider this too ^^^
<cody-somerville> There is no cost to creating a new PPA.
<cody-somerville> and yes, I intend to export the ability to delete as well.
<bigjools> there's always a cost :)  intellectronica, I said that this was ok since we added deletion now
<intellectronica> cody-somerville: i'd feel more comfortable if you said that the cost is very low :)
<intellectronica> bigjools: you mean there's already an api for deleting them, or that you can delete them using the web interface?
<bigjools> danilos: I just added a comment to your MP
<bigjools> intellectronica: just the web interface ATM, but as Cody said he's exporting that too
<intellectronica> cool
<cody-somerville> intellectronica, Sorry, there wasn't enough significant digits in my calculation to provide that level of precision :P
<intellectronica> is there a conclusion about whether displayname and description are really optional?
<intellectronica> cody-somerville, bigjools: ^^^^^
<intellectronica> other than that the branch is fine
 * bigjools looks at branch
<danilos> bigjools, nooooo, anything but a comment!
<danilos> bigjools, it's there because I played with different approaches to logging (ideally I'd use Launchpad logger, but didn't want to think about how I get that installed onto build slaves as well)
<cody-somerville> intellectronica, displayname in not optional in web UI but description is
<bigjools> cody-somerville. intellectronica: there's some refactoring needed
<bigjools> the code is slightly duplicating the save_action() in the browser code
 * cody-somerville nods.
<cody-somerville> I was thinking the same thing
<bigjools> cody-somerville: and in fact it misses a bunch of validation
<intellectronica> cody-somerville: if you can make the web code use the same model code for doing that it would be good
<bigjools> see also validate() in the browser code
<intellectronica> cody-somerville, bigjools: i think i'll leave you to it, then ;)
<bigjools> I'll comment on the MP
<cody-somerville> bigjools, I did look at validate.
<cody-somerville> bigjools, a lot of it isn't necessary
<bigjools> cody-somerville: why?
<cody-somerville> bigjools, because that view is a hack
<cody-somerville> bigjools, its different depending if you're creating your first PPA or if you're creating a second PPA.
<bigjools> cody-somerville: the validation on the form takes that into account
<bigjools> but your new code does not
<bigjools> it also doesn't check for duplicate names
<cody-somerville> bigjools, 1. and isn't necessary since you're always supplying the name whereas the form does not
<cody-somerville> 2. It'll error out if you try that which seems fine to me since it can't provide any more meaningful feedback via the webservices API to my understanding.
<bigjools> cody-somerville: that doesn't matter, you still need to check that it's the first PPA and then check if the CoC is signed
<cody-somerville> bigjools, Thats what the discussion was about with jml on Friday night.
<bigjools> re. (2), you're not testing that so you don't know by definition ;)
<bigjools> yes, we said we'd think about ToS-ing the Coc or something
<cody-somerville> also, the browser class doesn't check if the CoC is signed either
<cody-somerville> and we can't check if the PPA ToS is signed since its not actually recorded
<bigjools> cody-somerville: it checks to see if you have accepted the ToS
<cody-somerville> bigjools, using a hack. Its not actually recorded.
<cody-somerville> bigjools, which is why I added a note in the function docstring that using the API constitutes accepting the PPA ToS.
<bigjools> hack?
<bigjools> ok that's fine I think
<cody-somerville> bigjools, Yes. It doesn't record it. If a PPA exists, it assume the ToS is accepted. If no PPAs exist, then it'll try creating the default PPA and provide that different view that has that checkbox.
<bigjools> but, I think you should add more tests where it adds the same name twice
<bigjools> I suspect it'll generate an OOPS
<bigjools> and we don't want that
<cody-somerville> bigjools, I think thats fair feedback.
<bigjools> I'll write on the MP
<cody-somerville> bigjools, ty
<cody-somerville> :-)
<bigjools> cody-somerville: np, thanks for the change
<bigjools> how was the hangover on Saturday? :)
<cody-somerville> lol. didn't drink enough for one (un)fortunately.
<cody-somerville> bigjools, how about yourself?
<bigjools> cody-somerville: you were pretty hammered on Friday :)
<bigjools> I was tired, slept like a baby on the train hime
<bigjools> home
<BjornT> cody-somerville, intellectronica, bigjools: don't forget to add tests for making sure who has permission to call the createPPA() method.
<cody-somerville> Shouldn't there already be tests for that?
<bigjools> cody-somerville: since you added the method, no :)
<bigjools> you need a unit test for it
<BjornT> cody-somerville: yes, the one who added the method should have added tests for that ;)
<cody-somerville> createPPA is just a wrapper around new on ArchiveSet
<bigjools> still needs a test dude
<bigjools> you should use launchpadlib in the story test too
<bigjools> I updated the MP
<BjornT> cody-somerville: also think about the documentation. now you're saying that anyone can call createPPA. they shouldn't have to try to call it to find it out.
<cody-somerville> very well.
<bigjools> so, Ubuntu Software Centre fails under Kubuntu :/
<EdwinGrubbs> BjornT, have you had a chance to look at the schema changes in https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part1/+merge/25359
<gmb> intellectronica, abentley: I have another branch for your queue if that's okay: https://code.edge.launchpad.net/~gmb/launchpad/show-oops-ids-bug-564686/+merge/25442
<abentley> gmb, I'll take it.
<gmb> abentley, Thank you.
<BjornT> EdwinGrubbs: no, i haven't. i'll look now
* adiroiban changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -,- || queue: [adiroiban(bug-570899), adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -,gmb || queue: [adiroiban(bug-570899), adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> gmb, is it possible to use the same code path for generating oops URLs that other code does?
<gmb> abentley, I looked into that, and tried to reuse the code in webapp/tales.py, but there didn't seem to be a simple way to do use it (I thought there would be a formatter, but apparently not). I'll have a dig around, though, see if I can find anything else.
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: adiroiban(bug-570899),gmb || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> adiroiban: you've got several jslint notices. what about them?
<intellectronica> adiroiban: also, why can't you test?
<gmb> abentley, I can't find anything reusable so far; will keep looking but I think we'd be better off having a formatter do the work for us. I might as well fix that in this branch.
<abentley> gmb, that would be great if you don't mind.
<gmb> abentley, Sure, no problem.
<BjornT> EdwinGrubbs: remind me, what is section, and can it change between different series?
<EdwinGrubbs> BjornT, section for debs are "apps", "graphics", etc.
<BjornT> EdwinGrubbs: ok. let's say a package was in "apps" in karmic, and "graphics" in lucid. first a new release is uploaded to lucid, and then one is uploaded to karmic. what value will DistributionSourcePackage.section have?
<BjornT> EdwinGrubbs: also, what happens if a PPA package is uploaded?
<EdwinGrubbs> BjornT, that's an interesting point. The trigger which caches it, gets the latest one. I'm not sure what the current behavior is for the page that gets the data directly from the SPPH table.
<BjornT> EdwinGrubbs: so, i would suggest discussing this patch with bigjools
<EdwinGrubbs> BjornT, I forgot, I need to select only PRIMARY packages to eliminate PPAs.
<bigjools> EdwinGrubbs: why are you using a trigger?
<bigjools> EdwinGrubbs: BjornT also has a good point about which series' data you're caching
<EdwinGrubbs> bigjools, since we would like to consolidate the DistributionSourcePackage class and the DistributionSourcePackageInDatabase class, the object will need to exist in the db by the time the code will use it. This could be done with a cronjob, but that actually seems like it would have more intensive queries, since it would have to query all the SPPHs for a distr+spn in order to find the one with the latest date.
<bigjools> I suspect it should only be the one that's marked 'current' for each distro
<bigjools> Since DSP is going to start getting published-specific data, you need to define which series that will be for, and how it gets updated when a new series is opened
<bigjools> I think you can do that without a trigger or cron job
<EdwinGrubbs> bigjools, where is a distroseries specified as current?
<bigjools> its status
<EdwinGrubbs> bigjools, when you say that I can do that without a trigger or cronjob, do you want the class to automatically create the DSP record whenever it is requested?
<bigjools> EdwinGrubbs: I think that's a good idea, yes.  It would only happen at package upload time.
<bigjools> (which is never a webapp transaction)
<bigjools> so when creating a publishing record, you can check to see if it's the right series and then update/create the DSP
<bigjools> this will also be good for Soyuz when we start caching stuff :)
<EdwinGrubbs> bigjools, which script creates the spph record? I assume it's triggered by dput.
<bigjools> and it's also an easy check to see if it's the PRIMARY archive
<bigjools> EdwinGrubbs: let me check where the best place to do this is
<adiroiban> intellectronica: back. I don't know how to fix those jslint.
<bigjools> EdwinGrubbs: see lib/lp/soyuz/model/publishing.py
<bigjools> EdwinGrubbs: IPublishingSet.newSourcePublication()
<adiroiban> intellectronica: as for LEFT and RIGHT keys, it looks like Windmill does not support trigering such events
<bigjools> EdwinGrubbs: you need to check that the archive.purpose = PRIMARY, then you can see if the DSP exists or not and create/update
<bigjools> that's much easier to test than a trigger :)
<adiroiban> intellectronica: here is the windmill bug http://github.com/windmill/windmill/issues#issue/33
<bigjools> EdwinGrubbs: have you figured out which series you need to cache data for?
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: adiroiban(bug-570899), - || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> bearing in mind development is newer than current
<bigjools> having said that, section is not likely to change
<intellectronica> adiroiban: ok, let me see if i can get rid of the jslint errors. we can't land with them.
<adiroiban> intellectronica: I had those errors in other branches and Danilo said that it is ok and this is the way YUI3 works
<intellectronica> adiroiban: i rather check first, and at least register the problem somehow. it's not a good idea to keep jslint errors in the codebase.
<danilos> adiroiban, I don't remember saying so unless if it was a branch where you didn't touch the faulty JS which I don't want to hold you responsible for :)
<EdwinGrubbs> bigjools, I don't know for which series the section should be cached. Maybe sinzui knows.
<intellectronica> adiroiban: also, i think that at this point it would make sense to assign the magic numbers to CONSTANTS, but I won't ask you to do it because i see that you're just extending legacy code.
<sinzui> series cache>
<bigjools> EdwinGrubbs: it's an important question since this table will be useful beyond your use case I think
<sinzui> Is someone watching my keystrokes as I addtal:content="cache:public, 1 hour"
<sinzui> to series templates?
<bigjools> \m/
<adiroiban> intellectronica: I will create those contants as it should improve the code readability
<intellectronica> adiroiban: excellent, thanks!
<sinzui> bigjools, I think I am working on the issue you are talking about. I am working on the timeouts for distroseries and milestones that dominate oops
<bigjools> sinzui: I am checking over Edwin's branch, I guess you're both working on this?
<gmb> abentley-lunch, I've added a formatter for oops IDs and some tests to go with it. I've pasted a diff in the MP for your perusal.
<sinzui> bigjools, I made the initial proposal. and I liked EdwinGrubbs suggested changes. I have not looked at the branch though
<sinzui> bigjools, I suppose you are remarking that once a series is released, the packages should not change.
<sinzui> is backports main?
<intellectronica> adiroiban: that code is a bit weird, no wonder jslint complains
<bigjools> sinzui: well a couple of things, 1. we should do it in code not in a trigger (soyuz will benefit from cache publishing data in places so I want to prepare for that), 2. yes a released series won't change
<intellectronica> danilos: ^^^^^
<bigjools> sinzui: backports is a pocket, not sure what you mean?
<bigjools> you mean the primary archive?
<sinzui> bigjools, thanks for your incite
<sinzui> yes I mean primary archive
<bigjools> so yes -backports is the primary archive
<sinzui> yuck
<bigjools> problems?
<sinzui> um, no actually. I had a momentary lapse of reason
<bigjools> I get that all the time :)
 * sinzui is still thinking of series and milestones
<EdwinGrubbs> sinzui, on which page will you be querying the cached section from the DSP table? It doesn't look like that is used on the +needs-packaging page.
<adiroiban> intellectronica: let me rewrite it. what do you consider to be weird ? :)
<intellectronica> adiroiban: i don't understand what is them `stem` second argument that the even handlers receive
<intellectronica> adiroiban: oh, is it the last argument you pass to on()?
<sinzui> EdwinGrubbs, I think the root page is the only user of memcached. I am wrapping portlets.
<sinzui> EdwinGrubbs, I have not tested memcached with a batched listing. I do not think it will work
<adiroiban> intellectronica: let me rewrite that code as I realise it is just stupid :)
<intellectronica> adiroiban: i have a solution for the first batch of lint errors, but it ain't pretty :)
<intellectronica> adiroiban: what were you thinking of doing?
<adiroiban> intellectronica: those functions are adapters for toggleWidget()
<adiroiban> as the YUI3 will pass the event as the first argument
<adiroiban> I can not use them directly with Y.on
<intellectronica> adiroiban: yeah, got it. the solution i have is to enclose the handlers in a closure. it isn't necessary in this case, since you're not capturing anything but using yui's event machinery to pass an argument, but jslint is correct to make that suggestion, and a nicer solution would require refactoring the code.
<EdwinGrubbs> sinzui, What does memcached have to do with the DistributionSourcePackage.section column?
<intellectronica> adiroiban: a nicer solution might be to move the body of the loop into a separate function
<sinzui> EdwinGrubbs, memcached+tales caches page fragments. Which page/part do you want me to test?
<adiroiban> intellectronica: i was thinking to create those adapter functions outside the loop
<intellectronica> adiroiban: yes, that's also a possibility. whichever you prefer.
<sinzui> EdwinGrubbs, memcached+tales does include the query string in the key. I think we can cache batches
<EdwinGrubbs> sinzui, I'm adding the section column to the DistributionSourcePackage table to cache the data from the SPPH table. I think you told me to do that on Friday. I don't know where the query is that we are trying to speed up. This should be unrelated to memcached.
<sinzui> EdwinGrubbs, yes, it is unrelated. To exclude language-packs, we use the section which is know in spph or in spr. I used spph in the branch I am landing today
<sinzui> EdwinGrubbs, yes, it is unrelated. To exclude language-packs, we use the section which is know in spph or in spr. I used spph in the branch I am landing today
* intellectronica changed the topic of #launchpad-reviews to: On call: abentley || reviewing:  - || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> abentley, i'd like to add https://code.edge.launchpad.net/~leonardr/lazr.restful/understand-mod-compress/+merge/25454 to the review queue
<mars> gary_poster, ping, have a moment to review https://code.edge.launchpad.net/~mars/launchpad/disable-windmill-tests/+merge/25457 ?
<gary_poster> mars, ok
<gary_poster> mars, you added in TESTOPTS for theoretical correctness, yes?
<gary_poster> that is, no one is actually using them, it seems?
<mars> gary_poster, it was already in there, just not used properly.
<mars> they are used by three targets lower down in the file
<gary_poster> ah, gotcha mars
<mars> I was going to rename VERBOSITY to TESTOPTS myself, and was surprised to find TESTOPTS was already in there
<gary_poster> yeah, another approach would be to rip it out, but I suppose this is safer
<mars> using the value of the VERBOSITY variable to pass "-vvv -t --layer" to the test process really rubbed me the wrong way.  Refactoring gene I guess :/
<mars> gary_poster, safer, yes.  I don't know who else used VERBOSITY that way.
<gary_poster> mars, have you checked to see what happens to the test runner if you say --layer='!WindmillLayer' --layer='WindmillLayer'?
<mars> Needed a fast fix.  This is one step forward.
<mars> My guess is that --layer=!Layer --layer=Layer *will* execute the desired test suite.
<gary_poster> well, for a fast fix, maybe leaving TESTOPTS out of the picture would be wiser.  Otherwise I have questions about it
<mars> But no, I have not explicitly tested that :)
<mars> gary_poster, oh?  What questions?
<gary_poster> for instance--if you are right (which would be my guess) then I would suggest that ``bin/test $(VERBOSITY) $(TESTOPTS) --layer=WindmillLayer`` should instead be ``bin/test $(VERBOSITY) --layer=WindmillLayer $(TESTOPTS)`` so that TESTOPTS can actually override things
<mars> gary_poster, I treat TESTOPTS like CFLAGS.  VERBOSITY is independent of the two.
<mars> gary_poster, ok, interesting.  That woudl have come up regardless if I used VERBOSITY or TESTOPTS.
<gary_poster> yeah, calling it TESTOPTS is more correct, and makes the possible problem clearer.
<gary_poster> mars, I'll approve, simply asking that you consider my question...possibly later. :-)
<mars> +1
<mars> gary_poster, I'll send a post to the list when this lands.
<gary_poster> mars, ok.  approved.
<mars> a post about the suite disablement
<gary_poster> right
<mars> thanks gary_poster
<gary_poster> mars, also note that when this branch lands the linked bug will be marked fix committed I think.  If so, I'm guessing (?) you'll want to manually change it back, since this is a bandaid not a fix.
<mars> gary_poster, yep!  Had to do that a few times now.  The QA scripts also do that.
<mars> I'm working against the grain!
<gary_poster> :-)
<leonardr> abentley, i don't know if you're still reviewing but i've also got a launchpad follow-up to my earlier lazr.restful branch
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/understand-mod-compress
* abentley changed the topic of #launchpad-reviews to:  On call: abentley || reviewing: leonardr || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> leonardr, did you mean to target your branch at db-devel, rather than devel?
<leonardr> abentley: i don't think so, i generally target devel--is devel closed atm or something?
<abentley> leonardr, devel is not closed.  You have proposed for landing against db-devel.  This means that some other changes from devel are showing in your diff.
<leonardr> abentley: i misunderstood your question
<leonardr> i thought you were saying "you targeted your branch at devel, but you probably meant db-devel" rather than vice versa
<abentley> leonardr, ah, thought so.
<leonardr> abentley, i'm redoing the proposal
<abentley> leonardr, thanks.
<leonardr> done
<abentley> leonardr, there should be two blank lines before a new header in a docstring.
<abentley> leonardr, also, the differences between '"foo-gzip"' and '"foo"' info '"foo"-gzip' are pretty subtle.  It would be nice to mention them both at the same time.
<abentley> e.g.:  mod_compress may turn '"foo"' into '"foo"-gzip' or '"foo-gzip"'.  We treat both forms as the same.
<abentley> also, you say "info" where you mean "into".
* abentley 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
<leonardr> gary: abentley reviewed my launchpad branch but not the lazr.restful branch it uses. would you take a look?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/understand-mod-compress/+merge/25454
<gary_poster> leonardr: ack, looking
<adiroiban> intellectronica: i have updated the JS code adding constanst for key codes and fixing jslint warnings.
<gary_poster> leonardr: approved, no comments.  thank you
<leonardr> gary, great
#launchpad-reviews 2010-05-18
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/git-update/+merge/25485 ?
<thumper> mwhudson: super trivial
<mwhudson> thumper: i guess you're fixing the conflicts?
<thumper> :(
<thumper> yeah
<thumper> I should have branched from db-devel not devel with the production ancestor
<thumper> will fix
<thumper> mwhudson: updated
<mwhudson> thumper: approved
<thumper> ta
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/package-branch-edit-owner/+merge/25291 if you are looking for something to do
* noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> jtv1: hi. Should I add you as a reviewer for the POTemplates API MP, or it is on your todo list and you will review it when you have time ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525371/+merge/25423
<jtv1> adiroiban: I'm terribly sorry, I'm just too distracted by what's going on here.  I'll continue trying to give you feedback, but reviews are pretty difficult under the circumstances.
<adiroiban> jtv1: ok. just let me know if the branch is to hard to review and I will need to rewrite or break it in multiple MPs
<jtv1> adiroiban: don't worry too much about the lint warnings...  sometimes it's just being annoying.
<jtv1> adiroiban: thanks for doing this btw... it'll be a great thing to have.
<jtv> adiroiban: it's looking good to me so far.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775, Is it your 567922-binarypackagebuild-new-table-7 that you need reviewed?
<noodles775> gmb: yes, that's the one. Thanks!
<gmb> noodles775, ... Cos' it's got a bunch of conflicts :)
<gmb> Aaah
<gmb> That's because it's a pipeline branch, isn't it.
<gmb> Right, okay. Good.
<noodles775> gmb: yeah, I've mentioned it in the MP. Great.
<gmb> noodles775, Yeah, I only had the topmost portion of the MP on my screen at the time. That'll teach me to not scroll down :)
<gmb> noodles775, r=me
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> BjornT: just sent a db review your way
* noodles775 changed the topic of #launchpad-reviews to: On call: gmb || reviewing: lunch || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb, just another branch ensuring tests pass after previous refactoring work: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-8/+merge/25515
<adiroiban> intellectronica: do you have time to review this branch that we have discussed yesterday, or should I find another reviewer ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-570899/+merge/25443
<gmb> noodles775, Righto, I'll take a look.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> adiroiban: no, i'll be happy to finish the review. thanks for making the changes!
<noodles775> gmb: thanks!
<intellectronica> adiroiban: that looks great. i really like the solution you've come up with.
<BjornT> intellectronica: ok. you could start with committing and pushing your changes to comments.sql, which you seem to have forget to do ;)
<intellectronica> BjornT: heh, of course :)
<gmb> noodles775, At the start of the diff you have a commented out `ALTER TABLE Build SET SCHEMA todrop;` and then an uncommented version immediately after it. Is the commented version unnecessary or is it an artifact of the pipeline process?
<noodles775> gmb: it's actually removing lines 8-11 (the diff makes this hard to see). ie. removing the comment and replacing it with the real deal. But I actually didn't intend that to be in the diff (the db-schema change will obviously need a separate review).
<gmb> noodles775, Ah right. Makes sense.
<gmb> noodles775, r=me
<noodles775> Thanks gmb.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: did you have trouble updating the sampledata? When I tried with a db-patch I'm preparing, I ended up with over 6k of sample data diff :/
<noodles775> (actually, that was on a pristine db-devel, not my branch).
<intellectronica> noodles775: ehrm .... haven't tried that yes, i'm afraid. is it even necessary? we're definitely not adding any new sample data, after all, and i don't remember creating new versions of the sample data for ages (and evidently other devs haven't either)
<intellectronica> of course for some changes that's unavoidable
<BjornT> gmb: want to review a really small branch? https://code.launchpad.net/~bjornt/launchpad/dont-hardcode-auto-generated-names/+merge/25518
<noodles775> intellectronica: ok.
<gmb> BjornT, Of course.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: BjornT || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> BjornT, r=me.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> BjornT: updated by db branch with comments
<BjornT> gmb: thanks. here's a related one, slightly longer: https://code.edge.launchpad.net/~bjornt/launchpad/compare-email-headers/+merge/25520
<gmb> BjornT, I'll take a look
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: BjornT || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> thanks
<gmb> BjornT, r=me
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> gmb: thanks. i guess you could take a look with this one as well :) https://code.edge.launchpad.net/~bjornt/launchpad/more-unique-factory-strings/+merge/25438
<BjornT> gmb: the previous two branches fixed all the test failures in the third one
<gmb> BjornT, Sure :)
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: BjornT || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> BjornT, And that's r=me too :)
<gmb> Would've been quicker but for my connection going away for a couple of minutes...
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> gmb: thanks!
<BjornT> EdwinGrubbs: i've added some questions/comments to your db patch review
* sinzui changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> gmb, do you have time to read a short branch about memcached. I do not consider it to be mechanical, though it looks like it is
<gmb> sinzui, Sure.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bigjools, BjornT made an interesting point about renaming the columns in DistributionSourcePackage to make it clear that they are cached values. That could be annoying if DistributionSourcePackageCache and DistributionSourcePackage are ever merged. The only non-cached value in either table that I'm aware of is the bug_reporting_guidelines. I'm wondering if it makes more sense to add the new columns to the DistributionSourc
<EdwinGrubbs> ePackageCache table instead.
<bigjools> EdwinGrubbs: OTP, gimme 20m
<BjornT_> EdwinGrubbs: btw, i was only talking about the ones that might be out-of-date. section is fine, since it should always be up-to-date.
<EdwinGrubbs> BjornT_, well, there is some hope that DistributionSourcePackageCache and DistributionSourcePackage would eventually be merged. That would mean there could be a bunch more columns with "cached" in the name. However, if we don't merge them, just moving cached values to DistributionSourcePackageCache makes it clear that they are cached without renaming the columns.
<gmb> sinzui, Sorry for taking so long - OTP and other things - is this your milestone-performance-0 branch that needs reviewing?
<sinzui> gmb: yes
<sinzui> I should have made that clear when I asked
<gmb> sinzui, r=me
* gmb changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> gmb, thanks
<abentley> rockstar, would you be able to review https://code.edge.launchpad.net/~abentley/launchpad/fix-recipebuilder/+merge/25528 ?
<rockstar> abentley, sure
<abentley> rockstar, thanks.
<mars> gary_poster, ping, do you have a moment to review this on-line change?  https://code.edge.launchpad.net/~mars/launchpad/fix-ec2-mailmanlayer/+merge/25527
<gary_poster> what, you did it on=line?  :-)
<gary_poster> yes
<mars> on=line ?
<mars> oh, typo of the typo
<gary_poster> yes :-)
<gary_poster> approved mars
<mars> gary_poster, thanks.  Passes "ec2 test", will be landing now with "bzr lp-land"
<gary_poster> mars, ack, cool
<abentley> rockstar, could you also review https://code.edge.launchpad.net/~abentley/launchpad/estimate-duration/+merge/25553 ?
<rockstar> abentley, sure.
<abentley> rockstar, thanks again.
<abentley> rockstar, could you please review https://code.launchpad.net/~abentley/launchpad/remember-archive/+merge/25564 ?
<rockstar> abentley, on it.
<abentley> rockstar, ta
<rockstar> abentley, are you EODing soon?  Do you need this before you EOD?
<abentley> rockstar, I have EOD'd.  No rush.
<rockstar> abentley, okay.  I'll make sure it's done before I EOD.
<abentley> rockstar, cool.
* adiroiban changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [adiroiban(bug-561586)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-05-19
<noodles775> Hi BjornT, stub: when you've time, I've got a db review request at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-changes-build-generalisation/+merge/25587
<adiroiban> jtv1: Hi. Is there anything I need to do for POTempalte api branch to have it approved? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525371/+merge/25423
<jtv1> adiroiban: getting the UN to intervene in my local situation would help! :)
<jtv1> adiroiban: seriously, it's hard under the circumstances to get anything done at the moment when you live in Bangkok.  Best to get someone else to do the bulk of the review.
<adiroiban> jtv1: sorry to hear that. I was not aware of the unrest from Bangkok.
<jtv1> adiroiban: getting pretty bad now...  where I live is mentioned as the hottest place.  So I'm hiding out elsewhere now.
<jtv1> Trying to find out if we're under the curfew... the government can be very vague when they're trying not to lie.
<jtv1> We're between 3 army bases here, meaning there's no actual fighting but not a good place to violate the curfew either.  Enough innocent passers-by have been cut down by the army.
* danilos changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [adiroiban(bug-561586), danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adiroiban, I'll take on reviewing your branch
<danilos> adiroiban, the cleanups you did (moving IHasTranslationTemplates around) should definitely have been a separate branch: both branches would have been much simpler to review
<gary_poster> stub, ping?  Actually trying to review your branch
<stub> yo
<gary_poster> Was wondering if postgres itself keeps pg_stat_user_tables up-to-date
<gary_poster> hey
<stub> Yes. pg keeps them up to date.
<stub> Magic stats tables
<gary_poster> Gotcha
<gary_poster> so, that stats_reset thing...
<gary_poster> that logic says...
<gary_poster> if pg has updated the stats since the last time we recorded ir, purge DatabaseTableStats?
<gary_poster> s/ir/it/
<gary_poster> I must be misreading it
<gary_poster> wait, yeah
<gary_poster> NowStat.seq_scan < LastStat.seq_scan
<gary_poster> but how could that happen?
<gary_poster> stub ^^^
<stub> No. Sometimes PG will reset the stats to zero.
<gary_poster> ...and then we just trash the old stats?
<stub> If that happens, we purge.
<gary_poster> why?
<stub> Yup. We have no way of knowing *when* it happened, so we can't compute valid deltas
<stub> It shouldn't happen unless someone explicitly tells pg to reset the stats, so I haven't put in any more convoluted logic than that.
<gary_poster> ah ok
<gary_poster> stub: so, first thought is that I'd like a comment explaining all that.  It was far from clear to me on reading.
<stub> ok
<henninge> danilos: wasn't there some problem with 'debug_exec', too?
<danilos> henninge, it's fixed (original branch was against production-devel, but that doesn't make any difference for build slaves)
<danilos> henninge, I had a bashism ("function debug_func {" instead of "debug_func()" which works everywhere)
<henninge> danilos: ah yes, I see.
<henninge> danilos: the python --version is pretty useless, though.
<henninge> danilos: you should use the python in the chroot. This script is still running outside of it.
<danilos> henninge, I know, but can't do that in the shell script :)
<henninge> not?
<danilos> henninge, well, chroot would first have to be set-up and that happens only at the end
<henninge> danilos: but python works from almost anywhere.
<henninge> let me try that ... ;)
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || reviewing: - || queue: [adiroiban(bug-561586), danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> danilos: yup, works. Just do "$BUILD_CHROOT/usr/bin/python --version"
<danilos> henninge, sure
<adiroiban> danilos: sorry about that. I can split them now if you think it will help
<henninge> danilos: nothing else to complain ;) r=me
<danilos> henninge, thanks
<danilos> adiroiban, nah, too late now, I've worked my way through it, review coming soon :)
<bac> gmb: ping
<gmb> bac, Hi
<bac> hi gmb, are you reviewing again this cycle?
<gmb> bac, Yes. I've updated the REviewerSchedule and put myself back in the Tuesday EU slot, though I can go to any EU slot you'd like.
<bac> gmb cool.  let's talk in the meeting.
<gmb> bac, Sure.
<gary_poster> stub (sorry had stand up call, which had technical difficulties, and now in reviewer mtg), understanding what schemaname and relname are seems to be pretty crucial to understanding what the heck is going on.  Could you explain?  I'll probably ask for a quick comment in the code so it is easier to follow along.
<stub> schema name is the schema name. This is 'public' for all lp tables, and '_sl' for the slony ones, 'pg_catalog' for postgres internal. The relname is the table name (relation name)
<stub> I'm just using the same names the PostgreSQL internals uses.
<gary_poster> sure, just didn't know, and that's kinda critical to understanding the logic
<gary_poster> stub, would ask that to be clarified (just as briefly as you just did) in the report code.
<gary_poster> Maybe less necessary once someone is able to actually still see the results, but still.
<gary_poster> stub, so right now we just compare INTERVAL ago to now.  Is that as much detail as you expect we'll need, or an incremental step towards producing a graph over time?
<gary_poster> AFAICT it doesn't answer a class of questions I thought we were trying to answer: what user is responsible for certain spikes.  We'll make another tuolomne graph for that?
<gary_poster> in any case, I'll approve with the two requests for explanatory comments.  Thank you!
<henninge> adiroiban: tests failed again. It's the windmill tests, so not your fault most likely. Can you merge devel, again, please? I heard they have been disabled now.
<adiroiban> henninge: but then they will fail on landing .. or not?
<henninge> adiroiban: no, shouldn't.
<adiroiban> I am not sure what is causing them... but they look spurious
<adiroiban> as I also get them on devel, when running the tests on a busy old P4 computer
<adiroiban> but it is not always the same test that fail
<adiroiban> I will merge the devel and see how it goes
<henninge> danilos: will you update and land your branch or did you want me to do that. I forget so easily ... ;)
<danilos> henninge, I'll update it and land it
* sinzui changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || reviewing: - || queue: [adiroiban(bug-561586), danilo, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge: i have merged devel and pushed the changes
<henninge> adiroiban: yup, that should help. See: http://paste.ubuntu.com/436183/
<adiroiban> henninge: but bug 570380 is only about hanging ec2 test, not failing test
<mup> Bug #570380: ec2test sometimes hangs when running the windmill test suite <build-infrastructure> <qa-needstesting> <Launchpad Foundations:In Progress by mars> <https://launchpad.net/bugs/570380>
<henninge> hm, true ...
<mars> adiroiban, did you see the mail to the list yesterday about spurious mailman layer failures on ec2?
<henninge> also, that was already merged earlier
<henninge> mars: but we already ran that revision, I just realized.
<mars> ok, just making sure
<adiroiban> mars: â ec2 test will be broken for a few hoursâ ?
<adiroiban> henninge: were you able to reproduce those errors on your computer?
<mars> adiroiban, yes, and the follow-up, 'ec2 test is working again'
<henninge> err, didn't try. You tried, didn't you?
<henninge> mars: when was that?
<adiroiban> henninge: yes. If I run only the failing tests, they pass
<mars> henninge, yesterday 3pm local, so 21:00UTC?
<henninge> mars: yes, just found it.
<henninge> This branch failed today, though.
<adiroiban> but if I'm running all windmill tests, I will see spurious failures
<adiroiban> and it the same for devel
<adiroiban> not only for my branch
<gary_poster> danilos: the branch I was talking about is https://code.launchpad.net/~gary/launchpad/bug531071/+merge/25618 .  Could you take a look when you have a moment?
<danilos> gary_poster, sure
<gary_poster> thank you
<danilos> gary_poster, r=danilo, and a suggestion on how to trigger these views on staging if you want to QA it
<gary_poster> danilos: thank you, QA info especially appreciated
<stub> gary_poster: hmm.... yes... I forgot to add command line arguments for historical views. I guess I should land that separately.
<gary_poster> stub, ok cool.  however you want to work it.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || reviewing: - || queue: [danilo, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui, I'm starting on your mp now.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || reviewing: sinzui || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs, bac did it thanks
<EdwinGrubbs> danilos, ping
* sinzui changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || reviewing:  || queue: [danilo] || 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: EdwinGrubbs || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> EdwinGrubbs: thanks for the review. If everthing is OK, can you also send this branch on ec2-test?
<adiroiban> I have done a local test and everthing looks fine
<EdwinGrubbs> adiroiban, sure, I'll land it for you.
#launchpad-reviews 2010-05-20
<bryceh> would someone mind looking at this MP?  https://code.edge.launchpad.net/~bryceharrington/launchpad/lp-253509/+merge/25655
<mwhudson> bryceh: the format of the docstring you've changed is wrong and there are no tests
<bryceh> hrm, nevermind I guess
* jelmer changed the topic of #launchpad-reviews to: On Call: jelmer  || reviewing:  || 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: jelmer, noodles  || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<thumper> jml: up for a relatively simple review
<thumper> ?
<thumper> jml: package branch related
<jml> thumper: sure.
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/package-branch-edit-owner/+merge/25291
<jml> thumper: we should delete 'make_erics_fooix_project' some time :)
<thumper> jml: I think it is used in a test somewhere
<thumper> jml: it was also useful in make harness
<thumper> to get some more sample data
<thumper> although...
<thumper> we chould just update the sample data that is used interactively
<jml> thumper: that second case feels like it should be something separate
<jml> thumper: reviewed
<jml> thumper: sorry it took so long â I'm getting rusty
<thumper> jml: thanks
<henninge> danilos, danilo_: https://code.edge.launchpad.net/~henninge/launchpad/bug-581746-serialization-error/+merge/25623
<danilo_> henninge, henninge_: thanks
<danilo_> henninge, r=danilo
<adiroiban> henninge: hi. have you sent my branch to landing, or is there something I need to do?
<adiroiban> I had the same problems with the branch for bug 561586, which was only about moving some files.
<mup> Bug #561586: Move javascript code to lp.app directories <javascript> <qa-ok> <tech-debt> <Launchpad Foundations:Triaged> <Launchpad Registry:Fix Committed by bac> <Launchpad Bugs:Triaged> <Launchpad Translations:Fix Committed by adiroiban> <Soyuz:Triaged> <https://launchpad.net/bugs/561586>
<adiroiban> Windmill test were randomly failing on my test machine but it look like it passed the official landing test
<EdwinGrubbs> BjornT, do you have any further comments for https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part1/+merge/25359
<BjornT> EdwinGrubbs: well, i guess the db patch is fine. you should have a talk with deryck, as to whether include private bugs in the count, since that might not be desired. also talk to him about how to keep the bug heat values up-to-date
<BjornT> EdwinGrubbs: doing it in addTask() only won't be enough
<EdwinGrubbs> ok, will do
<EdwinGrubbs> stub, can you review this db patch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part1/+merge/25359
<EdwinGrubbs> deryck, ping
<deryck> Hi EdwinGrubbs
<deryck> EdwinGrubbs, I can look at the MP in about 5 minutes or so (if your ping is related to BjornT's comments) :-)
<EdwinGrubbs> deryck, yes, that's it. I'll wait.
<stub> EdwinGrubbs: done. Its annoying having all these max_bug_heat columns everywhere, but I can't come up with a saner model at the moment.
* noodles775 changed the topic of #launchpad-reviews to: On Call: jelmer  || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> thanks
* adiroiban changed the topic of #launchpad-reviews to: On Call: jelmer  || reviewing:  || queue: [adiroiban(bug-561586-take-2)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On Call: jelmer  || reviewing: adiroiban(bug-561586-take-2) || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On Call: jelmer  || reviewing:  || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> EdwinGrubbs, looking at your mp.... max_bug_heat didn't exist on distributionsourcepackage initially?
<deryck> EdwinGrubbs, need to break for eating before last half of day starts.  Will catch up after that.
<EdwinGrubbs> deryck[lunch], max_bug_heat did exist before on DSP. It's already used in HasBugHeatMixin.setMaxBugHeat().
<deryck[lunch]> EdwinGrubbs, ah, didn't see that schema was commented out.  I see now.  Will follow up more shortly.  Was just curious about that.
<adiroiban> jelmer: thanks for the review. Can you please send the branch to ec2-test ?
<jelmer> adiroiban: noodles has sent it off to ec2
<adiroiban> thanks!
* 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
<salgado> bac, around?
<bac> hi salgado
<salgado> hi there
<salgado> I've talked to my new manager and we've agreed that it's not a good idea for me to keep my OCR duties on this new position.  is that ok with you?
<salgado> I'll try to do reviews when I can, but right now I don't think I can commit one day a week to it
<bac> salgado: i've already removed you from the schedule...just assumed you would not be reviewing
<bac> salgado: thanks for bringing it up
<salgado> oh, cool.  didn't notice you'd removed me.  thanks. :)
<salgado> bac, if you notice people overloaded in a specific day, please let me know and I'll arrange to give them some help
<bac> wow, thanks a lot salgado.  i'll keep that in mind but sure try not to abuse it.
<salgado> bac, please do (keep it in mind, I mean).  we expect to still be doing some Launchpad work, and everyone seems to agree it's a good idea to keep doing some reviews as well. :)
<bac> nice
<bac> hi rockstar, time for a quick review?
<bac> sinzui, rockstar must be out to lunch, would you mind doing the review for https://code.edge.launchpad.net/~bac/launchpad/bug-520476/+merge/25713
<sinzui> I'll take it
<bac> thx
<bac> i don't think you'll break a sweat
<rockstar> bac, sorry, was eating food out the back of a truck...
<bac> rockstar: food from a truck should always take precedence
<bac> rockstar: in durham there is now a korean BBQ truck.  i may have to take an extra long lunch some day soon
<bac> rockstar: weren't you anti-food-truck not long ago?
<rockstar> bac, no, not at all.
<bac> i seem to recall you making fun of my love of taco trucks
<rockstar> bac, there are two things I LOVE out the back of a truck: mexican food and bbq.
<bac> i want to hire one for a party
<bac> can't decide between a taco truck and the crepe truck
<rockstar> I especially like the ones that run out of food.  I feel like I'm getting something rare.
<adiroiban> rockstar: Hi. Do you have time for an UI pre-implementation review for bug 512133 ? It is not urgent.
<mup> Bug #512133: Make template description visible to translators <Launchpad Translations:Triaged> <https://launchpad.net/bugs/512133>
<rockstar> adiroiban, I'm on the phone now, but I can look at it afterwards.
<adiroiban> ok
<thumper> flacoste: https://code.edge.launchpad.net/~thumper/launchpad/bug-linkification/+merge/25488
<thumper> flacoste: can I get a general approval?
<thumper> flacoste: and I'll fix the conflicts
#launchpad-reviews 2010-05-21
* 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 changed the topic of #launchpad-reviews to: On Call: adeuring  || reviewing:  || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> hi adeuring, when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-fix-ec2-failures-after-db-devel-merge/+merge/25759
 * adeuring is looking
* adeuring changed the topic of #launchpad-reviews to:  On Call: adeuring  || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: before I start to read through the diff: is it correct with > 1000 lines?
<noodles775> adeuring: yeah, 1055 lines (sorry). It's mostly brainless substitutions (buildstate->status etc), I got carried away while fixing them and forgot to check the diff. If it's a pain for you, I can split it up.
<adeuring> noodles775: ok, no problem -- I just wanted to avoid to read a possibly wrong diff ;)
<noodles775> OK, I just pushed 9306 (one line change that I missed in archive-views.txt). Thanks.
<adeuring> noodles775: r=me
* 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> Thanks adeuring
<deryck> adeuring, I have a branch for review.
* deryck changed the topic of #launchpad-reviews to: On Call: adeuring  || reviewing: - || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> deryck: sure, I'll look
<deryck> adeuring, thanks!  https://code.launchpad.net/~deryck/launchpad/move-pagetests-bug-583472/+merge/25776
<adiroiban> henninge: hi. What do you suggest we should do for landing the branch for bug 525992 ?
<henninge> adiroiban: merge devel and try again ...
<adeuring> deryck: r=me. Thanks for this cleanup!
<deryck> adeuring, thanks!
* 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
<adiroiban> danilos, henninge: are you in the mood for a quick UI review for bug 512133 ?
<mup> Bug #512133: Make template description visible to translators <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/512133>
<henninge> adiroiban: that sounds simple. Let's see the mp ;)
<henninge> adiroiban: actually, maybe adeuring can do that, too.
<adiroiban> henninge: I have attached a mockup http://launchpadlibrarian.net/48833845/translate-file-details.pdf
<adiroiban> no code yet, as placing that information on an already busy page is not trivial
<adeuring> henninge, adiroiban Im not even an apprentice for UI reviews ;)
 * henninge wonders why Chromium cannot handle PDS
<henninge> adeuring, adiroiban: I overead the "UI" bit .... ;-)
<adiroiban> :)
<henninge> adiroiban: you will have to get a proper UI review.
<henninge> adiroiban: the second box is not dismissible?
<adiroiban> henninge: no
<adiroiban> is the template details
<henninge> and it expands when I click on more ... ?
<adiroiban> you will be able to âfoldâ it
<adiroiban> yes
<henninge> I know, we talked about that at UDS
<adiroiban> on âmoreâ and on the right icon
<henninge> adiroiban: I know the icon will fail UI review.
<henninge> these triangles belong on the left side.,
<henninge> BUT
<henninge> thare also the icons that you find next to the input boxes. May one of those works.
 * henninge just found the second page ... ;)
<adiroiban> in the PDF, you should be able to click on âmoreâ and it will take you to the second page
<henninge> Wow!
<henninge> :D
<henninge> other things I see, just checking if they are deliberate:
<henninge> - urls get linkified (like elsewhere in LP)
<henninge> - when minimized, newlines in the text are ignored
<adiroiban> henninge: ignoring newlines in minimeze is deliberate
<adiroiban> to compact the text and since the minimezed details will only be one line
<henninge> - I assume we will have the same thing on  the template's page
<henninge> adiroiban: good idea
<henninge> adiroiban: I like it, apart from the green triangle. See what danilo thinks.
<henninge> oh, to continue my list:
<noodles775> Looks great Adi :). Just a thought, why not have the normal (X) instead of the triangle, so that it can be dismissed in the same way? (given that clicking on 'more' will display the rest?)
<gmb> adeuring, Good afternoon. Could I get a review for https://code.edge.launchpad.net/~gmb/launchpad/inline-help-for-bugwatches-2-bug-530162/+merge/25784 please?
<adeuring> gmb: sure
<gmb> adeuring, Thanks.
<adiroiban> noodles775: the idea is that those details shoud be permanent
<henninge> - The text "Developers notes" is static text, not editable.
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring  || reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> not just a transient notice like âdid you knwo about the guidelinesâ
<danilos> adiroiban, it looks good, I agree with henninge for most of it :)
<gmb> mrevell, I've added some popup help to explain bug watch errors and give pointers for fixing them (where possible) do you have the time to review it?
<adiroiban> henninge: yes. âDevelopers notesâ is static
<mrevell> gmb, I'd love to. Should I check the queue?
<adiroiban> to inform translators what are all those words about
<danilos> adiroiban, well, link to translation guidelines is as permanent
<gmb> mrevell, The MP is here: https://code.edge.launchpad.net/~gmb/launchpad/inline-help-for-bugwatches-2-bug-530162/+merge/25784
<henninge> - the text is indented (I wonder if that is necessary)
<mrevell> thanks gmb
<danilos> adiroiban, it's just that once you read it there's no need for it to take up that much space on the most prominent part of the page
<gmb> mrevell, The file you want to look at is lib/lp/bugs/templates/bugwatch-error-help.pt
<noodles775> oh really? I'm not sure that info/message boxes were meant to be permanent like that (although I think I have seen them used that way elsewhere... it'd be good to know where).
<henninge> danilos: wasn't it that the balloon re-appears every day?
<gmb> mrevell, I've created a dynamic help page rather than using the static /+help/ system.
<gmb> So that I can give them links to specific things.
<henninge> balloon = message box
<adiroiban> danilos: adding a cookie for each POTemplate can generate lots of cookies
<danilos> henninge, I don't remember the details of how the cookie is set up, but it is limited to a certain context
<adiroiban> danilos: and I was thinking that the description is updated more frequent than the guidelines
<danilos> adiroiban, then you should add a cookie only if it's dismissed :)
<danilos> adiroiban, a session cookie then? :)
<noodles775> adiroiban: not that it's a great example, but I think the precedent for text like that is to use a drop-down like on the PPA page: https://edge.launchpad.net/~michael.nelson/+archive/pocketsphinx
<noodles775> (see "Technical details about this PPA")
<adiroiban> danilos: on Chromium, session cookies last a long time
<danilos> adiroiban, uhm, ok, so?
<adiroiban> so they are not deleted when closing the browser
<noodles775> (another example on source page "Other versions of 'compiz' in unt..." https://edge.launchpad.net/ubuntu/+source/compiz )
<mrevell> gmb, Cool! So, the links go to the bug watch page from which the pop-up was called, is that right?
<danilos> noodles775, just for clarification, this is something that might not be defined, and if it is, we want people to read it
<adiroiban> and it is hard to know what a âsessionâ means for each browser
<gmb> mrevell, Yes. Also to the remote bug, where appropriate (i.e. where I'm saying things like "check that the remote bug (link) exists."
<adiroiban> noodles775: thank. I will go for those icons for fold/unfold
<mrevell> gmb, That is excellent. Nice one.
<gmb> Ta
<noodles775> danilos: yep, I understood that, just not sure that using a msg box/balloon is something we want to do for permanent notices.
<mrevell> gmb, Is it reusable or would it require more dev work for each different situation?
<danilos> noodles775, I don't think this one should be permanent either :) but I am just arguing over that with adiroiban, so let's see what we come up with :)
<noodles775> danilos: great.
<danilos> adiroiban, that clearly sounds like a bug in chromium then
<gmb> mrevell, It's pretty context-specific. There's no back-end view to this, I'm just referencing the current context (i.e. the bug watch) in the template.
<gmb> I don't think it can be easily genericised.
<adiroiban> danilos: my only problem with this solution, is that if you accidentaly dismissed that notification
<mrevell> Ah right.
<adiroiban> it will be hard to undo your action
<danilos> adiroiban, well, the same is true for anything dismissable, and I agree that's a problem, but that's a problem as much for the guidelines box, isn't it?
<adiroiban> danilos: but maybe we can add a small button / text for âShow details againâ
<danilos> adiroiban, uhm, no :)
<danilos> adiroiban, we should just show that text permanently on the pofile +details or +overview page
<adiroiban> danilos: yes. same problem for guidelines box
<adiroiban> danilos: ok. then the details box, will behave like the guidelines box
<gmb> mrevell, Thanks for spotting the typo :). I had a hell of a time writing that sentence in a coherent manner.
<danilos> adiroiban, the fact that pofile:+details page is almost useless now doesn't mean it needs to stay that way; though, getting it fixed so it resembles anything useful would be hard
<gmb> Launchpad can't be described in natural language, I think...
<mrevell> gmb, heh, I can imagine :)
<danilos> adiroiban, the "expand" trick is nice, though :)
<mrevell> gmb, I've spent the past three and a half years trying
<gmb> :)
<danilos> adiroiban, I'd even consider putting it in the same box for now, but basically, what we need on +translate page is a nice place to put things like "interesting things to look at"
<danilos> adiroiban, btw, if you are filing a chromium bug, reference http://www.ietf.org/rfc/rfc2109.txt and section 4.3.1 where it says that cookie should be discarded when user agent exits ;)
<adiroiban> danilos: yep... but now with the feature of saving tabs and state, i guess this is the desire behavior
<danilos> adiroiban, not sure who is it desired by (not by me, even as a user), but it's directly against the spec
<danilos> adiroiban, I don't want temporary cookies to accumulate in my browser cache
<adiroiban> danilos: back to our box. I don't like the idea of putting guidelines and details in the same dismissable box, since maybe some users will want to dismiss the guidelines links, but always keep the POTemplate details
<adeuring> gmb: I think the "elif" in line 58 of the diff is never executed because UNKNOWN apperas in error_message_mapping
<gmb> adeuring, I know, but all if...elses must have an elif.
<danilos> adiroiban, when jumping from page to page of the same PO file? I am not convinced, and if so, keeping the single line of guideline links shouldn't be too much of a distraction
<gmb> adeuring, It protects us from OOPSes down the line if we add an error status but forget to update that property.
<gmb> adeuring, Hang on, I'm talking nonsense, aren't I...
<gmb> Wait a minute.
<adeuring> gmb: ;)
<adeuring> gmb: argh, no, _I_ am talking nonsense.
<adiroiban> danilos: no. when going to different templates. the guidelines will keep poping together with the template details. Or are you saying that we should put them in the same box, with different dismiss buttons ?
<adeuring> mis-read the != operator as ==
<adeuring> ...in this "elif"
<gmb> adeuring, Ah, yes.,
<adeuring> so, everything is fine
<gmb> adeuring, I was about to agree with you and remove it. That could have been amusing...
<danilos> adiroiban, no, it should be a single dismiss button
<danilos> adiroiban, is that how guidelines dismissal works today?
<adiroiban> danilos: guideline dismissal state is kept by a sessio cookie. is that the answer for your question ? :)
<adeuring> gmb: but nevertheless.. I think you can s/elif/else/ and remove the current else part
<danilos> adiroiban, afaics, they already dismiss guidelines only for the pofile you are on
<gmb> adeuring, Yes, I agree.
<danilos> adiroiban, i.e. when you switch templates, you have to dismiss guidelines again
<danilos> (I keep typing "dismiss" as "dimiss": too many esses in there :)
<adiroiban> danilos: true. the path is the canonical URL for the pofile
<adiroiban> danilos: true. the cookie path is the canonical URL for the pofile
<danilos> adiroiban, I find that just fine, so there should be no issues to use the same box afaict
<adiroiban> danilos: ok. then we will use the same box
<danilos> adiroiban, it would be nice to add the collapsing and expanding to it because potemplate details can get big, but that's up to you :)
<danilos> henninge, what do you think of that?
<danilos> henninge, i.e. using the existing box for the potemplate description as well?
<adiroiban> danilos: yes. fold/unfold or (collape/expand) is a must
<adiroiban> example of long description: https://translations.edge.launchpad.net/ubuntu/lucid/+source/ubiquity-slideshow-ubuntu/+pots/ubiquity-slideshow-ubuntu
<danilos> adiroiban, I'd probably favour fold/expand or collapse/unfold, but that's just me :P
 * henninge reads
<danilos> adiroiban, yeah, definitely then :) anyway, it'd be nice to add write support for potemplate description so we can have maintainers set it directly in the page :)
<danilos> API write support, that is
<adiroiban> danilos: hm... the POTemplates API branch should implment that
<danilos> adiroiban, then I've done a bad job reviewing it ;)
<henninge> adiroiban: danilos: They should be seperate boxes.
<henninge> Remember taht the guildelines are _per language_, therefore per pofile.
<henninge> whereas the potemplate description is, well, per template ;)
<henninge> but then again, one translator will usually just work in one language, anyway ...
<adiroiban> danilos: why ? becouse there are no tests for that?
<danilos> henninge, yeah, I don't really buy the per-language vs per-template argument :)
<henninge> danilos: no, I just realized that, too.
<danilos> adiroiban, could be, and this is something very interesting to make sure we got right, because of the permissions
<henninge> adiroiban, danilos: let's put them in one popup, expandable and dismissable like it is now. No need for a new cookie then.
<danilos> henninge, cool, let's do that then :)
<henninge> but leave it like it is now on the template index page, right?
<adiroiban> danilos: while having a review for a previous API MP, i was told there is no need to test for security, since they are already tested for the API
<adiroiban> danilos: since the permissions are checked for the browser, this should also cover the API. Security should be checked only if the security.py mechanism is not used. But maybe I am wrong.
<adeuring> gmb: r=me. nice work!
<adiroiban> danilos, henninge: one more insignificat detail: The POTemplate should be indented like in the current PDF, or âDevelopers notes:â should only be appended to the text in bold and then use the normal paragraph text flow?
<danilos> adiroiban, right, it should work fine for all the stuff except methods
<danilos> adiroiban, (on the API permissions)
<danilos> adiroiban, now, considering it's easy to break permissions without it affecting the UI code, I strongly believe we should have unit tests for these
<danilos> adiroiban, now, back on the topic: I am not sure, it's best to try it out and see how it works with both approaches
<danilos> adiroiban, that text should probably not be "Developer's notes" though: we should come up with something more appropriate like "Tips for translating this"
<danilos> adiroiban, oh, btw, have you considered using something like a help popup for the potemplate details?
<adiroiban> danilos: yes, but I could not find any strong argument for using a popup
<adiroiban> it should take the same number of clicks
<gmb> adeuring, Thanks!
<adeuring> gmb: welcome. sorry for forgetting an irc notice
* 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
<adiroiban> danilos: when adding the âDeveloper's notesâ text, I was keeping in mind the bug 211, and to have both âNotes from other translatorsâ and âNotes from developerâ
<mup> Bug #211: Add comments to POFile for communication between translators <feature> <Launchpad Translations:Triaged> <https://launchpad.net/bugs/211>
<gmb> adeuring, Can you update the mp so I can ec2 land it?
<adeuring> gmb: done
<gmb> adeuring, Thanks
<danilos> adiroiban, those ancient bugs are a a grab-bag of things and +translate page would need serious redesign to cope with all this :)
<adiroiban> danilos: to bad. I was thinking that I will be able to solve that bug by using this notification box :(
<danilos> adiroiban, oohhh, no, pleaaase not
<danilos> adiroiban, we've got to have a measure of dirty hacks we let in, imo
* adeuring 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
<adiroiban> yes. but then there will be always something more important than the redesign of +translate page
<adiroiban> and until the page is redesigned, we can have those dirty hacks/workarounds
<gmb> adeuring, Can you add a review to that MP? The merge proposal is approved but the code review is still pending.
<adeuring> gmb: arghh, sorry
<gmb> adeuring, No worries :)
<adeuring> gmb: no nmy best day; i had too a few nitpick about the help text but somehow manage to forget click the "save comment" button...
<adiroiban> danilos: so what will happen with the POTemplate API branch ? are you going to have a new review for it ?
<danilos> adiroiban, no, I am going to take a quick look to confirm it's all fine and land it as-is; if I don't get it landed I'll let you know
<adiroiban> danilos: ok. thanks!
<adiroiban> danilos, henninge: the UX and design for the new +translate page will be done by Rosetta devs or there will be a dedicated UX/Design engineer to handle it?
<danilos> adiroiban, it'll be done by us mostly, we'll be asking for user input :) as a matter of fact, it's something we can do together before the Epic and then implement it during the Epic :)
<gmb> adeuring, I see them; I'll fix them.
<gmb> THanks.
<adiroiban> danilos: and until now, Henning mockups (I saw them and give feedback for them during the UDS) are the only ideas/design that we have?
<danilos> adiroiban, yes, they are
<danilos> adiroiban, btw, where is that feedback?
<adiroiban> danilos: mrevell should process it. everyhing was audio recorded :)
<danilos> adiroiban, oh, you took part in the user study, cool :)
<mrevell> adiroiban, danilos, I'm writing the report right now :)
<danilos> mrevell, yeah, yeah, as if I believe you :P
<mrevell> haha, wanna screen shot? :)
<adiroiban> intellectronica: hi. any news regarding the landing of this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-570899/+merge/25443 ? :)
<adiroiban> henninge, danilos: here is what I understood from the previous chat: http://launchpadlibrarian.net/48876861/translate-file-details.pdf :)
<danilos> adiroiban, looks great :)
<henninge> adiroiban: really good!
<bdmurray> Is there a reviewer now?
<bdmurray> If so I'd like https://code.edge.launchpad.net/~brian-murray/launchpad/bug-546078/+merge/25740 reviewed
<rockstar> sinzui, can I get a UI review from you?
<sinzui> yes
<rockstar> sinzui, https://code.edge.launchpad.net/~rockstar/launchpad/change-recipe-link/+merge/25790
<sinzui> rockstar r=me
<rockstar> sinzui, nice, thanks.
 * rockstar goes to make the tests pass...
<deryck> bdmurray, I can't do it right now, but if you haven't found someone before I return later, I can do it then.
<bdmurray> deryck: okay, thanks.  is there a reviewer schedule posted somewhere?
<deryck> bdmurray, https://dev.launchpad.net/ReviewerSchedule
<deryck> bdmurray, you can also just poke around the approved reviewers list and ask if anyone has a spare moment.
#launchpad-reviews 2010-05-23
<rockstar> I don't suppose anyone is around for a review?
<thumper> rockstar: what's up?
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/git-update-attempt-2/+merge/25861
<thumper> mwhudson: ^^
<mwhudson> thumper: approved
