#launchpad-reviews 2009-11-16
 * thumper is back
<thumper> rockstar: I've got the popup-diff working perfectly
<thumper> rockstar: I'd like you to take another look
<rockstar> thumper, okay.  I looked at your markup yesterday, and saw some issues.  Have you checked it across browsers?
<rockstar> I'm not sure if it's going to work across browsers.
<thumper> rockstar: it works in konq
<thumper> rockstar: what were the issues you saw?
<rockstar> thumper, the ones I'm concerned about are IE and Opera, for the opposite reasons. Opera is standards compliant, so the nesting of tables is inconsistent enough to freak it out, and IE ignores standards and sticks its head in its butt.
<thumper> rockstar: I don't have the environment set up to test ie
<thumper> rockstar: we can use ec2?
<rockstar> thumper, but IE issues with the overlay itself are known.
<rockstar> thumper, we can, but I don't know how to do it.  I have a Windows vm I used last week.
<thumper> ...
 * thumper thinks
<thumper> rockstar: where do I get opera from?
<rockstar> thumper, there are debs for opera available.
<thumper> rockstar: I think I could tweak my local settings to have a local windowd box to look
<rockstar> thumper, I'd be happy to check what it looks like, but just looking at your code, it's not going to work on IE.
<thumper> rockstar: I'm booting windows on another laptop
<thumper> rockstar: and downloading opera
<rockstar> ...for the same reason the current incarnation of the wizard widget will never work on IE.
<rockstar> thumper, so I'd first suggest a if (Y.UA.IE) { return; } in the connect stuff.
<thumper> rockstar: it doesn't seem to be connecting in opera either
 * rockstar wonders if we updated lazr-js in the versions.cfg
<thumper> rockstar: are there examples elsewhere where we do this?
<rockstar> thumper, the subscriber stuff does it.  lib/canonical/launchpad/javascript/code/branchsubscription.js
<thumper> ok
<thumper> so... why doesn't it work?
<rockstar> thumper, by the way, I'm working on a subscription widget that makes that code better, because it sucks terribly right now.
<thumper> rockstar: excellent
<thumper> I'd love to get this and the other branches landed asap
<thumper> nothing like shaking things out like real use
<thumper> :)
<rockstar> thumper, so, in IE, it won't work because the overlay uses a table layout, and IE hateses innerHTML on tables.
<thumper> hmm..
<thumper> the overlay is also broken in that it give everything id=shadow
<thumper> v.bad
<rockstar> thumper, yes, I need to fix that as well.
<thumper> arse 
<thumper> I can't get my windows laptop to see the local launchpad on my laptop
<rockstar> thumper, so once you draw the overlay, in IE you can't update it.
<rockstar> thumper, you'll have to edit your apache configs to listen on the public interface.
<wgrant> thumper: https://dev.launchpad.net/Running/RemoteAccess
<thumper> rockstar: did that
<thumper> wgrant: ta
<rockstar> thumper, can you change the lazr-js popupdiff namespace to code.branchmergeproposal.popupdiff ?  I'm going to send an email to our team (and eventually to the list) on the organization of namespaces.
<thumper> rockstar: sure
<thumper> rockstar: although
<thumper> rockstar: why add the branchmergeproposal into the namespace?
<thumper> rockstar: it is used on branch and bug pages
<rockstar> thumper, also, Y.popupdiff = Y.namespace('code.popupupdiff') is probably unnecessary.  Do you have a specific reason for it?
<thumper> no
<thumper> not really
<rockstar> thumper, yes, but diffs hang off merge proposals.
<thumper> I just copied that from other samples
<rockstar> thumper, so you could do "var namespace = Y.namespace('code...')" and then use namespace as a shortcut var to the namespace.  Attaching it to Y is kinda pointless.
<thumper> rockstar: I don't think we use in internally
<thumper> rockstar: just to get access to the methods from outside I think
<rockstar> thumper, yea, you'll notice that all of us have been doing things oddly.  We're learning.
<thumper> :)
<rockstar> thumper, yeah, but you use it to define "connect_diff_links"
<thumper> oh, ok
<thumper> can we just then go "Y.code.branchmergeproposal.popupdiff.connect_diff_links()" ?
<rockstar> thumper, yes.
<thumper> ok
<rockstar> thumper, the javascript needs documentation.
<thumper> :)
<wgrant> bzr: ERROR: Server sent an unexpected error: ('error', "ImportError instance has no attribute 'message'")
<wgrant> Local codehosting is unhappy with me.
<thumper> damn
<thumper> wgrant: I just used local codehosting to get some branches in
<wgrant> thumper: Hm, odd. I must have broken something at some point.
<rockstar> wgrant, I wonder if codehosting can't find one of the needed libraries.  I suggest looking at the oopses.
<leonardr> in the absence of an on-call reviewer let me restate my humble request for someone to review https://code.edge.launchpad.net/~leonardr/lazr.restful/latest-version/+merge/14791
<bac> leonardr: i can review it for you.
<leonardr> bac, thanks
<leonardr> i've got a sequel coming soon
* abentley changed the topic of #launchpad-reviews to:  on call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: looks good.
<leonardr> great
* henninge changed the topic of #launchpad-reviews to:  on call: abentley, henninge || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Hi abentley!
<abentley> henninge: Hi!
* sinzui changed the topic of #launchpad-reviews to:  on call: abentley, henninge || reviewing: -,- || queue [sinzui, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> sinzui: If you mention my nick when you request a review, I'll respond promptly.
<sinzui> abentley: sorry, I was promptly pinged after I updated the topic, went into a meeting, then was pinged again
<abentley> sinzui: np.
<leonardr> abentley or henninge, can you add https://code.edge.launchpad.net/~leonardr/lazr.restful/active-versions/+merge/14912 to the queue?
* abentley changed the topic of #launchpad-reviews to:  on call: abentley, henninge || reviewing: -,- || queue [sinzui, sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> leonardr: queued.
<abentley> sinzui: r=me on musette-and-drums
<sinzui> thanks. By tomorrow there may be no open trivial bug in the registry
<abentley> sinzui: On involvement-code-link-bug-482256, what would you think about mentioning the product by name in the Link?
<sinzui> That would be inconsistent with the involvement menu We do not do that for the dozen other exceptions
<abentley> sinzui: Dozen other exceptions to what?
<sinzui> The involvement menu is about the pillar. The links are activated for and by the pillar. We have exceptions for many subordinate objects to enable or disable the links for series and package, for each facet
<sinzui> We also have exceptions for project-groups because the are broken by design
<abentley> sinzui: I see.  r=me.
<sinzui> fab
<abentley> sinzui: I just thought the phrasing was a bit awkward and terminology-laden, and since we already knew what the series' product was...
* abentley changed the topic of #launchpad-reviews to:  on call: abentley, henninge || reviewing: -,- || queue [sinzui, leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> abentley: I understand. The menu and the 3.0 headers may need more reconciliation to make it clear what is going on. Since few project enable official_codehosting, this oops from the menu was rarely seen. Yet the goal of the involvement menu to to encourage projects to choose official_codehosting
* abentley changed the topic of #launchpad-reviews to:  on call: abentley, henninge || reviewing: sinzui,- || queue [leonardr] || 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: abentley, henninge || reviewing: leonardr,- || queue [] || 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: abentley, henninge || reviewing: -,- || queue [] || 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: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> thumper, did you see my review?
<thumper> rockstar: yeah, I think so
<rockstar> thumper, I also forgot that we had talked about disabling Opera and IE last night - other than that and the things I recommended, we should land it.
* 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
#launchpad-reviews 2009-11-17
* 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
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> adeuring: Is https://code.edge.launchpad.net/~jtv/launchpad/custom-language-codes/+merge/14555 good to land? If so, can you flip the MP status please? I always feel kinda dirty doing it for someone elses reviews.
<gmb> adeuring: Same for https://code.edge.launchpad.net/~sinzui/launchpad/packaging-page-bug-481351/+merge/14811, too, please.
<adeuring> gmb: right, I'll update the status
<gmb> Thanks.
<bac> thanks for the review gmb.  and good morning.
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Morning bac.
<bac> gmb:  much going on here today?
<gmb> bac: Not atm. There were some small branches in the +activereviews queue; didn't take long to clear. No requests so far.
<bac> cool. i'm about to amble up the street for some pastries but will be back in a jiffy.
 * gmb -> lunch
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi sinzui
<sinzui> hi bac
<bac> bigjools: are you going to review cody's branch?
<bigjools> bac: I worked with him on it so I am happy with what it's doing, so if you want to finish it off that would be good!  I am pretty busy at UDS
<bac> bigjools: ah, ok.  you were listed on the MP so i wanted to ask first.  i'll do it.
<bigjools> oh I was wasn't I
<flacoste> leonardr: have you seen https://code.launchpad.net/~jkakar/launchpadlib/testing-support/+merge/14444 ?
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,cody || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> flacoste, looking
<leonardr> hmm
<leonardr> flacoste, i've commented on https://code.edge.launchpad.net/~jkakar/launchpadlib/testing-support/+merge/14444
<leonardr> jml, i'd like to read your thoughts on that as well
<gmb> bac: Once you're done with Cody's branch could you take a look at https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-471974/+merge/14955 for me?
<allenap> sinzui: Are you still okay to land https://code.edge.launchpad.net/~wgrant/launchpad/distroseries-source-format-selection-part1/+merge/14729?
<allenap> rockstar: Can you land https://code.edge.launchpad.net/~zematynnad/lazr-js/fx_fix/+merge/14793 on behalf of Danny Tamez?
<sinzui> The change is fine to land in db-devel. That was not acceptable to wgrant. Given the security change, I do not know what he can do about it. He may b able to split the work into separate branches, each getting a new review
<allenap> sinzui: Does he want it to get to edge sooner?
<sinzui> yes
<allenap> sinzui: Okay, I'll leave it in his court for now. Thanks.
<allenap> beuno: Hi, do you think you could land https://code.edge.launchpad.net/~jblount/lazr-js/gimme-some-docs/+merge/12272 for Josh?
<beuno> allenap, please do
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: -,- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> beuno: Ah, I was asking if you could do it ;)
<allenap> beuno: If you're really busy, then I will, just say.
<beuno> allenap, if you can, I'd be super happy
<bac> gmb: you still here?
<salgado> hi bac
<bac> hi salgado
<salgado> bac, can you do a review for me? https://code.edge.launchpad.net/~salgado/launchpad/bug-481375/+merge/14961
<bac> salgado: indeed!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: gmb || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: sorry i got distracted.  r=bac
<salgado> bac, no worries, thanks! :)
<wgrant> sinzui, allenap: It's no use on edge unless we cherrypick it, so it is fine to land to db-devel. Please do.
<sinzui> wgrant: I will land it as soon as we have a working py2.5 image for ec2
<wgrant> sinzui: Thanks.
<wgrant> sinzui: Don't worry about it. al-maisan is landing for me.
<sinzui> wgrant: al-maisan: okay. thanks
<al-maisan> sinzui: you are welcome 
<wgrant> Thanks al-maisan, sinzui.
<leonardr> bac, if you have time, add this to your queue: https://code.edge.launchpad.net/~leonardr/lazr.restful/double-your-enjoyment/+merge/14967
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: sorryi didn't update the topic but i'm done for the day
<leonardr> bac, np
<thumper> rockstar: can you approve my popup-diff branch now?
<rockstar> thumper, yessir.
<rockstar> thumper, whoa whoa whoa.  Why is there a diff inline in your comment?
<thumper> :)
<thumper> rockstar: because I attached it to the comment
<rockstar> thumper, that looks scary...
<thumper> rockstar: why?
<rockstar> thumper, it just looks weird.  I'm not sure it's wrong though, but I _feel_ like it is.
 * rockstar is not a UI expert, no matter how hard he tries to get beuno to teach him
<rockstar> thumper, the comments aren't yuidoc format (basically javadoc format)
<rockstar> thumper, um, land it the way it is.  After this branch I'm working on now, I need to audit our javascript and make some changes anyway.
<rockstar> thumper, basically, we've been doing it wrong, and that's my fault.
<thumper> rockstar: ok
<rockstar> thumper, r=me
<rockstar> Also, inline bmp status changing is the sexiness
<EdwinGrubbs> mars: when you get a chance, can you review my lazr-js branch? https://code.edge.launchpad.net/~edwin-grubbs/lazr-js/activator-ie-fixes/+merge/14969
<mwhudson> someone review this pls: https://code.edge.launchpad.net/~mwhudson/launchpad/fix-public-image-building/+merge/14970
<jml> mwhudson, done
<mwhudson> jml: thank you
#launchpad-reviews 2009-11-18
<stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14980 for staging and dogfood rebuild goodness. Pretty trivial.
<thumper> stub: shouldn't you be closing cursors?
<stub> thumper: Nah- they close themselves when they drop out of scope and the script is in autocommit mode anyway
<thumper> ok
<thumper> done
<thumper> trivial review: https://code.edge.launchpad.net/~thumper/launchpad/branch-visibility-fubar/+merge/14982
<mwhudson> thumper: is not specified in the private case really appropriate?
<mwhudson> i guess it's consistent with our approach in general
<thumper> mwhudson: yes, it is consistent with what we have
<mwhudson> thumper: fair enough
 * mwhudson approves
<thumper> mwhudson: thanks
 * mwhudson wonders if submitting the review is going to time out
<mwhudson> ah, not quite
<gmb> allenap: Could you do a JS review of https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-471974/+merge/14955 for me? bac suggested I get someone to review the JS specifically.
<allenap> gmb: Okay.
<gmb> allenap: I'll request a review.
<allenap> gmb: Cool.
<gmb> AAAAGH.
 * gmb hates at chromium's behaviour
<gmb> allenap: requested. It doesn't say JS, but I guess you can figure that out for yourself.
<allenap> gmb: Why does it try to load the portlet after the ids?
<allenap> gmb: I'm going to go to lunch now, so I'll catch up with you later :)
<gmb> allenap: Because it... needs to load the portlet?
<gmb> I suppose we could load the portlet first, then grab the IDs before setting up the subscribe / unsubscribe links. That might make more sense.
<sinzui> henninge: r=me
<henninge> sinzui: cheers
<allenap> gmb: Ah, I meant, why after? Why not fire off the request at the same time?
<abentley> flacoste: Can I get an RC on this for cherry-picking? :
<abentley> flacoste: https://code.launchpad.net/~abentley/launchpad/parse-binary-2/+merge/14915
<flacoste> abentley: approved
<abentley> flacoste: Thanks.
<gmb> allenap: Well, we need the IDs before we set up the portlet, or at least before we set up the subscribe / unsubscribe links.
<gmb> allenap: So it might make more sense to load the portlet and then, when that's loaded, get the IDs before setting up the links.
<allenap> gmb: If the IDs fail to load, and the portlet is loaded anyway, I assume the links will not be set up?
<gmb> allenap: Correct.
<gmb> allenap: In fact, we can use that to our advantage by only setting up the links if the IDs are present.
<gmb> Instead of trying to set them up anyway with unpredictable results.
<allenap> gmb: Sounds good.
<gmb> allenap: Cool. I'll go do that now.
<allenap> gmb: Instead of using a place-holder template into which you put some JSON, and which then you have to parse at client end to get the JSON back out, you could override LaunchpadView.render() to simply return some JSON.
<gmb> allenap: Ooh, sweet. I'll do that, thanks.
<allenap> gmb: The request.response mime type might need setting too, but it'll probably work without (and I never said that).
<gmb> allenap: Hah. What to, assuming that that's what I need to do?
<allenap> gmb: application/json I think.
<gmb> allenap: Right, thought as much; wasn't sure about the first bit though.
<flacoste> abentley: talk to salgado, he also has a r-c
<flacoste> abentley: you should land it together
 * flacoste hopes it's not too late
<abentley> flacoste: Sorry, it's too late.
<bac> hi allenap
<allenap> bac: Hi there.
<bac> allenap: what do you think of http://pastebin.ubuntu.com/321559/
<allenap> bac: Looks good. Instead of cd'ing in and cd'ing out again, I'd run it in a sub-shell, like (cd $$subdir && bzr clean-tree ...)
<bac> allenap: hey i'll give that a try
 * bac lacks makefile-fu and even bash-fu
<bac> allenap: hey that worked:  http://pastebin.ubuntu.com/321567/
<allenap> bac: The exit $$? will only exit the sub-shell I think, but if you move it outside of the braces it should work.
<allenap> bac: So (cd $$subdir && bzr clean-tree --ignored --force) || exit $$?;
<allenap> bac: r=me though. And, does this get called as part of a make clean on the top-level Makefile?
<bac> allenap: it does not
<bac> allenap: i think cleaning sourcecode is something we should do only rarely and on-demand
<bac> what do you think?
<bac> i just wanted a target to make it easier without a lot of fuss
<allenap> bac: That's fair.
<bac> allenap: if we change our minds it's an easy fix later
<allenap> bac: Yes. If we wire it in now, it probably annoys a lot of people who keep having to rebuild stuff.
<bac> allenap: and normal tree updates shouldn't need a clean and rebuild.  we only experienced it this week as the 2.4->2.5 change could not trigger a rebuild
<bac> thanks for the review and improvements
<allenap> bac: Yep.
<allenap> bac: While you're there, do you normally reply to the "Project License Submitted" messages?
<allenap> bac: As in, should I leave them alone?
<bac> please continue to point them out to me
<bac> i do not regularly review the feedback queue
<bac> sinzui will often reply to them whether he is CHR or not
<allenap> bac: Okay. And I've just noticed https://wiki.canonical.com/Launchpad/PolicyandProcess/CommunityHelpRotation/LicensingIssues which I can work through.
<allenap> bac: Thanks.
<bac> np
<sinzui> I do, but I only looked once last week. I had to do some project review catchup over the weekend too
<gmb> allenap: I've updated https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-471974/+merge/14955 with an incremental diff of the suggested changes.
<allenap> gmb: Top banana.
<gmb> bac: See above about the branch you reviewed yesterday.
<bac> gmb: will do
<gmb> Thanks
<bac> gmb:  i'll update the MP.  thanks for adding the test.
<gmb> bac: No worries. Working on trying to get windmill working now.
<bac> gmb: my use of "fragility" was from frustration at not being able to figure out why the icon alignment issue was there.  your explanation that it was pre-existing is good.
<gmb> Ah, right.
<gmb> bac: Yeah, I can't figure that one out, either. That reminds me - I'll file a bug now.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> edwin, can you please review https://code.edge.launchpad.net/~leonardr/lazr.restful/double-your-enjoyment/+merge/14967 ?
<EdwinGrubbs> leonardr: sure
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: leonardr || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: That's a beaut. One final question: why do you need to do the .replace('bugs.', '') bit?
<gmb> allenap: I think it's to do with the correct path to the bug. I admit, I've cribbed that from what's already there. I'll remove it, see what breaks.
<allenap> gmb: Cool.
<gmb> allenap: Answer: nuffink. I'll remove it.
<allenap> gmb: Even cooler.
<gmb> allenap, bac: Thanks.
<gmb> bac: FTR, the windmill tests break on this machine, but not because of the changes I've made; the changes are tested before the failure happens and I'm seeing the same failure on other branches, so think it's a not-enough-CPU issue with this box.
<gmb> I *hate* having to say "It fails, but that's okay"
<bac> gmb: that's fine, in a despicable sort of way
<gmb> Yeah, I feel kinda dirty.
<gmb> I'll submit it to EC2 then take a shower.
<allenap> EdwinGrubbs: Will you have time for a small clean-up review? https://code.edge.launchpad.net/~allenap/launchpad/remove-striped_class/+merge/14996
<EdwinGrubbs> allenap: sure
<allenap> EdwinGrubbs: Thanks.
* allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: leonardr || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> leonardr: what does this do? taggedValue(LAZR_WEBSERVICE_NAME, dict(singular="contact", plural="contacts"))
<leonardr> edwin, that sets the human-readable singular and plural name used by lazr.restful in wadl generation
<leonardr> ordinarily it happens automatically when the class is generated
<leonardr> but since i'm not using the class generation features, i need to do it manually
<EdwinGrubbs> leonardr: is there a reason that ContactSet.path="contact" is singular?
<leonardr> edwin: no, that's a mistake. i'll fix it but i probably won't be using .path anyway
<EdwinGrubbs> leonardr: why do the ITestDataSet.get() methods take a new request parameter but the getAll() methods do not?
<leonardr> edwin: because get() is part of the implementation of ITraverseWithGet (which i made the classes implement instead of Traversable), and getAll() is not part of any interface
<EdwinGrubbs> leonardr: I see that TraverseWithGet has the request parameter, but ITraverseWithGet does not. Does that need to be updated in this branch?
<leonardr> maybe...
<leonardr> edwin: yes, i'll change that while i'm at it
<EdwinGrubbs> leonardr: review sent
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> edwin, here's my incremental diff. i fixed everything we talked about and added a tiny bit more tested code
<leonardr> https://pastebin.canonical.com/24847/
<leonardr> i also cleaned up the prose such as unfinished sentences
<EdwinGrubbs> leonardr: looks good. r=me
<intellectronica> EdwinGrubbs: can i ask for a small review, please?
<EdwinGrubbs> intellectronica: of course
<intellectronica> EdwinGrubbs: great. https://code.edge.launchpad.net/~intellectronica/launchpad/bugs-homepage-searchbox/+merge/15007
* intellectronica changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [allenap, intellectronica] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> EdwinGrubbs: looks like i can get a face-to-face review from deryck here, so don't worry about it
<EdwinGrubbs> intellectronica: is there a purpose to having a bug supervisor and security contact listed in launchpad if they are not using the launchpad bug tracker? or at least should the edit buttons be removed?
<EdwinGrubbs> intellectronica: oops, I didn't see your last comment. nevermind.
* intellectronica changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sidnei> EdwinGrubbs: got time for a review?
<EdwinGrubbs> sidnei: how big is it?
<EdwinGrubbs> sidnei: it looks like I will have a bunch of time between meetings tomorrow at UDS, so I might be able to do it then also.
<sidnei> EdwinGrubbs: the mp says 2100, but 70% of that is under 'examples' in lazr-js
<sidnei> EdwinGrubbs: https://code.edge.launchpad.net/~sidnei/lazr-js/combo-service/+merge/15009
<EdwinGrubbs> sidnei: I'll start on it now, but I probably won't finish until tomorrow.
<sidnei> EdwinGrubbs: im more than happy with that!
<al-maisan> Hello intellectronica, do you mind reviewing a simple and innocuous Soyuz branch? Life at the UDS :)
<al-maisan> *live
<intellectronica> al-maisan: there's no such thing as a simple and iccocuous soyuz branch ... but sure, where are you? :)
<bigjools> al-maisan: he's sat next to me, shall I hit him? :)
#launchpad-reviews 2009-11-19
<al-maisan> intellectronica: it was worth a try :) we can meet on level 3, in the central area, I'll be there in a few minutes.
<intellectronica> al-maisan: oright, will join you there shortly
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mwhudson> thumper: hey, want to review a branch!?
<mwhudson> https://code.launchpad.net/~mwhudson/launchpad/bzr-svn-imports/+merge/15027 fwiw
<henninge> noodles775: Hi! I requested a UI review of you, if that is ok! ;)
<noodles775> henninge: np, although I was chatting with beuno and we thought that, when there's time, it'd be great to always first have a review by someone who's going through the graduation process. So if sinzui is keen to take a look first, I'll then go over it afterwards, otherwise if we don't hear back I'll do it later this afternoon.
<noodles775> is that ok with you?
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> henninge: fine by me
<henninge> noodles775: ^ :)
 * henninge relocates
<allenap> noodles775: Got time for a quick one? https://code.edge.launchpad.net/~allenap/lp-dev-utils/open-project-review-in-browser/+merge/15037
<noodles775> allenap: sure. do we have http://api.* urls?
 * noodles775 has only seen http://lp.net/api/beta/*
<allenap> noodles775: I think the /api/beta urls are for the benefit of the js apis (can't do cross-domain stuff). The api.* domains are old-skool I think.
<noodles775> ok.
<noodles775> allenap: done.
<allenap> noodles775: Thanks.
<gmb> noodles775: Can I get a review of https://code.edge.launchpad.net/~gmb/launchpad/no-sync-from-dupes-bug-484609/+merge/15036 please?
<noodles775> gmb: sure.
<gmb> Ta
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: gmb || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775: I've just spotted some errors circa lines 84 and 85 of the diff - references to multiple bug watches when there's only one. I'll fix it.
<noodles775> gmb: ok, it's just comments in your doctest right? So I'll still start it now.
<gmb> noodles775: Right.
<noodles775> gmb: s/bug_watche/bugwatch on line 80 of diff?
<gmb> noodles775: Yes. 
<noodles775> gmb: also, the import of transaction on line 87 is unnecessary.
<gmb> noodles775: Okidoke. I'll remove it.
* sinzui changed the topic of #launchpad-reviews to: on call: noodles || reviewing: gmb || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi sinzui, let me know if you can/want to do an initial ui review for henninge's branch at:
<noodles775> https://code.launchpad.net/~henninge/launchpad/bug-422466-status-picker/+merge/15035
<sinzui> I do
<noodles775> sinzui: great!
<sinzui> I'll do it now in fact
<allenap> noodles775: I've replied to your review. Thanks for the good comments. It's not urgent that you look back at it, so this isn't meant to be a "me me", just a nod :)
<noodles775> allenap: yep, I'll try it out in a tick. Thanks.
<gmb> noodles775: Changes made and pushed.
<noodles775> Thanks gmb
<gmb> noodles775: Thanks for the review.
<noodles775> gmb: np!
<rockstar> noodles775, can I put a branch in your queue?
<noodles775> rockstar: pop it in the queue, if I don't get to it before EODing, the next person will grab it :-)
<rockstar> noodles775, well, I SHOULD be the next person, but I'm at UDS.
<noodles775> rockstar: yep, but abel will be on tomorrow too (hopefully I'll get through sinzui's and henninge's branches quickly).
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: sinzui || queue [henninge, rockstar] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> rockstar: where is your branch? (I can't see it on activereviews...)
<rockstar> noodles775, intellectronica just walked up.  I can have him review so you can EOD without worry.
<rockstar> noodles775, I haven't submitted it.  I usually line up a reviewer before I submit.
<noodles775> rockstar: ah ok. Great.
<sinzui> henninge: noodles775: I did a ui review.
<henninge> sinzui: cheers
<noodles775> Thanks sinzui, I'll do one too after your branch.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles || reviewing: henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> henninge: is your branch intentionally targeted at db-devel? (or is it just that you created it when devel was closed)
<henninge> noodles775: It depends on yui-3final
<noodles775> henninge: sorry, maybe I missed that... is yui-3final on db-devel but not devel?
<henninge> noodles775: yes, mars landed it into db-devel. If all works out, it will be landed on devel, too.
 * noodles775 is still in the process of going through email from the past week.
<noodles775> Great, thanks henninge.
<henninge> noodles775: I heard flacoste and mars talk about it ... ;)
<henninge> you were in the room, too ;-P
<intellectronica> which reminds me, what's with yui3-final? we still have a few branches that depend on it waiting to land.
<noodles775> Yeah, but I don't hear much when coding ;). But you'll land this on devel if the yui-3final lands there right? (so that it gets more exposure before release etc.)
<henninge> noodles775: definitely
<henninge> intellectronica: it's landed on db-devel
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: henninge || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> and what's stopping it from landing in devel? as noodles775 said we prolly want to have it used on edge a bit before releasing it
<henninge> ask flacoste 
<henninge> noodles775: I somehow anticipated sinzui's comment about moving the edit link closer to the actual data, while pondering it over lunch. If you agree, I'll change that.
<noodles775> henninge: I'm just getting your branch to run it locally now.
<henninge> cool
<noodles775> henninge: but yes, looking at your screenshots (thanks for those!), I'd definitely agree that the edit icon for the item (as opposed to the status) should be next to the item itself.
<flacoste> intellectronica: we want to do QA on staging before releasing to users on edge
<flacoste> intellectronica: just to make sure there isn't any major fuck-up
<intellectronica> flacoste: oright. i'll land stuff on db-devel then. get that tested too
<flacoste> intellectronica: plan is to land on devel on Monday
<intellectronica> excellent. can't wait!
<bac> noodles775: are you EOD or still accepting reviews?
<bac> sinzui: i just sent in a MP.  would you mind reviewing it?  i'll not be here for an hour, though, as i'm out to get lunch.
<sinzui> I do it
<salgado> EdwinGrubbs, you're not reviewing sidnei's combo-service branch, right?  I'm about to start reviewing it so just wanted to make sure you haven't done so
<EdwinGrubbs> salgado: I didn't get very far, so feel free to take it over.
<salgado> ok
<sidnei> salgado: actually, mars just finished it
<salgado> really?
<sidnei> salgado: yep, right before you joined
<noodles775_> henninge: you've got a screenshot there logged in as no-priv with the picker displaying, but when I'm logged in as no-priv I don't have an option to click on? What did I miss?
<henninge> noodles775_: ah sorry, you need to create an entry as no-priv. Actually, create a project first, activate translations on it and upload something to the queue ...
<noodles775_> henninge: ok, doing so now. Thanks.
<noodles775_> henninge: also, when logged in as Carlos, are the Deleted and Blocked items in the picker meant to be disabled? They appear grayed out, but I can click on them and it updates (it could just be my colour-blindness).
<henninge> noodles775_: I chose darkgray for those states ...
<henninge> noodles775_: so, no, they are not grayed out.
<henninge> noodles775_: got a suggestion for a better color scheme? ;)
<noodles775_> henninge: nope - I'm not a great resource for choosing colours - I normally either (1) look for precedents, or (2) use a decent colour-scheme generator.
<henninge> oh, didn't know such things exist
<henninge> noodles775_: anyway, that is easy to fix in css.
<noodles775_> yep.
<noodles775_> henninge: if you don't mind flash, http://kuler.adobe.com/
<henninge> noodles775_: wow, this is really kul!
<noodles775_> :-)
<henninge> noodles775_: bugs uses gray for invalid and won't fix, too.
<henninge> but they don't have disabled items.
<noodles775_> henninge: aha.
<henninge> sinzui: I just saw that I may have broken the jacascript behind the expander button - maybe that was why it was hard to figure out what it does.
<sinzui> henninge: I also tested one of my branches
<henninge> sinzui: but I agree that it looks like it should either be moved to the left of not make use of the "expander" semantics.
<henninge> s/of/or/
<sinzui> henninge: the expander is always on the left on gtk, qt, macos, beos, java swing, and java swt
<henninge> as I said, I agree
<sinzui> henninge: I did not recognise it as an expander since it pointed to my scrollbar. I clicked the info icon and it expanded, then I had to toggle while looking at the other side of my browser to see what happened
<henninge> ;)
<sinzui> henninge: This is definitely a different bug
<leonardr> anyone want to review a lazr.restful branch? https://code.edge.launchpad.net/~leonardr/lazr.restful/double-your-enjoyment-2/+merge/15045
<flacoste> rockstar: i have 11 left before i disapppear in another meeting
<rockstar> flacoste, well, can you show me some code where canonical_url hackery is done in the way that I need to do it for windmill?
<flacoste> rockstar: can you remind me of the problem again?
<rockstar> flacoste, canonical_url returns a url without the port number on it, so it's generally useless for use in windmill tests.
<flacoste> ok
<rockstar> flacoste, so what you said was that we need to install something into canonical_url space that tells it the url.
<flacoste> rockstar: so what you need to do is modify the WindmillLayer testSetUp and testTearDown methods
<flacoste> in testSetUp you should push the following on the config
<flacoste> [vhost.mainsite]
<flacoste> rooturl: http://launchpad.dev:8085/
<flacoste> and so on for all the vhosts
<rockstar> flacoste, ah, okay.
<flacoste> look in configs/testrunner-appserver/launchpad-lazr.conf for the whole list
<flacoste> all vhost.* sections should be pushed
<flacoste> and then you pop that config back in the testTearDown
<flacoste> et voilÃ 
<flacoste> that should work
<rockstar> flacoste, easy peasy pumpkin squeezy.
<sinzui> bac: r=me
<henninge> noodles775_: pushed new version with moved edit icon and new colour scheme.
<noodles775_> henninge: ok, merging now, thanks!
<noodles775_> henninge: did you decide not to put the edit icon also next to the pot file (as sinzui suggested?), it only seems to be next to the template?
<henninge> noodles775_: well, that is what is mainly been used. The file name is seldom changed.
<noodles775_> henninge: OK, makes sense.
<henninge> noodles775_: If we ever get as far as in-line editing all this data, it would be worth adding another icon.
<noodles775_> henninge: well, I thought that was part of the advantage of sinzui's suggestion - that the UI wouldn't change when in-lining the other items (as you'd already have the icon there).
<henninge> :-)
<henninge> shot in the foot
<henninge> noodles775_: I just tried it out and it looks too confusing. Have a look: http://people.canonical.com/~henninge/screenshots/status-picker-more-edit-icons.png
<henninge> noodles775_: the two edit icons for different entries get dangerously close ...
<henninge> noodles775_: I'd rather leave it with one icon per entry.
<noodles775_> Yep, it does produce a lot of edit icons. I've written in the (as yet unsent) review that I'd leave that up to you - I think your previous justification is a good one (that it's normally only the template that users want to edit).
<henninge_> noodles775_: daily disconnect, that is when I know it's time to go home .. ;)
<noodles775_> henninge: yeah, I'm about to do a runner too.
<noodles775_> just sending your review :)
<henninge_> noodles775_: thank you very much! Enjoy your evening.
<noodles775_> henninge, sinzui : ok, review sent, but I'm keen for sinzui's thoughts on two points mentioned in the review.
<noodles775_> Night!
#launchpad-reviews 2009-11-20
* jtv changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jtv] || 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: - || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> hi adeuring!  are you on call?  I put one in the queue and, to make up for it, reviewed one that was already there.  :-)
<adeuring> jtv:  it's quite long diff... 
<jtv> adeuring: looking it up now... I hadn't realized
<jtv> adeuring: argh, the gateway's power supply blew up earlier so I'm on a problematic connection, can't check now.  :(  Never mind then...
<adeuring> jtv: can't you send the "real" diff via mail?
<jtv> adeuring: don't have the branch handy here either
<al-maisan> hello jml, I'd appreciate it if we you could review https://code.edge.launchpad.net/~al-maisan/launchpad/duration-485524 before the sessions start
<al-maisan> s/we you/you/
<henninge> adeuring: are you working on jtv's branch?
<adeuring> henninge: that's 2000 lines... or the diff is wrong
<henninge> adeuring: yeah, I just saw that ....
<henninge> adeuring: so you could take on one for me?
<adeuring> henninge: sure
<henninge> adeuring: cool, let me update the mp
<jml> al-maisan: it's too late for before now.
 * jml suspends
<henninge> adeuring: review requested
* henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [heninge,jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> henninge: ok
<henninge> adeuring: danke
<adeuring> immer grerne :)
<jml> al-maisan, hi
<jml> al-maisan, doing code review tasks now
<al-maisan> jml: thanking you, please let me know if you want to meet and talk (I am currently on level 3 in the Riviere room).
<adeuring> henninge: why did you add "pipes" to .bzrignore?
<henninge> ;)
<henninge> adeuring: that's what "bzr reconfigure-pipeline" does for you ...
<adeuring> henninge: ah, ok
<henninge> I don't know if that is supposed to go into the tree. What do you say, abentley?
<abentley> henninge: It goes into the tree because reconfigure-pipeline is not intended for your use case.
<henninge> abentley: ???
<abentley> henninge: It's meant for people who don't have a shared repository and want to convert their standalone working tree into a pipeline.
<abentley> henninge: since you already have a shared repository, and presumably a lot of location-specific configuration, you should just create a lightweight checkout of your branch.
<sinzui> flacoste: ping
<henninge> abentley: e.g. of devel etc.?
<abentley> henninge: Your branch, not a mirror of upstream.  Well, you have have upstream as your first pipe if you like, but it's not the recommended configuration.
<henninge> abentley: do we have this use case documented somewhere?
<adeuring> henninge: could you add some short documentation to the new function init_status_choice()?
<henninge> adeuring: sure
<adeuring> thanks!
<abentley> henninge: Probably not.
<henninge> abentley: because I have to admin I didn't understand the instructions you just gave me ... :-/
<henninge> *admit*
<abentley> henninge: Instead of running reconfigure-pipeline in your branch, you should create a lightweight checkout of that branch.
<henninge> abentley: in another directory, I assume
<henninge> abentley: and when I create a new pipe?
<abentley> henninge: Yes, it's not a lightweight checkout if you create it in the same directory.
<abentley> henninge: When you create a new pipe, you run "bzr add-pipe".  It will create a new branch/pipe in the same directory as your first branch/pipe.
<abentley> henninge: Perhaps this is a "how do I turn my branch into a pipeline?" question?
<abentley> henninge: By default, branches are pipelines with a single branch in them.
<henninge> abentley: I guess. It's a bit clearer now, I will have to try it out, though.
<abentley> henninge: I mean, all branches are part of pipelines.  If you haven't explicitly added them to a pipeline, then it's a single-branch pipeline.
<henninge> abentley: I understand!
<abentley> henninge: There's no equivalent to "bzr-loom's loomify" because it's not needed.
<henninge> abentley: how do I turn a lightweight checkout back into a branch?
<abentley> henninge: bzr reconfigure.  I'm not sure whether you want a branch or a branch with a tree in it.  The commands are "reconfigure --branch" or "reconfigure --tree" respectively. 
<henninge> abentley: I mean, I have to find a way to get rid of that .bzrignore and the pipes directory
<henninge> I will try that
<abentley> henninge: I suggest: "bzr branch pipes/$FOO ../$FOO; rmdir pipes; bzr switch --force ../$FOO"
<flacoste> hi sinzui
<sinzui> flacoste: I have an extraordinary request: I want a review of https://code.launchpad.net/~sinzui/launchpad/parent-packaging-links-bug-484828/+merge/15092 and I want to run it in production today or Monday.
<flacoste> sinzui: looking at it now
<abentley> henninge: thumper posted a blog about pipelines, though the shelve machinations it describes are thankfully obsolete: http://how-bazaar.blogspot.com/2009/07/breaking-up-work-for-reivew.html
<adeuring> henninge: in HasTranslationImportsView.change_status_action(), aren't ou simply trusting user input, when you omit all the permission checks?
<henninge> adeuring: I don't
<adeuring> so, how/where is the check done?
<henninge> adeuring: checks using check_permissions build on what is configured in zcml and enforced in security.py, right?
<adeuring> rkght
<henninge> This allows to restrict access to attributes and methods.
<adeuring> henninge: ah, now I get it...
<henninge> but here we are restricting the value of parameters, so we have to do the checking ourselves.
<henninge> that is what canSetStatus does, it checks the given user if he's allowed to set the desired status.
<henninge> the user is taken from the view
<henninge> adeuring: does that explain it?
<henninge> adeuring: I spend some time on the last branch which prepared the API for this and thought about these things ... ;-)
<adeuring> henninge: what I was looking for is the call of canSetStatus() in setStatus() ;)
<henninge> is it not?
<adeuring> henninge: no, it _is_ called. Problem was that I asked before I searched....
<adeuring> sorry for the noise
<henninge> np
<adeuring> henninge: r=me. Just one detail (UI stuff, so, nothing ou asked for ;): for example on your screenshot http://people.canonical.com/~henninge/screenshots/status-picker-after.png there are six buttons which pop up a dialog, and the values shown identical for two set of three dialogs. Couldn't you change the dialog title from "Change status to" to "Change status of <queue entry> to" so that it is clear what exactly the dialog is doing?
<henninge> adeuring: the problem is that <queue entry> could be quite long. Unless I just use the filename but even that could have a long path.
<adeuring> henninge: I see. So, let's leave it as it is.
<henninge> adeuring: also be aware that since the screenshots the rightmost edit buttons have been moved to the left, right next to the text of the entry.
<henninge> as Curtis had suggested
<henninge> adeuring: thank you for the review1
<henninge> !
<adeuring> henninge: right. But I was more concerned about the _vertical_ ambiguities: Once you click the the "edit" button, you can't see what queue entry will be affected. 
<adeuring> henninge: you could perhaps mark the complete row of the entry with a different background colour
<adeuring> ...for the time the dialog is shown
<al-maisan> hello jml, how are things? Any luck with the schema patch review?
<henninge> adeuring: We still have some work for this page in the future, so we could include that idea then.
<adeuring> henninge: OK, great
<jml> al-maisan, reviewing it now.
<jml> al-maisan, I've added a comment. Basically, it would help me a lot if you could give more context on the patch. :)
 * jml offline to stalk the corridors.
<deryck> rockstar, https://code.edge.launchpad.net/~brian-murray/launchpad/bug-485229/+merge/15040
<bac> adeuring: can i add a branch to your queue?  barry are you reviewing today?
<adeuring> bac: sure
<barry> bac: i am, but probably not until after lunch
<bac> adeuring: https://code.edge.launchpad.net/~bac/launchpad/bug-485237/+merge/15095
* bac changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [heninge,jtv, bac] || 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: bac || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> bac: r=me
<bac> adeuring: thanks, that was fast!
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jml has anyone mentioned lately how much 'ec2 land' rocks?
* adeuring changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jtv,sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue [jtv,sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: jtv || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* barry changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-11-22
<jtv> lifeless, stub: requesting db review for the schema changes in our Recife feature branch.
<jtv> https://code.launchpad.net/~launchpad/launchpad/recife/+merge/41426
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing:  || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<stub> jtv: Having a look now. On the phone before.
<stub> jtv: So my initial response is similar to lifeless. I don't like that we are embedding ubuntu into our data model when Launchpad is supposed to be distro agnostic. But I also understand that this might be the most practical solution for us for the next few years at least.
<stub> jtv: Or maybe I'm overreacting - when LP is supporting translating multiple distributions, will they all be Ubuntu derivatives?
<stub> I can't help but look at TranslationMessage and notice the msgstr1..msgstr5 columns might fit nicely into the Cassandra model ;)
<stub> jtv: Why is this split into two patches?
<stub> Cool - ALTER INDEX supports renaming now. I think you used to have to use ALTER TABLE to rename indexes.
<jtv> stub: the msgstr0â¦msgstr5 would fit nicely into an array, I guess.
 * jtv catches up on review
<jtv> stub: the ubuntu-specific naming of the column is something we went over at the time.  The upshot was that it followed logically from our strategic direction.  We'll also support Ubuntu derivatives, but we never really supported translating completely separate distros on a par with Ubuntu anyway.
<stub> Cool. Matches what I was thinking. Does that make is_current_distro a better name, or would that just be confusing? I don't think we use 'ubuntu' or other celebrity names in the data model at the moment.
<stub> I'll let either pass - just bringing up the point.
<stub> How does this interact with patch-2208-28-1 and patch-2208-29-0 (the other translation message patches from danilo)?
<stub> (15:09:01) stub: Cool. Matches what I was thinking. Does that make is_current_distro a better name, or would that just be confusing? I don't think we use 'ubuntu' or other celebrity names in the data model at the moment.
<stub> (15:10:11) stub: I'll let either pass - just bringing up the point.
<stub> (15:11:55) stub: How does this interact with patch-2208-28-1 and patch-2208-29-0 (the other translation message patches from danilo)?
<jtv> stub: sorry, was off updating the MP to respond to Robert.
<jtv> I'll have a look at those other patches now.  AFAICT we have two patches because they were written at very different times, by different people, and for different purposesâI don't see any reason against merging them except basic laziness.
<jtv> stub: 2208-28-1 is a cleanup after a model change that was an enabler for the recife branch in terms of managing the complexity.  It drops a column, and replaces some indexes that referenced it (with a lot fewer indexes, knowing Danilo's style of indexing).
<jtv> Ah, the actual "drop column" happens in 2208-29-0.
<stub> So we need to be careful when your branch lands? As I understand it, danilo's patches needed testing on staging
<jtv> stub: yes, that's one of the very next things on our plateâdelayed only by staging updates.
 * stub just approved the review
<jtv> Thanks!
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing:  || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing:  || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Hi abentley!  Danilo's actually not here, so henninge and I are trying to get his branch through.  (We're sort of on the clock).
<jtv> Assuming you'll be ocr'ing today, if you have any problems with the branch, please mention them to either of us.
<abentley> jtv: Hi.  Okay.
<abentley> jtv: henninge is also OCR.  If he approves, I wouldn't normally get involved.
<jtv> (Sorry to pounce on you first thingâjust making sure we don't lose the opportunity due to timezone differences)
<jtv> abentley: yes, henninge's finishing something for the same deadline though.
<jtv> (Plus, we don't want toget _too_ incestuous with these reviewsâa little understanding can be a bad thing :-)
<abentley> jtv: Okay.
<henninge> abentley: thanks
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing:  || queue: [danilo, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jtv: I don't really understand what this code does.
<jtv> abentley: ah, I see there's a bit of conceptual background missing.
<jtv> We're changing our data model.  Every TranslationMessage currently has two flags: "is_current" and "is_imported."  That means that for every translation in Launchpad, we're basically tracking what Launchpad considers the current translation, and what the latest translation is that we saw from upstream.
<jtv> In the new data model, we merge those.
<jtv> So we'll share the same TranslationMessage between a project and an Ubuntu package (where appropriate), and we change the flags to "is_current_ubuntu" and "is_current_upstream" respectively.
<jtv> That doesn't change much for Ubuntu translations, but it changes everything for the project ("upstream") translations.
<abentley> So is_current now means is_imported, and the old is_imported should go away?
<jtv> Not that simple, alas.
<abentley> jtv: What is the code at 65-72 doing?
<jtv> For Ubuntu translations, you could say that is_current merely gets renamed to is_current_ubuntu whereas is_imported is both renamed to is_current_upstream and changed to get its information from a project in LP, rather than Ubuntu imports with a special "this is from upstream" flag set.
<jtv> For upstream translations, the information that used to come from is_imported is basically useless and merely confuses and annoys our users.
<jtv> But the important thing for upstream translations, what used to be is_current, will now come from the is_current_upstream flag.  Which is exactly not the right one.
<jtv> So this script copies, for upstream translations, the old is_current flag to the new is_current_upstream flag.
<jtv> Now for your question.  :-)
<jtv> That looks like a piece of puzzling optimization.
<jtv> (Go away windmill, I'm trying to concentrate!)
<jtv> It's definitely not following our coding standards for indentation, but it's a one-off script so that's not likely to matter so much.
<abentley> jtv: What is is_current_upstream?  It does not appear in the script.
<jtv> abentley: it gets confusingâ¦ this script is to be landed on devel, and run _before_ we migrate.  So it refers to the old-style flag names.
<jtv> So is_imported == is_current_upstream.
<jtv> Anyway, about that strange-looking query:
<jtv> it looks as if it gets "all the translationmessages that share the same set of potmsgsets."
<jtv> That's one of those things we do a lot with message-sharing scripts.
<jtv> So I guess it's not a piece of optimization at all, just a kind of back-and-forth join.
<jtv> TM â POTMsgSet â TM
<jtv> The _reason_ it does this is that there is a unique constraint involving the is_imported flag.
<jtv> The flag basically means "this is the blessed upstream translation, for this potmsgset and language."  There can't be two of those for the same potmsgset & language, so the old ones get cleared first.
<abentley> jtv: I see.
<abentley> jtv: So it seems that the ones that are being set as is_imported are the ones that were formerly TranslationMessage.is_current == True, TranslationMessage.is_imported == False
<abentley> jtv: So it seems that the ones that are being set as is_imported are the ones that were formerly TranslationMessage.is_current == True, TranslationMessage.is_imported == False
<jtv> abentley: the is_current part of that is the really important information; the "is_imported == False" looks like it's just there to save on replication.
<jtv> Setting it to True where it's already True won't change anything, but it will cause the row to be re-replicated which is a waste of time.
<abentley> jtv: Is it possible that 65-72 could be applied to something that was is_current + is_imported, setting it to False when it should be True?
 * jtv ponders
<abentley> jtv: And then since we're ignoring is_current + is_imported, it wouldn't get reset correctly?
<jtv> abentley: I don't think soâit'd be nice to have a helpful docstring there but it looks like the intent of the method is to set the flag to True exactly on the given TMs.
<jtv> First it disables just those that are in the way, then it enables those that it's supposed to set.
<abentley> jtv: And it's not supposed to set those that are is_current + is_imported.
<jtv> abentley: where is the check for is_imported == False, actually?
<abentley> 120+
<jtv> I see, thanks.
<jtv> Well ISTM those TMs will never get passed to the TranslationMessageImportedFlagUpdater.
<jtv> Let me think about that some more.
<jtv> (To see if there's any other way in which they could ever be passed to it)
<jtv> abentley: man, you're good.  You spotted a bad one.
<abentley> jtv: Glad I can help.
<jtv> This is what happens:
<jtv> The code seems to assume that there can be only one is_imported TM per POTMsgSet.
<jtv> In which case, it's perfectly valid to say "I'm doing each TM once, and for each, I'll clear any previous is_imported flags first."
<jtv> But what of another TM for the same POTMsgSet and a different language, that's already been processed?
<jtv> It's had its flag set in a previous iteration, but now gets it callously cleared again "to make room" for a TM it doesn't even compete with.
<abentley> jtv: So are we just missing a condition on language?
<jtv> If only!
<jtv> AFAICT there's a similar situation with divergence (which is the case where TM.potemplate is non-null).
<jtv> So we'd need some extra join conditions: tm1.language = tm2.language and tm1.potemplate is not distinct from tm2.potemplate.
<abentley> jtv: it sounds like you need some extra tests as well, to check that those cases are handled correctly.
<jtv> abentley: absolutely.  I'm very glad you spotted this.  I'll work on that next.  I'll have to submit a separate MP once I make changes to danilo's branch.
<abentley> jtv: You can resubmit this merge proposal, and change the source branch to your own.
<jtv> ah!
<jtv> Thanks for the tip.
<abentley> jtv: np.  I just implemented that recently.
<jtv> This is another one of those subtle reasons why reviews are useful.  :)
<jtv> It's our water-cooler.
<jtv> abentley: I'll want to be sharp for this changeâ¦  I guess I'd better finish it tomorrow to avoid stumbling.
<abentley> jtv: A few style things: at 18, I'd prefer to import from storm.locals, and do a multi-line import.
<jtv> abentley: ah yes, I'll fix that.
<jtv> abentley: that one's done, go ahead.  :)
<abentley> 316 could be reformatted to a normal multi-line import.
<jtv> Done.
<abentley> At 145, I think tm_loop would be better as import_flag_loop.
<abentley> or update flag loop.
<abentley> something about what it does, not what it affects.
<jtv> OTOH this isn't just a loop that _affects_ TMs; it's a loop _over_ TMs.  So it can be insightful to express that.
<jtv> I'm pretty sure the docstring for that last class is wrong.  :)
<abentley> jtv: to me a loop _over_ TMs is "what it affects".
<jtv> It's that, but it's also structure.
<jtv> If you feel strongly about it though, I'll change it.  It's the least I owe you.  :-)
<abentley> jtv: No, not strongly.
<abentley> jtv: Could I get a docstring on _updateTranslationMessages though?
<jtv> (Pushing updated docstring for that class at the end)
<jtv> With pleasure!
 * jtv types
<jtv> abentley: â
<jtv> (22:29:57) jtv: With pleasure!
<jtv> (22:29:59) ***jtv types
<abentley> jtv: I think that's it.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing:  || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> abentley: a showcase for meaningful reviews if ever I saw one.  Thanks again.
<abentley> jtv: You're welcome.
<abentley> jtv: Every time I see a branch with "recife" in the name, I think you guys can't spell recipe.
<jtv> heh
* henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> abentley: I've looked at jtv's branch already, so I'll do a proper review.
<abentley> henninge: Great, thanks.
<henninge> jtv: Is there a good reason to expose validateTranslations in IPOTMsgSet?
<jtv> henninge: yes, the browser code will call it.
<henninge> ah, permissions ;)
<henninge> jtv: r=me
<jtv> thanks henninge
<jtv> Anyone up for a largish Translations review?  I took enough advantage of our OCR already, so would appreciate a volunteer!
<jtv> https://code.launchpad.net/~jtv/launchpad/recife-kill-updatetranslation-browser/+merge/41483
<henninge> jtv: Why don't you go to bed and I look at that mp and we talk about it in the morning (my morning). I 'll make sure I'll be her much earlier than today.
<jtv> henninge: great, thanks.  I'm just wrapping up the day here.
<allenap> abentley: Hi, can you review a change to ec2test please? https://code.launchpad.net/~allenap/launchpad/update-ec2-image/+merge/41485
<abentley> allenap: VALID_AMI_OWNERS should come after EC2Account in the __all__
<allenap> abentley: Oh yes :)
<abentley> allenap: You can avoid the sort, groupby and for loop by using setdefault.  e.g. setdefault(result, revision, []).append(image)
<abentley> allenap: I mean, result.setdefault(revision, []).append(image)
<allenap> abentley: I still want to sort, so I'll still need to sort by revision and unpack.
<abentley> allenap: I think it'll still be simpler, though.
<allenap> abentley: D'oh, yes, you're right. I've also used a defaultdict.
<abentley> allenap: Cool.
* abentley changed the topic of #launchpad-reviews to: On call: - || Reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-11-23
<wallyworld_> StevenK: bambi's b1tch asked me to flick a couple of reviews your way. you should see them in your email. thanks in advance :-)
<thumper> wallyworld_: my headset isn't working
<wallyworld_> thumper: ok, talk later then i guess :-)
<thumper> I can hear you
<thumper> but mumble can't hear me :(
<henninge> jtv:Hi!
<henninge> jtv: I am surprised this works: http://paste.ubuntu.com/535469/
<jtv> hi henninge!
<jtv> Q about my MP?
<henninge> yes
<jtv> It doesn't for me.
<jtv> But give my http connection a bit more time.
<jtv> Ahhh there it is.
<henninge> makeSourcePackage makes an *Ubuntu* source package?
<jtv> What's the surprise?
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing:  || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Heh, I hadn't thought of that. But we now basically assume that all source packages are Ubuntu, don't we?
<StevenK> We shouldn't
<jtv> Because I think you're right: it creates a new distro AFAIK.
<jtv> StevenK: this is Translations, it's special.  :)
<henninge> jtv: we do???
<henninge> oh, right, we do .. we are Translations ....
<henninge> ;-)
<jtv> We are special.
<jtv> Our new data model accommodates only one family of distributions.  Once we have the sharing working, we can worry about shoehorning in moreâif required.
<henninge> So it's our checking if productseries is None -> it's ubuntu
<henninge> but it wasn't in focus ;)
<jtv> 'zacly
<henninge> jtv: but still, I always use the "ubuntu" celebrity, I think.
<henninge> to be explicit
<jtv> That's perfectly correct, just very verbose.  My personal feeling is we'll want a makeUbuntuSourcePackage at some point.
<jtv> Or maybe even a makePOFile/makePOTemplate option for_ubuntu.
<henninge> yes, our dear Ubuntu. :-)
<jtv> Or in this case, our ubiquitous ubuntu.
<henninge> jtv: so can you please be verbose in this test and use ubuntu?
<jtv> Our user-friendly but undeniably un-upstream ubiquitous ubuntu.
<henninge> "un-upstream" as in "not is_current_upstream!
<henninge> "
<henninge> ?
<jtv> Why?  We're assuming that it'll always be either Ubuntu or a derivative.  There's no easy way for the code to discover the difference.  If this ever becomes not what we want, we'll have more places to change and that would be a good time to come up with something more convenient.
<jtv> Exactly.
<henninge> ok, ok ...
<jtv> It's really Ã¼ber-un-upstream.
<henninge> jtv: template.getTranslationPolicy
<jtv> uh-huh
<henninge> I guess you added that in a recent branch?
<jtv> Yes.  It gets the ITranslationPolicy implementation that applies to the template.
<jtv> (Which is either a Project or a Distribution, though a Project may also inherit part of its policy from a ProjectGroup)
<jtv> I considered calling it getPillar, but we (including yours truly) have been overusing that a bit, and it's not quite right here.
<jtv> This also gives us the option to break out the repetitive policy data into its own ORM-backed class in the future.
<henninge> cool
<henninge> jtv: two more things
<jtv> (I'm sitting here facing a lady reading a house-and-garden magazine.  The rear cover shows either a palace or a stately mansion with an elephant either grazing in the massive land around it, or carrying the house on its back.  The perspective makes it unclear.  The front cover says "Humble Living.")
<jtv> Shoot.
<henninge> It feels to me like there should be more test_revert_unselected_translations where plural_indices_to_store has a value other than []
<henninge> there is only one so far
<henninge> only a feeling, though. I did not check test coverage very closely.
<henninge> jtv: also, I think it would be cool to implement contains_translations using "any" ;-)
<jtv> I considered both, believe it or not.
<henninge> any([text is not None and len(text) != 0 for text in translations.itervalues()])
<henninge> ok
<henninge> then we are done, I guess
 * henninge knows that jtv always considers thoroughly.
<henninge> oh, i thing the [...] are not even needed in 2.6
<henninge> s/thing/think/
<jtv> :-)  Thanks.  Two reasons I didn't use any(): call it micro-optimization but I didn't feel like constructing a list for something this small; and to make it look really _nice_ I'd have to evaluate text as a boolean, which we're not really supposed to do.
<jtv> Oh, without the [] we'd probably avoid the listâ¦
<jtv> â¦but that wasn't the main thing.  :)
<jtv> About the tests: I could add test for various set relationships, but it seemed a bit redundant.  As they say, one needs to test the zero case and the one case, and all else just follows.
<henninge> yes but I was just thinking of more conditions around [0]
<jtv> OK, I'll spend a bit more time (not too much :) thinking about things I could test for.  It's a valid concern.
<jtv> But now I need to run to a place where I can't conveniently use wifi.
<henninge> although I had not heard "them" say that yet. Sounds sound, though.
<henninge> run!
<jtv> Assuming I want to keep this laptop dry.
<jtv> Well, "hop" is more accurate at this stage.
<jtv> Limp.
<jtv> Scurry.
<StevenK> gmb: O hai, you're free to do some reviews?
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: StevenK  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> StevenK: I am now.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> gmb: That SHA1 comes from sampledata
<StevenK> It actually comes from the gina_archive in the source tree
<gmb> StevenK: So, can we update the source so that it's obviously manufactured?
<StevenK> gmb: I can update the test to extract the same changelog, run sha1 over it, and then compare them if that would please you more?
<gmb> StevenK: That would work, yes. Please do it.
<StevenK> gmb: I could change it to do .read() and check the output, if you don't like the sha1
<gmb> StevenK: Even better.
<StevenK> gmb: Changed pushed, diff updated
<adeuring> gmb: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-675595/+merge/41595?
<gmb> adeuring: Sure
<adeuring> thanks!
<gmb> adeuring: r=me. Nice work.
<adeuring> gmb: thanks!
* mars changed the topic of #launchpad-reviews to: On call: gmb,mars || Reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: gmb,mars || Reviewing: -,-  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring: I'm heading out for a run, so just put whatever you need reviewing on the queue and I'll attend to it when I return.
<gmb> Oh, or maybe mars will look first :). Hi mars.
<mars> Hi gmb
<adeuring> gmb: ok, thanks for the notice
<adeuring> gmb, mars: could one of you please have a look at https://code.edge.launchpad.net/~adeuring/launchpad/bug-675595-2/+merge/41604 ?
<mars> adeuring, sure
<adeuring> mars: thanks!
* mars changed the topic of #launchpad-reviews to: On call: gmb,mars || Reviewing: -,adeuring  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: gmb,mars || Reviewing: -,adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> salgado can you review https://code.launchpad.net/~sinzui/launchpad/upstream-downstream-bug-links-0/+merge/41371
<salgado> sinzui, sure, but I may not be able to get to it today.  can it wait until tomorrow?
<sinzui> okay
<salgado> cool
* mars changed the topic of #launchpad-reviews to: On call: gmb,mars || Reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> mars: thanks for the review!
* sinzui changed the topic of #launchpad-reviews to: On call: gmb,mars || Reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> gmb, mars: I have a small branch that needs review: https://code.launchpad.net/~sinzui/launchpad/private-persontransferjob-0/+merge/41614
<gmb> sinzui: I'll take it.
* gmb changed the topic of #launchpad-reviews to:  On call: gmb,mars || Reviewing: sinzui,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> sinzui: Please add a comment / docstring to test_no_warning_for_PersonTransferJob() explaining the expected behaviour. Otherwise, r=me.
<sinzui> gmb: thanks. I will update the test
<gmb> Cool.
* gmb changed the topic of #launchpad-reviews to:  On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * gmb calls it a day for reviewing
<abentley> mars: could you please review https://code.edge.launchpad.net/~abentley/launchpad/more-harness/+merge/41619 ?  It's tiny.
<mars> abentley, about to go on a call, but I will as soon as possible afterwards
<abentley> mars: also: https://code.edge.launchpad.net/~abentley/bzr-builder/launchpad/+merge/41623
* abentley changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [abentley, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> mars, do you have time for a short branch? https://code.launchpad.net/~sinzui/launchpad/prefill-homepageurl-0/+merge/41652
<mars> sinzui, sure
* mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> thanks
<thumper> abentley: https://code.edge.launchpad.net/~thumper/launchpad/recipe-binary-builds/+merge/40686
#launchpad-reviews 2010-11-24
<jtv> StevenK: we're on a very intense deadline today, and a critical branch hasn't been reviewedâ¦ could you help henninge out?
<StevenK> jtv: I sure can, link me?
<henninge> StevenK: hang on
<jtv> thanks guys :)
<StevenK> henninge: Still hanging on :-)
<henninge> StevenK: thanks for hanging on ;-)
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/recife-current-tm-view-1/+merge/41690
<henninge> StevenK: stop!
<henninge> StevenK: requisite branch missing ...
<henninge> pre
<StevenK> Hah
<henninge> I think
<StevenK> You say, after I've read through the entire diff
<henninge> StevenK: well, actually, that part needs review, too.
<henninge> ;-)
<henninge> StevenK: mp updated with a comment. Sorry again.
<StevenK> henninge: +1'd, I just need a mentor review
<StevenK> Sadly, my mentor EOD'd about 3 hours ago
<henninge> StevenK: there'll be others waking up ... ;) Thank you very much!
<henninge> StevenK: Your review is for all files in the diff, right?
<StevenK> Right
<henninge> cool, thanks ;)
<henninge> StevenK: "other" is short for "other side" which term is used in other places in the recife branch.
<StevenK> henninge: The UI displays which of those two terms?
<henninge> The UI currently just says "Other:" but the next branch changes that to "In Upstream:" and "In Ubuntu:" respectively.
<henninge> The term "other" or "other side" will not appear in the UI.
<henninge> "next branch" == work in progress
<StevenK> henninge: Then feel free to ignore that bit
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: - || queue: [] || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<stub> jelmer: https://code.launchpad.net/~stub/launchpad/pending-db-changes/+merge/41691 is pretty trivial, almost self reviewable.
<jelmer> hi stub
<jelmer> I'll have a look
* henninge changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: - || queue: [henninge] || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> sinzui, I went to UI review your branch but noticed that rockstar has done so already.  if you'd like a second review just let me know and I'll be glad to do it
<salgado> hi jelmer.  can I add another one to the queue?
<jelmer> salgado: Hi Salgado. Of course :-)
* salgado changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: - || queue: [henninge,salgado] || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> stub: r=me
<stub> ta
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: henninge || queue: [salgado]  || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> henninge: the branch that needs review is lp:~henninge/launchpad/recife-current-tm-view-1 ?
<henninge> jelmer: yes!
<henninge> jelmer: Steven already had a look at it but it needs a second review.
<henninge> jelmer: thanks a lot. I'll be off to lunch soon but would be happy to reply after it, if needed.
<jelmer> henninge: np
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: salgado || len(queue) == 0 || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: salgado || [henninge] || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jelmer: got another one. ;-)
<henninge> jelmer: but only if you can finish it before your EOD ;-)
<jelmer> henninge: Yeah, that should be doable.. I'm here for at least another hour.
<henninge> cool, it's not very big either ..
<jelmer> yeah, I just had a quick look
<jelmer> henninge: r=me
 * jelmer dives back into the {s,blue}prints code
<henninge> jelmer: thank you!
* jelmer changed the topic of #launchpad-reviews to:  On call: jelmer || Reviewing: salgado || queue == []  || This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue == [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> sinzui, did you see my comment on https://code.launchpad.net/~james-w/launchpad/expose-blueprints/+merge/30026?
<sinzui> I did
<sinzui> I set the bug you are working on as high since it is now linked to a branch
<salgado> sinzui, yeah, saw that.  did you also see my branch which addresses your comments?  If I'm not missing anything, it looks like the two remaining issues are the removal of updateLifecycleStatus() from browser code and unexporting of .distribution/.product/.productseries/.distroseries, with setTarget() and proposeForGoal() exported instead of them?
<salgado> jelmer, thanks for the review!  I'll add the docstrings you suggested.  btw, I guess you didn't notice the XXX I left in personHasDriverRights() as I don't quite like that name?
<jelmer> salgado: Yeah, but I don't really have any better suggestions (other than personHasDriversLicense :-P)
<sinzui> salgado, I will look at the branch after my meeting
<salgado> jelmer, heh, ok, I'll remove the XXX then
<salgado> thanks sinzui
<salgado> jelmer, AFAICT, there's nothing on my branch that requires it to land on db-devel rather than devel.  do you agree?
<jelmer> salgado: Yes.
<jelmer> salgado: IIRC you also proposed it against devel, no?
<salgado> I did, just wanted to make sure
<salgado> jelmer, also, do you know if there's a wiki page describing the tags that one should include in the PQM request?  I guess I need the qa-untestable one?
<salgado> or similar
<jelmer> salgado: There is a bug associated with your branch so you don't need the no-qa tag in the commit. You should remove the needs-testing tag on the bug after the branch has landed and add qa-untestable
<jelmer> salgado: I'm not sure if there's a wiki page, it would indeed be useful to have one.
<salgado> ok, cool
<salgado> jelmer, found this: https://dev.launchpad.net/PQMCommitMessages
<salgado> doesn't mention QA tags but as you pointed they're not needed on commit messages anymore. :)
<jcsackett> back
* henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi! If somebody would look at my latest branch I would be thrilled. ;-)
<thumper> abentley: ping
<abentley> thumper: pong
<thumper> abentley: https://code.edge.launchpad.net/~thumper/launchpad/recipe-binary-builds/+merge/40686
<abentley> thumper: https://code.launchpad.net/~abentley/launchpad/better-recipe-errors/+merge/41780
<thumper> abentley: done
#launchpad-reviews 2010-11-25
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Since this is the reviewers channelâ¦ if we're going to write the queue in the topic in python, how do I make it follow our coding standards?
<StevenK> Submit a patch
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge, jtv, StevenK] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> queue.append(StevenK)
<StevenK> jtv: It can't be a tuple, they're immutable :-)
<jtv> StevenK: won't queue = queue[1:] still work?
<StevenK> I think that creates a new instance, doesn't it?
<jtv> Yes, so what?
<jtv> We care about the value of queue.
<StevenK> Yay, semantics :-)
 * StevenK notes lifeless is apparently OCR today
<StevenK> *cough*
<jtv> Why, dear God, does my neighbor have to be such an enthusiast on the saxophone?
<StevenK> jtv: Ah, but is he any good?
<jtv> From the Halls of Montezuma to the Shores of Tripoliâ¦ he's a nice guy but he does make me wish he'd make the trip.
<jtv> It's *just* *not* *a* *saxophone* *melody*
<jtv> Better than some of the other stuff he's been playing today, I'll grant you, but still.
<jtv> I'm no expert but there must be better choices of brass for this.
<lifeless> StevenK: uhh, indeed. OTOH today is insane for me, sorry.
<lifeless> thats why my name isn't in there.
<StevenK> lifeless: Mostly teasing, so it's all good
<lifeless> and it would have been morning only anyway, which finished 6 hours ago
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge, jtv, StevenK, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: unexpected breakthrough with my branch; it turned out to be easy after all.  Shall I review yours now?
<henninge> jtv: please do ;)
<henninge> That's great news! ;)
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [jtv, StevenK, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: we're talking about your -3 branch, right?  I see that -1 is merged and -2 is approved.
<henninge> jtv: right. -2 is merged, too.
 * henninge checks that
<jtv> OK.
<jtv> The UI doesn't say it's merged yet.
<henninge> no, it's not :(
<henninge> I'll do that now.
<jtv> (Also, when I ran a Recife branch this morning, I noticed that the statistics said "changed in Ubuntu" on both sides.
<jtv> )
<jtv> BTW we're supposed to open a card per branch when we split things up like this.
<henninge> right
<henninge> jtv: of cource, now the diff on the MP is wrong, it contains -2, too.
<jtv> Maybe we can still set -2 as a prereq on the -3 MP.
<jtv> henninge: yup, we can.  Shall I just do that then?
<henninge> jtv: please do
<jtv> ok
<jtv> Done.
<henninge> Although I'll land it now anyway, dunno how that affects things
<jtv> (Through resubmission, so don't be alarmed by any weird emails you may get about it)
<jtv> Won't matter.
<henninge> Recife has been update.
<henninge> d
 * jtv pulls
<jtv> henninge: in _makeTranslation, consider naming "is_other" to be consistent with what we do in the factory.
<jtv> henninge: oh never mind, I see it has a different meaning.
<henninge> yes, it makes a "just other" message, not a sharing one.
<jtv> In a somewhat weird wayâwhy not create a suggestion, then activate it on the appropriate side?
<henninge> jtv: activate + review.
<henninge> although, I guess that would work, too.
<henninge> "markReviewed" and "setFlag"
<jtv> henninge: well you're doing the latter anyway.  But I'm talking about setting just the right flag, instead of setting one, then checking if it's the wrong one, and if so clearing it and setting the other one.  That's what I think you do now.
<henninge> it does
<henninge> I do
<henninge> jtv: I can change that.
<henninge> jtv: how's that: http://paste.ubuntu.com/536216/
<henninge> jtv: maybe this reads better ;) http://paste.ubuntu.com/536217/
<jtv> henninge: slow down, I was just looking at the last one.  :)
<henninge> tests still pass with it ...
<henninge> jtv: the second is the same but not a diff, just plain python
<jtv> Ah.  It's fine with me, though an "elif" could probably have been easier.
<jtv> (though with a bit of repetition)
<jtv> Never mind.  It's fine as you have it.
<jtv> henninge: next, now that you've betrayed knowledge of what the method does by renaming it, could you provide a docstring for _assertConfirmEmptyPluralOther?  I found the test hard to deal with because it assumes familiarity with a pretty large, complicated, underdocumented view class.
<henninge> Sure, I can do that.
<jtv> Thanks.
<jtv> henninge: in test_other_translation, I take it the "other translation" is actually a "current translation on the other side"?
<henninge> jtv: yes
<jtv> May be worth saying, to avoid confusion with "another translation"
<henninge> but "current_translation_on_other_side" is such a long name for a variable.
<henninge> jtv: I think that if we consistently use "other side" and "other" in our new code, there shouldn't be any confustion.
<henninge> (eat that "t" if you like)
<jtv> :)
<jtv> Fair enough, but it'd be nice to have just one word in there somewhere to indicate that it's other-side, not just other.
<henninge> I mean "other side" is a concept, just like "imported" was. We did not explain that every time we used it, either.
<jtv> No need to explain it every time, I agree.
<henninge> I can add that.
<jtv> Thanks.
<jtv> (Meanwhile we seem to be in conflict with db-stable againâ¦ I'll see if I can fix that)
<henninge> jtv: weren't you supposed to be at a dinner soon?
<jtv> Yes.
<jtv> So quickly:
<jtv> Several notes on _set_dismiss_flags.
<jtv> The docstring says the flags are always set to False.  Begpardon?
<jtv> (Not your fault I guess, but still: begpardon? :)
<henninge> got your point ;)
<henninge> "The flags have been initialized to False in the constructor. This method activates the right ones."
<henninge> jtv: ^ better?
<jtv> Better!
<jtv> Also, instead of the nested ifs, maybe just something like this:
<jtv>     has_new_other = other is not None and (
<jtv>         self.context.date_reviewed is None or
<jtv>         self.context.date_reviewed < other.date_created)
<jtv> If you think that's easier, that is.  If you prefer the ifs, never mind.
<henninge> usually I don't prefer complicated if constructs.
<henninge> jtv: much better, thank you.
<jtv> A hybrid would work as wellâkeep the outer "if" but remove the inner one.
<jtv> Oh, nm then, glad you like it.  :)
 * henninge had to read and compare first. ;)
<jtv> :-)  Another one that could be just slightly nicer to read is:
<jtv> if len(local_suggestions) == 0 and not has_new_other:
<jtv> instead of if not(len(local_suggestions) > 0 or has_new_other):
<jtv> Again matter of taste though.
<henninge> good idea, just now I stumbled when reading that line. ;)
<jtv> :)
<jtv> Finally, I'd leave out the "OK, let's set some flags" commentâsince that's what the method does anyway.  If we need a comment like that, it's probably a sign that the method gets too long and/or complicated.
<henninge> ... I had considered removing that, too.
<henninge> jtv: actually, just figured out another improvement.
<jtv> ?
<henninge> has_new_other is not needed, I can assign directly to can_dismiss_other
<henninge> and just check for len(locals) == 0
<jtv> Oh, yes.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: None || queue = [jtv, StevenK, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> That way you don't have to keep track of whether your "early escape" condition still applies to the rest of the code in the method.
<henninge> jtv: in all its glory: http://paste.ubuntu.com/536231/
<jtv> glory even?
<henninge> well, "juice" ?
<henninge> s/new/newer/
<jtv> That really is a much nicer read.
 * jtv moves to push the button
 * henninge gets excited
<jtv> done!
<jtv> thanks for the branch
<henninge> Yipee! (sp?)
<henninge> jtv: thanks for the review
<jtv> I don't think spelling of that word is particularly rigorous.  I also just pushed an updated recife with the latest db-stable merged; it had some conflicts outside of translations.
<henninge> jtv: thank you very much. Enjoy your dinner!
<jtv> Thanks, bis morgen!
<henninge> allenap: Nice to see you!
<henninge> a reviewer that is not having a holiday today ... ;)
<henninge> allenap: if you start on jtv's branch, you can ask me about it. I hope I can answer ... ;)
<allenap> henninge: Hi there :)
<henninge> Hi allenap ;)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue = [StevenK, jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> I'm not very quick off the mark today.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue = [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> allenap: Thank you for the review :-)
<allenap> StevenK: Welcome :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> hey allenap!  I got an easy one for you: https://code.launchpad.net/~julian-edwards/launchpad/log-parser-bug-680463/+merge/41865
<henninge> abentley: Hi!
<abentley> henninge: Hi!
<henninge> abentley: Are you able to finish the review for danilo's migration script branch?
<henninge> https://code.launchpad.net/~danilo/launchpad/migrate-current-flags/+merge/41364
<abentley> henninge: Okay.
<henninge> abentley: I have looked into it and it looks good to me now. I can approve it, too, but did not want to ignore you here.
<abentley> TBH, I wasn't sure how you guys wanted to handle it.
<henninge> abentley: danilo's branch is the right one
<henninge> He is unavailable now, so I am going to land this.
<henninge> abentley: is that what you meant by "handle it" ?
<abentley> henninge: No, I mean that when I reviewed, jtv was going to revise it, but I gave him some suggestions on IRC that didn't appear in the review, and I don't know whether they appeared in danilo's branch...
<henninge> abentley: I would not expect so, no :(
<henninge> abentley: AFAICT danilo just fixed the problem jtv had indentified.
<henninge> abentley: that was this channel, right?
<abentley> henninge: yes.
<henninge> abentley: I am trying to find the irclogs to that conversation
<abentley> henninge: I'm just looking at the changes jtv made
<henninge> abentley: where are you looking?
<abentley> henninge: http://bazaar.launchpad.net/~jtv/launchpad/migrate-current-flags/
<henninge> abentley: that only contains cosmetic changes
<henninge> over danilo's branch, I mean.
<abentley> henninge: True.  And I will approve it, but I will note that the changes are missing.
<henninge> abentley: thanks. ;) Do you remember the day and roughly time of day you suggested these changes to jtv?
<abentley> henninge: LP says it was 2010-11-22 and it was probably 13:00 UTC or so.
<allenap> bigjools: I'll look at that one now!
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bigjools || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> allenap: it's a little gross but I have no better ideas
<allenap> bigjools: It's cool. Why do you need the & 0xffffffffL line? If it's a number from a 32-bit field then that seems superfluous?
<bigjools> allenap: I think it's to cope with sign.  I shamelessly copied it from the gzip module source
<bigjools> without thinking much about it, admittedly
<allenap> bigjools: Oh yes, weird. +1
<bigjools> allenap: what do you think?
<bigjools> it'll blow up if we read >~4.2GiB anyway
<allenap> bigjools: I think it can stay. I can't see that it makes a difference, but my bit manipulation is rusty so I might be missing something.
<bigjools> allenap: mine also :)
<bigjools> I figured there was probably a good reason for it to be in the original code
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> anybody interested in reviewing a slightly long diff which exposes blueprints on the API?
<mwhudson> salgado: maybe, but i can't promise a response time better than "today"
<salgado> mwhudson, that'd be great; I wasn't expecting anybody would be able to review it before my EOD anyway. :)
<mwhudson> salgado-afk: sorry about that
<salgado-afk> mwhudson, about what?
<mwhudson> disappearing on you
<salgado-afk> mwhudson, oh, no worries.  did you get my reply?
<mwhudson> salgado-afk: only
<mwhudson> <salgado> mwhudson, that'd be great; I wasn't expecting anybody would be able to review it before my EOD anyway. :)
<mwhudson> did you request a review?
<salgado-afk> mwhudson, oh, good point, I forgot to tell you where my branch is.  do you want me to ask a review from you?
<mwhudson> i think i found it
<mwhudson> https://code.launchpad.net/~salgado/launchpad/expose-blueprints/+merge/41898
<salgado-afk> yeah, that's the one. :)
<mwhudson> cool
 * mwhudson claims 
<wallyworld> thumper: sorted(builds, key=attrgetter('id')) won't work - the named tuple doesn't have an id attribute :-)
#launchpad-reviews 2010-11-26
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Any volunteers for my branch?  It's tiny!  https://code.launchpad.net/~jtv/launchpad/recife-gettranslationrows/+merge/41925
<StevenK> jtv: My only concern is:
<StevenK> + # The message is included, but is still marked as obsolete.
<StevenK> 84	+ self.assertEqual(0, vpoexport.sequence)
<StevenK> I dun geddit. Zero is a magic number?
<jtv> Yes.
<StevenK> jtv: Can you explain how?
<jtv> I would very much like us to use null for that, but 0 is how our forefathers did it and we never quite broke away from that.
<jtv> Every POTMsgSet occurring in a template has a sequence number in that template.
<jtv> (This sequence number is stored, in Entity-Relation terms, as an attribute on the relationship between POTemplate and POTMsgSet.  In the schema you find it in TranslationTemplateItem.)
<jtv> When a later update of that template no longer includes the message, the message's sequence number in the template becomes 0.
<StevenK> Ahhh, it's a refcount of sorts?
<jtv> No, it's a 1-based sequence number for non-obsolete messages in a template, with 0 being a special case for messages that are no longer in the template.
<StevenK> Right
<jtv> Absolutely ought to be null AFAICâif only to make the ordering and the unique indexes sensible.
<StevenK> jtv: I've +1'd it, but you'll need one other
<jtv> Right ho.  Thanks!
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv*] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: approved
<jtv> henninge: molt dankon or whatever the esperanto is :)
 * StevenK high fives henninge 
<henninge> ;-)
<jtv> thanks guys, it's landed.
<jtv> (on recife)
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> topic On call: adeuring || Reviewing: jtv || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> herzlich wilkommen abel :)
<adeuring> jtv: hey jtv!
<jtv> :)
<adeuring> jtv: IN line 37 of the diff, you run "if self.form_is_writeable:" for each loop iteration. I think you can move the "if" outside the loop
<jtv> adeuring: trueâ¦ it's a cachedproperty, but with this short a loop I guess that makes sense.
<adeuring> jtv: yeah, that's a real micro-optimisation...
 * jtv looks again
<jtv> Heh, I had missed how completely nonsensical the loop becomes in one of the two cases.  :)
<adeuring> ;)
<jtv> And now it becomes clear that this is just one big list comprehension.
<adeuring> jtv: perhaps I had not have yet enogh coffee -- but where did the "alt_current.setPOFile(alt_pofile)"  from line 49 go to? Or is it simply unnecessary?
<jtv> adeuring: that's what made me clean this upâalt_current goes into alt_submissions, which is really just [alt_current] + alt_external, so at the end I just have to loop over alt_submissions instead of alt_external.
<adeuring> jtv: ah, thanks! r=me, then.
<jtv> adeuring: thank you.  I'm pushing an update to that loop that also removes a nesting level.
<adeuring> jtv: cool
<jtv> adeuring: diff has updated.
 * adeuring is looking
<adeuring> jtv: again, r=me. I like it!
<jtv> thanks :)
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: jcsackett || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: still around or already a square?
<jtv> henninge: not asquare yet, no
<henninge> jtv: branch pushing
<jtv> See it.
<henninge> jtv: here is the diff of my changes becaue the mp diff will just show added files. http://paste.ubuntu.com/536664/
<jtv> Very helpful, thanks.
<jtv> henninge: beware of expressions like "old model" and "new model" in the codebase.  Many changes down the road someone may read that and not be able to figure out which "new" model you were referring to, way back when.  Make sure you have at least one specific reference near the beginning to set the right expectations.
<henninge> jtv: I fully expect these three files to be removed from the codebase soon ... ;)
<jtv> Even so, form the habit!
<henninge> jtv: I have that habit and have been telling it to others in review. "Document the state, not the change"
<jtv> Good, good
<henninge> but this script *is* about change.
<jtv> Which is why I'm not complaining about the repeated mis-spelling of "in" as "int."  :-)
<henninge> the reason for it existence is that we have an "old model" and a "new model" ...
<henninge> oh
<henninge> Hennign Fast-Finger Eggers
<henninge> ?
<jtv> Then there's "loose," which is an adjective where you mean the verb "lose."
<jtv> Hennign?
<jtv> As in hennigne?
<henninge> kind of ;)
<jtv> Â«For this reason, all current messages in \n source packages need to get their "is_current_upstream" flag set.Â» â You mean the ones in upstream projects, right?
<henninge> "lose has lost an 'o'", how could I forget that ... ;)
<henninge> oh
<henninge> of course, that is a very dumb error
<jtv> Also, I find it a bit confusing that you seem to rename all references to "imported" to ones with "upstream," but do not update "current" to "ubuntu" where appropriate.  Frankly I'd be more comfortable with this branch if you left all the variable names etc. intact, and only changed identifiers where needed to port this to Recife.
<henninge> I find that more confusing, actually ...
<henninge> but where did I leave "current" in ?
<jtv> Well it is more confusing to read, but it's a lot less work to review!
<henninge> oh, sorry
<henninge> ok, I found the "CurrentTranslation" class
<jtv> Looks to me like you let all uses of "Current" (with the capital C) in place.
<jtv> e.g. test_getCurrentNonimportedTranslations_current_upstream
<jtv> Which is doubly confusing because I can't tell if that means "current on the upstream side" or "current as in the old model, and current-upstream."
<jtv> Given that this is a migration script, I'd have no problem with reading a script documented along the lines of the old model but with just the identifiers updated to work in the new model.
<jtv> We still have plenty of code like that in the Recife branch anyway.
<henninge> ok, ok
<henninge> jtv: http://paste.ubuntu.com/536667/
<henninge> Renames for Current
<henninge> jtv: still reviewing or sobbing in a corner? ;-)
<jtv> henninge: just a sec
<jtv> henninge: thanks, that's betterâa Â¾ reduction
<jtv> Oh, no it's not
<jtv> it's incremental over the last one.
<jtv> That only makes things harder.
<jtv> henninge: I really don't see the point in having to review 500 lines of diff for renaming two flags in one script.
<jtv> This was meant to be a near-trivial change.
<henninge> jtv: but it is just renaming - that reads fast, does it not?
<jtv> It makes it a lot harder to see whether any of the renamings are wrong.
<henninge> You dign't expect you to check if I renamed every item correctly
<henninge> jtv: I can reduce, though, if you like.
<jtv> Well if you replace all use of "imported" or "current" all over the place, I _do_ need to check that because it's so easy to get it wrong!
<henninge> jtv: ok, then stop and let me redo it.
<jtv> Thanks!
<henninge> jtv: here you go, the "minimal impact" edition. ;-)
<henninge> http://paste.ubuntu.com/536678/
<jtv> How very environmentally conscious.  :)
<jtv> henninge: you may find makeCurrentTranslationMessage's current_other parameter helpful.
<henninge> oh, I forgot about that
<jtv> (In the tests I am working on, I also have a helper for making a message that's current only on the other side)
<henninge> yes, I am missing that
<henninge> We should include that in the factory
<jtv> Maybe, yes.
<jtv> henninge: there's a test that creates a message that's shared in upstream and Ubuntu, but at the same time diverged.  That's a bit weird.
<henninge> Hm, maybe you need to think "old model"?
<jtv> I can't promise that makeCurrentTranslationMessage will do thatâor that makeDivergedTranslationMessage will accept it.
<henninge> It's current, imported and diverged.
<jtv> Ah, of course, in the old model it's possible!
<jtv> Have you checked that makeCurrentTranslationMessage really produces such a message?  It may quietly ignore one of the parameters in this strange case.
<henninge> jtv: the test does, does it not?
 * jtv unscrews eyes
<henninge> This is in test_updateTranslationMessages_diverged, right?
<jtv> ah, the test goes directly into the tm to set the flag anyway.
<jtv> Yes.
<jtv> And by the way, thanks for also cleaning up that misplaced "variants" docstring!
<henninge> I thought that had been done, actually. Aaron had complained about it, too.
<jtv> I did it in my branch based on Aaron's review, but then danilo went in and fixed the bug in the script in his own branch.
<jtv> The fixes were purely cosmetic, which is a bit pointless in a one-off script, so we never made the effort to backport them.
<henninge> right and I landed danilo's branch directly
<jtv> henninge: you're reviewed.  Good night!
<henninge> now I remembe I had considered doing Aaron's bidding but decided against it.
<jtv> And God speed.
<henninge> jtv: thank you very much! ;-)
<jtv> np
<henninge> Yes, willneed that ...
<jtv> Hope to see our branch landed Monday morning!
<henninge> So do I
 * henninge eyes buildbot
 * henninge goes to lunch
<gmb> Hi adeuring, could you review https://code.edge.launchpad.net/~gmb/launchpad/bug-276723/+merge/41952 for me?
<adeuring> gmb: yure
<adeuring> sure, even
<gmb> Thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: gmb || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> gmb: r=me..   thanks for taking the boring task of fixing all these trivial test failure
<gmb> adeuring: No problem. I seriously considered rewriting a bunch of those tests as unit tests, but then I decided that I'd like to actually land the branch before Christmas.
<adeuring> gmb: yeah, completely understandable ;)
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || Reviewing: - || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: - || Reviewing: gmb || queue: [] ||  This channel is logged:  http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
