#launchpad-reviews 2009-11-02
* al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: -,- || queue: [sinzui, al-maisan<http://tinyurl.com/ycqtx3t>] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> intellectronica and henninge: when you open up shop please note that the branch in the queue fixes a release-critical bug.
<al-maisan> henninge: when you open up shop please note that the branch in the queue fixes a release-critical bug
<henninge> al-maisan: oh right, I am OCR today! ;)
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: -,- || queue: [sinzui, al-maisan<http://tinyurl.com/ycqtx3t>] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> henninge: :)
<henninge> Shop's open. Please feel free to stroll around and look at the offerings, I'll be with you soon.
<henninge> noodles775: I just requested an r-c review from you. Please feel free to ask me about it when you get to it.
<noodles775> henninge: will do.
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: al-maisan || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
<henninge> al-maisan:  The branch looks really good but I am wondering about test coverage.
<al-maisan> henninge: aha
<al-maisan> henninge: do you think it needs more/different testing?
<henninge> al-maisan: I am just wondering if it would be possible to extend the test to test the case where the user has permission to upload to a series other than currentseries.
<al-maisan> henninge: I can do that, no problem at all.
<henninge> al-maisan: currently it only tests "allowed for currentseries, denied for others"
<henninge> al-maisan: cool, I'd feel much better about that.
<al-maisan> henninge: I'll ping you with the incremental diff in a few minutes.
<henninge> al-maisan: did you already run the test suite on this? I am surprised that no other tests need to be adapted.
<al-maisan> henninge: package sets are a pretty narrow niche .. but I will run the package upload tests as well .. thanks for pointing this out!
<henninge> al-maisan: np
<henninge> al-maisan: one last issue. I am not familiar with _schema_circular_imports.py but I think I see what it does.
<al-maisan> henninge: it's the avoidance of circular imports..
<henninge> al-maisan: yes, so I gathered. I am just trying to figure out how it works.
<henninge> al-maisan: I trust, though, that this is correct the way you did it, so no real issue for the review.
<henninge> ;-)
<al-maisan> the LP API metadata is manipulated afterwards out of the interface declaration scope 
<al-maisan> henninge: thanks
<henninge> al-maisan: np, r=me
<henninge> (what I meant)
<al-maisan> henninge: thank you very much indeed.
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || (r-c requests skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
<stub> henninge: https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14155 is an rc canditate
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: stub || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
<al-maisan> hello henninge, I needed to tidy up a few things to get the requested tests to work: http://pastebin.ubuntu.com/307456/
<al-maisan> henninge: just want to make sure you approve of these changes as well
<henninge> al-maisan: I understand
<al-maisan> thanks
<henninge> stub: I am onto yours next.
<al-maisan> henninge: I also ran all package upload and archive related tests to make sure nothing breaks.
<henninge> cool
<henninge> al-maisan: so you had to make these changes just to be able to extend the test in this way?
<al-maisan> henninge: yes, in order to manipulate archive permissions based on package sets, I needed to change the API to pass the package sets as opposed to their names.
<al-maisan> that was the main change really.
<henninge> al-maisan: btw, you seem to be using the API exclusively to test this. Is there a reason for that?
<al-maisan> henninge: yes, this functionality is presently only exposed via the LP API.
<henninge> al-maisan: ah
<al-maisan> the UI is still to come
<henninge> al-maisan: and now a packageset can only be passed in as an object, not represented by its name, as used to be the case, right?
<al-maisan> henninge: yes, the reason being that package names are *not* globally unique any more
<al-maisan> now, the combination of a package set name and a distro series is unique
<al-maisan> s/package names/package set names/
<henninge> al-maisan: ah, I understand.
<al-maisan> I could have added a 'distroseries' parameter but I thought it was more straight-forward to just pass in the package set *object* 
<henninge> al-maisan: but shouldn't there be some obsolete code now, that did the lookup from name to object?
<al-maisan> henninge: let me check quickly
<al-maisan> henninge: good catch, the ArchivePermission._nameToPackageset() method is now superfluous.
<al-maisan> henninge: http://pastebin.ubuntu.com/307472/
<al-maisan> henninge: actually .. on a second thought .. scratch that .. the methods involved are internal (as in not exposed via the LP API) and it may still be OK to pass in package sets or package set names in this case.
<al-maisan> the methods involved = the IArchivePermission methods in the most recent diff
<al-maisan> henninge: hrm .. this is a bit complicated .. I guess one important aspect is that IArchivePermission methods are exposed *indirectly* via the IArchive LP API and that's where some of the confusion arose
<henninge> al-maisan: sorry, team call atm. Won't be long.
<al-maisan> henninge: cool.
<henninge> al-maisan: so, your patch remains as it was, right?
<al-maisan> henninge: yep
<al-maisan> henninge: this change is still required: http://pastebin.ubuntu.com/307456/
<henninge> al-maisan: yeah, I believe you ... ;-)
<henninge> al-maisan: and the new test looks very good! Thanks for adding that!
<henninge> al-maisan: still r=me
<al-maisan> henninge: thank you for prodding me :)
<henninge> ;)
<al-maisan> .. and thanks for the review
<henninge> np
 * henninge lunches
<al-maisan> hello henninge-lunch, I have another release critical branch that needs a review: https://code.edge.launchpad.net/~al-maisan/launchpad/sync-470411/+merge/14301
<al-maisan> henninge-lunch: believe it or not but it's a 1-liner :)
<henninge> al-maisan: done, r=me
<al-maisan> henninge: thanks a million :)
<henninge> al-maisan: nee, war doch nur *eine* Zeile! ;-)
<al-maisan> aha :)
<henninge> al-maisan: eine Million Zeilen hÃ¤tte ich abgelehnt ...
<al-maisan> henninge: ich hÃ¤tte es Dir nicht 'mal Ã¼belgenommen :)
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: stub || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
* abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: stub, - || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
<wgrant> jml: Can you please review https://code.edge.launchpad.net/~wgrant/launchpad/distroseries-source-format-selection-db/+merge/14304?
* danilos changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: stub, - || (r-c candidates skip the queue) queue: [sinzui, danilo, danilo, danilo] || This channel is logged: http://irclogs.ubuntu.com/
<danilos> henninge, abentley: mine are not RC-candidates, so do give precedence to what is :)
* henninge changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: -, - || (r-c candidates skip the queue) queue: [sinzui, danilo, danilo, danilo] || This channel is logged: http://irclogs.ubuntu.com/
<henninge> danilos: I don't see why strip_last_newline is needed?
<danilos> henninge, because gazillion of tests would have to be changed if it was something like rstrip("\r\n")
<henninge> danilos: ;-)
<danilos> henninge, well, not gazillion, but something like 4 very obscure tests, and if it was just .strip(), then something around 14 :)
<henninge> danilos: ok, but my hunch was correct, that rstrip would be enough to do what you need!
<danilos> henninge, also, .rstrip() strips all final newlines, and with some comments, whitespace should stay there
<danilos> henninge, almost, I started with that, but tests correctly brought header comments to my attention, and they usually have a few newlines before the initial message, and I feel they belong there :)
<danilos> henninge, I would have changed the few tests if it were not for these cases where we should keep extra newlines
<henninge> danilos: sorry, phone call
<danilos> henninge, was it for me?
<henninge> danilos: ;-)
<henninge> danilos: no, it was my dad
<henninge> danilos: the other thing: strip_last_newline is testing for all versions of line breaks, but you only do split("\n") eventually.
<danilos> henninge, oh, ok then :)
<henninge> danilos: is there a magic in there that I forget that does line break type detection?
<danilos> henninge, well, I don't have to fix all problems with that code? also, .split("\n") would work correctly with DOS-style line endings as well
<danilos> henninge, and no, there isn't anything special that you are missing
<henninge> >>> "Henning\r\nEggers".split("\n")
<henninge> ['Henning\r', 'Eggers']
<danilos> henninge, see :)
<henninge> danilos: if you are fine with that behaviour ... 
<danilos> henninge, now, you can insert "#" in front of 'Henning\r' and 'Eggers' and it'll be fine
<danilos> henninge, we've got another set of bugs for dealing with \r's, but I am not touching those right now :)
<henninge> danilos: ok, ok. r=me
* henninge changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || (r-c candidates skip the queue) queue: [sinzui,  danilo, danilo] || This channel is logged: http://irclogs.ubuntu.com/
<danilos> henninge, thanks
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<sinzui> abentley: I have an rc-candidate to fix a problem with the product-release-finder: https://code.edge.launchpad.net/~sinzui/launchpad/prf-files/+merge/14311
<sinzui> oh, that is a bad diff
 * sinzui deletes and tries again
<sinzui> abentley: RC candidate: https://code.edge.launchpad.net/~sinzui/launchpad/prf-files/+merge/14312
<abentley> sinzui: r=me.
<sinzui> thanks
<abentley> sinzui: You should make two separate review requests, one for RC one for code.
<sinzui> yes, you are right
<sinzui> I added RC so that you would see it should hop in front of non-rc branches
<abentley> sinzui: If you requested an RC review from flacoste, I would see that also.
<sinzui> I see that I was not thinking
<abentley> sinzui: Since I can't provide an "rc code" review, the Canonical Launchpad Engineering review request will never be satisfied.
<flacoste> sinzui: do you have a r-c review request for me?
<abentley> flacoste: We were discussing https://code.edge.launchpad.net/~sinzui/launchpad/prf-files/+merge/14312
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com/
#launchpad-reviews 2009-11-03
<noodles775> Hi stub, is your branch (bug 461800) already through ec2 test etc., and ready to land? (or is there other manual testing that you had to do?)
<mup> Bug #461800: new-slave.py no longer works <current-rollout-blocker> <Launchpad Foundations:In Progress by stub> <https://launchpad.net/bugs/461800>
<stub> noodles775: That script is only tested on staging - everything in database/replication is outside of our test suite.
<noodles775> stub: great, so land away then :)
<stub> noodles775: ok. It was a branch of db-devel - is that going to be a problem with the current buildbot situation?
<noodles775> stub: strange? the MP says that it's for merging into devel... 
 * stub double checks
<stub> noodles775: Yes, you are right. It can land on launchpad/devel.
<noodles775> stub: would it be quicker to get it qa'd if you land directly into db-devel?
<noodles775> I'm about to try sending off a manual ec2 test run of db-devel...
<stub> noodles775: Not really
<noodles775> OK, then devel it is. Thanks!
<al-maisan> hello gmb, when you start your day: I have a branch fixing an r-c bug for review: https://code.edge.launchpad.net/~al-maisan/launchpad/subsets/+merge/14343
* al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || (r-c candidates skip the queue) queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com
<gmb> al-maisan: Thanks. I'll get started on it shortly.
<al-maisan> gmb: thanks
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: al-maisan || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> al-maisan: Looks good. r=me. Go ahead and request an rc review for it.
<al-maisan> gmb: thank you very much.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<al-maisan> noodles775: ^^
<noodles775> Thanks al-maisan 
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<al-maisan> gmb: here's another likely r-c candidate: https://code.edge.launchpad.net/~al-maisan/launchpad/getbyname-472608/+merge/14354
<al-maisan> please take a look when you get back.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: al-maisan || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> al-maisan: Okay, looking now.
<al-maisan> gmb: thanks!
<gmb> al-maisan: Looks good, r=me.
<al-maisan> gmb: thank you very much!
<gmb> al-maisan: You know it's a good review day when you get small Soyuz branches ;)
<al-maisan> gmb: :)
<bac> gmb: quiet day?  i'll be joining you here after i grab a quick bite to eat.
<gmb> bac: Very quiet. I've reviewed entirely 2 RC requests, so everything must be working perfectly and we need fix nothing ever again.
<bac> gmb: your assessment is spot on IMO.  perhaps i'll take a longer breakfast in light of our accomplishments.
<gmb> A wise move.
<noodles775> hey bac! There's 3 'In progress' registry crb bugs listed at https://bugs.edge.launchpad.net/launchpad-project/+bugs?field.searchtext=&orderby=-importance&assignee_option=any&field.assignee=&field.bug_reporter=&field.bug_supervisor=&field.bug_commenter=&field.subscriber=&field.omit_dupes.used=&field.has_patch.used=&field.has_cve.used=&field.tag=current-rollout-blocker&field.tags_combinator=ANY&search=Search
<noodles775> woops, sorry.
<noodles775> But just wondering whether you know if they are really still in progress, or what their status is?
 * bac looks
<bac> noodles775: i know the two distroseries timeouts are being investigated.  i'm not sure about the other.
<bac> noodles775: based on the last comment of bug 437184 i'd guess that edwin is working on it
<mup> Bug #437184: New "See full publishing history" location is less useful <current-rollout-blocker> <post-3-ui-cleanup> <trivial> <Launchpad Registry:In Progress by edwin-grubbs> <https://launchpad.net/bugs/437184>
<noodles775> bac: thanks - it's just getting quite late if they need QA on staging.
<bac> noodles775: yes, i agree
<leonardr> gmb, can you put https://code.edge.launchpad.net/~leonardr/launchpadlib/choose-your-client/+merge/14361 in the queue?
<gmb> leonardr: Sure
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: leonardr || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> leonardr: RC candidate?
<leonardr> gmb: no
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: leonardr, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> bac: Could you take a look at https://code.edge.launchpad.net/~gmb/launchpad/affects-me-spinner-bug-458766/+merge/14360 for me? It's not RC.
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: leonardr, gmb || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> bac: I've just noticed that I've introduced some lint in the last hunk of the diff. Please ignore that; I'm fixing it now.
<bac> ok
<gmb> leonardr: Your branch looks good. r=me.
<leonardr> gmb, that was quick, ok
<gmb> leonardr: No, I'm just single-minded. I can review it again if you'd like ;)
<leonardr> haha
<gmb> I've also had two years of reviewing cprov's Soyuz patches. That makes you faster at reading code...
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: -, gmb || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, gmb || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> argh
<al-maisan> :)
* Ursinha changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, gmb || (r-c candidates skip the queue) queue: [Ursinha] || This channel is logged: http://irclogs.ubuntu.com
<gmb> Ursinha: Do you have an m-p for your branch?
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: Ursinha, gmb || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<Ursinha> gmb, yes sir
<Ursinha> gmb, https://code.edge.launchpad.net/~ursinha/launchpad/bug-391569-add-last-changed-column/+merge/14366
<gmb> Ursinha: Thanks.
<gmb> Ursinha: Is this RC?
<bac> gmb: r=bac.  sorry it took so long, i got pulled into a long stand-up call
* bac changed the topic of #launchpad-reviews to: on call: gmb, - || reviewing: Ursinha, gmb || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> bac: No worries, it's not RC.
<gmb> Thanks.
<bac> gmb: it even works with webkit
<gmb> bac: Glad you said so... I was wondering if it might just be for me that it worked. Webkit compatibility seems more'n a bit flaky in places.
<bac> gmb: yeah, that's why i try testing stuff where possible
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac|| reviewing: Ursinha, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> Ursinha: r=me with some stylistic changes (see my comments, they're pretty minimal).
<gmb> Nice job.
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: -, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> bac: I'm going to go an try and get some hacking done. Give me a shout if the sky starts falling.
<bac> gmb: ok.  have a call at the top of the hour that may last an hour or so.  can you keep on eye out here during that time?
<gmb> Sure.
<bac> gmb: thanks
<adeuring> bac: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-435556/+merge/14368 ? (140 lines diff)
<bac> adeuring: is this for an RC?
<adeuring> bac: no
<bac> adeuring: i have a call starting in 4 minutes.  i can review it or in an hour or you can ask gmb
<adeuring> bac: sure, no problem. (i think gmb doesn't want to do more reviews today)
<bac> adeuring: ok i'll get to it when i can
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || (r-c candidates skip the queue) queue: [abel] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> bac: sure, no hurry. (the branch will sit anyway around until PQM opens again)
<gmb> bac, abel: I'll take it - I was OTP.
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> adeuring: ^^
 * gmb curses nickname changes across servers
<adeuring> gmb: thanks!
* bac changed the topic of #launchpad-reviews to: on call: bac-otp || reviewing: abel, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac-otp || reviewing: abel, - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<Ursinha> gmb, thanks for your review and the tips
<gmb> adeuring: Can you re-push your branch? There's no diff showing up and I can't branch from it; I get a "not a branch" error.
<gmb> Ursinha: You're welcome.
<adeuring> gmb: done. new branch, new MP
<gmb> adeuring: Thanks.
 * gmb goes to get a drink while the branch updates
<gmb> adeuring: Weird; that branch still shows as "Not pushed to yet" Could you push it to another location on LP for me? I wonder if something in codehosting has gone *boing*
<adeuring> gmb: what do you mean with "other location"?
<gmb> adeuring: Try `bzr push lp:~adeuring/launchpad/foo` rather than just `bzr push`.
<adeuring> gmb: ah, ok
<adeuring> gmb:  lp:~adeuring/launchpad/review-435556
 * gmb looks
<gmb> adeuring: Yep, I think something on codehosting is broken; both branches are showing as not having been pushed to.
<adeuring> gmb: right, but you can branch from them.
<gmb> adeuring: Interesting; I can't. 
<adeuring> odd...
<gmb> Ah, well, I can't *merge* from them. Hang on...
<adeuring> gmb: here's the diff: http://paste.ubuntu.com/308634/
<gmb> adeuring: Thanks.
<gmb> adeuring: Your changes look good. r=me.
<adeuring> gmb: thanks1
<adeuring> erm, thanks!
* gmb changed the topic of #launchpad-reviews to: on call: bac-otp || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> gmb: could you please formally give your review here: https://code.edge.launchpad.net/~adeuring/launchpad/bug-435556/+merge/14370 ?
* bac changed the topic of #launchpad-reviews to: on call: bac-lunch || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<gmb> adeuring: Done
<adeuring> gmb: thanks!
<EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/disable-summaries-for-distroseries/+merge/14374
<sinzui> EdwinGrubbs: thanks for getting this done so quickly
<EdwinGrubbs> sinzui: bac doesn't seem to be around, can you review ^^^
<sinzui> I will
<sinzui> Edwin: I am not excited by the "Not displayed on distroseries page for performance reasons."
<sinzui> Also, this is both distroseries +index and +milestone
<sinzui> The change also removes the milestone summary
<sinzui> EdwinGrubbs: You could replace the technical message with
<sinzui>     <tal:summary          content="structure milestone/summary/fmt:shorten/80" /> 
<sinzui> so that users can see something about the milestone
<EdwinGrubbs> sinzui: are you saying that you don't want this change to affect both the distroseries +index and +milestone?
<EdwinGrubbs> sinzui: oh, I see, you are saying that the comment about the "distroseries page" is inaccurate.
<sinzui> correct.
<sinzui> Your fix should fix both page :)
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<EdwinGrubbs> sinzui: new diff posted: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/disable-summaries-for-distroseries/+merge/14374
<sinzui> EdwinGrubbs: Looks good to land.
<EdwinGrubbs> sinzui: thanks
<EdwinGrubbs> noodles775, flacoste: can I get an RC for https://code.edge.launchpad.net/~edwin-grubbs/launchpad/disable-summaries-for-distroseries/+merge/14374
<al-maisan> hello bac, I have a branch that fixes an r-c bug: https://code.edge.launchpad.net/~al-maisan/launchpad/uploadurl-472929/+merge/14382
<al-maisan> Could you please take a look?
<bac> al-maisan: on it
<al-maisan> bac: thanking you.
<flacoste> EdwinGrubbs: done
<EdwinGrubbs> thanks
<bac> al-maisan: at line 16 of your diff, is self.context.distribution a dictionary?
<al-maisan> bac: self.context is an IArchive instance
<al-maisan> and distribution is one of its properties
<bac> al-maisan: right but your indexing into it with ...distribution[series] so i wonder if that is a dict
<bac> argh!  s/your/you're
<al-maisan> bac, that's right, Archive implements a __get_item__() method
<bac> al-maisan: so can you use .get(series) and avoid the try/NotFoundError ?
<al-maisan> bac, wait, it's Distribution that has that method.
<al-maisan> bac, let me look.
<bac> al-maisan: yes, see if you can collapse lines 15-18 to
<bac> series = self.context.distribution.get(series)
<al-maisan> bac, ah, I see what you mean. Just treat it like a dict.
<bac> yes
<al-maisan> hmm ..
<al-maisan>       File "/home/mhr/canonical/lp-branches/uploadurl-472929/lib/lp/soyuz/browser/archive.py", line 294, in _traverse_permission
<al-maisan>         series = self.context.distribution.get(series)
<al-maisan>     ForbiddenAttribute: ('get', <Distribution at 0xbbef1d0>)
<al-maisan> bac ^^
<al-maisan> I was trying http://pastebin.ubuntu.com/308740/
<bac> al-maisan: ok then!
<al-maisan> sorry
<bac> al-maisan: i'm confused by the multiple checks at 13 and 19.  can 20-21 be moved under the check at 13?
 * al-maisan looks
<bac> perhaps not
<al-maisan> bac: so, on lines 15-18 I am making sure that the series passed in by the user (via an URL param) is actually there.
<al-maisan> if I can't find it, it is the same as if it was missing i.e. the result is a 404
<al-maisan> otherwise I look up the package set, and, if I find that I attempt to find at least one archive permission
<al-maisan> bac: please note also that I tested the branch on the Soyuz dogfood system.
<bac> al-maisan: was there a reason for s/grouchy-thunderbird/thunderbird/ ?
<al-maisan> bac: yes, I wanted 2 package sets with the same name
<bac> ah, ok
<al-maisan> in order to test that they are disambiguated on a distro series basis
<bac> gotcha
<bac> al-maisan: r=bac.  thanks!
<al-maisan> bac: thanks a million
<al-maisan> !!
<bac> al-maisan: you going to try for an r-c from flacoste?
<al-maisan> bac: I did ping him on lp-code
<al-maisan> flacoste: the branch in question is: https://code.edge.launchpad.net/~al-maisan/launchpad/uploadurl-472929
<al-maisan> could I please have an r-c for it?
<al-maisan> flacoste: sorry for nagging but the midnight UTC deadline is drawing closer.
<flacoste> al-maisan: has this been QA-ed on dogfood yet?
<al-maisan> flacoste: yes indeed.
<flacoste> al-maisan: ok, rc=me, did you update LPS with your UPDATE query yet?
<al-maisan> flacoste: I am working on it as we speak.
<al-maisan> flacoste: thanks for the rc.
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<mwhudson> abentley: want to review this branchmergeproposaljob join fix?
<mwhudson> it's about 15 lines
<abentley> mwhudson: Okay.
<mwhudson> abentley: you should have an email by now
<abentley> mwhudson: r=me
<mwhudson> abentley: thanks
#launchpad-reviews 2009-11-04
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<jtv> stub: care to review this tiny reordering?  https://code.edge.launchpad.net/~jtv/launchpad/message-sharing-rearrange-commits/+merge/14342
<stub> approved
<jtv> hi mrevell!  I've written up that article about trying out Translations on staging.  Care to proof-read it?  (Warning: it's pretty long).
<mrevell> jtv, I'd love to, thanks man. Wanna email it or do you have a link?
<jtv> mrevell: the email is on its way
<jtv> mrevell: what does strike me about it is how long it all gets, what with the bzr setup.  Wish there were some easy https-based way to get people started or something.  It's liberally interspersed with links to your documentation though.
<jtv> (14:16:36) jtv: mrevell: the email is on its way
<jtv> (14:17:44) jtv: mrevell: what does strike me about it is how long it all gets, what with the bzr setup.  Wish there were some easy https-based way to get people started or something.  It's liberally interspersed with links to your documentation though.
* jtv changed the topic of #launchpad-reviews to: n call: - || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
* jtv changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<jtv> stub: removing the entire debug output stanza and gc.collect() from the script for production... that ok with you?
<jtv> I mean, in the branch you reviewed
<stub> jtv: Yup
<jtv> thx
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: EdwinGrubbs || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> barry: ping
<sinzui> barry: noodles775_: flacoste: I hope for a review and an RC for https://code.edge.launchpad.net/~sinzui/launchpad/full-page-width/+merge/14427
<flacoste> sinzui: why did you add a .full-page-width CSS class?
<flacoste> sinzui: ignore me, i understand
<flacoste> sinzui: can you add a comment above the CSS rule pointing to a test-case for it (what people should look at when they update it?
<sinzui> There is no nest case for it. The behaviour was seen because the markup was invalid
<sinzui> flacoste: I considered not adding the CSS rule. The nesting experience as a surprise. It was not something obvious, so I added a rule to prevent it from happening again. It is like the clear rules in the CSS that define after rules to ensure the core rule is consistent
<flacoste> sinzui: that in a comment would be enough
<sinzui> okay
<sinzui> /* The content is already full width; the content should remain full width. */
<sinzui> flacoste: The branch that landed the invalid markup was fixing the layout of two edit icons for webkit. There were no test changes, we just reviewed the UI
<al-maisan> Hello EdwinGrubbs, could you please review https://code.edge.launchpad.net/~al-maisan/launchpad/parc-456551/+merge/14430 ?
<al-maisan> It's a potential r-c candidate.
<EdwinGrubbs> al-maisan: Im out of the office running an errand. Is this something i can review by just looking at the diff on my phone?
<al-maisan> EdwinGrubbs: sure, http://pastebin.ubuntu.com/309759/
<al-maisan> EdwinGrubbs: please note: it is the first 20 lines of the diff that matter.
<al-maisan> EdwinGrubbs: I need to grab dinner but will be back later.
<EdwinGrubbs> al-maisan: What is the last else?
<al-maisan> EdwinGrubbs: the last else are primary archive (ubuntu) uploads
<al-maisan>         else:
<al-maisan>             archives = [self.policy.archive]
<EdwinGrubbs> al-maisan: i dont quite understand the difference between the archive for the elif and the one for the else
<al-maisan> EdwinGrubbs: the 'elif' clause is for uploads to PPAs
<EdwinGrubbs> al-maisan: r=me
<al-maisan> EdwinGrubbs: thanks!
<Ursinha> all: do I have to do something else besides filing a merge proposal to get a db review from stub?
#launchpad-reviews 2009-11-05
<danilos> EdwinGrubbs, hi, do you have time to take a look at https://code.edge.launchpad.net/~danilo/launchpad/bug-474874/+merge/14450
* danilos changed the topic of #launchpad-reviews to: on call: EdwinGrubbs || reviewing: - || (r-c candidates skip the queue) queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com
<danilos> noodles775, al-maisan: I assume you are not doing any reviews today?
<al-maisan> danilos: if need be I can review.
<danilos> al-maisan, it's very short, just adding some DB permissions for a script: https://code.edge.launchpad.net/~danilo/launchpad/bug-474874/+merge/14450
<noodles775> danilos: I'm not no, sorry.
<al-maisan> danilos: OK .. give me 5 minutes and I'll review it.
<danilos> al-maisan, excellent, thanks a lot
<al-maisan> you are welcome 
<danilos> noodles775, no problem, al-maisan is taking care of it :)
<al-maisan> danilos: this is supposed to go into 'devel' .. is that correct?
<danilos> al-maisan, yes, but I am actually going to ask for this to be CPed (or perhaps not, GRANTs might be enough)
<danilos> al-maisan, note that security.cfg changes do not need to go into db-devel
<al-maisan> danilos: fine, just checking.
<al-maisan> danilos: looks good, r=me
<danilos> al-maisan, thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com
* danilos changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> al-maisan: fancy a relatively small review? (or is already time for "Feierabend" for you?)
<al-maisan> adeuring: a small one is OK.
<adeuring> al-maisan: thanks! https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-udev-submissions-noise-reduction-2/+merge/14479 (130 lines; just two tuples changed)
<sinzui> beuno: ping
<beuno> sinzui, hi
<sinzui> your suggest for the yes-no widget rocks. it hard to do
<al-maisan> adeuring: OK
<sinzui> beuno: I'll work on it later today and see if I can make a universal dsolution
<beuno> sinzui, because it uses db enums, yadayadayada?
<sinzui> beuno: no actually not
<sinzui> beuno: The launchpadformview custom_widget function does not work with macro-style widgets
<sinzui> beuno: making my widget work or fixing LaunchpadFormView is a big win for reuability
<al-maisan> adeuring: so, this is a 'devel' branch, right?
<adeuring> al-maisan: yes
<al-maisan> cool.
<sinzui> beuno: Your suggestion is *much* better that what I did. I spent more than an hour trying to craft a sentence that would work with yes/no. or true/false, or on/off.
 * sinzui thinks all the boolean radio macros suck.
<beuno> sinzui, so I don't think yes/no will work well, no matter how we craft the phrases, if this is an urgent branch, I'd be happy to trade an approve now for a fix before week 4
<sinzui> beuno: no not urgent. I completely agree with your suggest
<beuno> sinzui, so can we still be friends if I push back on this?
<sinzui> beuno: I think you misunderstand. I have no intention of landing this branch until that widget rocks.
<al-maisan> adeuring: LP is still chewing on lp:~adeuring/launchpad/hwdb-udev-submissions-noise-reduction-2
<beuno> sinzui, ah! great!  :)
<al-maisan> I can't even merge it
<adeuring> al-maisan: did you try to simply branch it?
<al-maisan> yes
<beuno> sinzui, other branch approved, it's a great improvement, thank you
<al-maisan> adeuring: this is very funny:
<al-maisan> $ bzr merge lp:~adeuring/launchpad/hwdb-udev-submissions-noise-reduction-2
<al-maisan> bzr: ERROR: Not a branch: "bzr+ssh://bazaar.launchpad.net/~adeuring/launchpad/".
<adeuring> al-maisan: odd... Let me paste the diff
<al-maisan> am I missing something?
<al-maisan> adeuring: please do.
<adeuring> al-maisan: http://paste.ubuntu.com/310697/
 * al-maisan looks
<al-maisan> adeuring: r=me
<adeuring> al-maisan: thanks!
<adeuring> al-maisan: und schÃ¶nen Feierabend!
<al-maisan> adeuring: in 30 minutes or so :) but thanks anyway!
<gary_poster> barry: I'd like your thoughts (consider it somewhere between a late pre-impl and a pre-review :-) ) on this: http://paste.ubuntu.com/310732/  It is supposed to be a fix for https://bugs.edge.launchpad.net/launchpad-foundations/+bug/475550
<mup> Bug #475550: _pythonpath whitelist of clean_modules is too fragile <Launchpad Foundations:In Progress by gary> <https://launchpad.net/bugs/475550>
 * barry looks
<gary_poster> ty
<gary_poster> barry: The old warning still has value, now that I think of it.  Here's a variant:
<gary_poster> If I find any of the namespace packages using the approach in the pastebin, then I go through sys.modules again.
<gary_poster> if anything in sys.modules begins with ``${one_of_the_namespace_packages}.`` (notice the dot) then blow u[
<gary_poster> up
<gary_poster> complaining, as before, that _pythonpath should be imported earlier
<barry> gary_poster: what's an example of "fewer attributes" (or really different number of them)?
<gary_poster> argh, my second idea doesn't work.  OK, here's the idea.
<gary_poster> but I can make it work.
<gary_poster> barry: In this example, ``example`` is a "real" package with only an empty __init__.py
<gary_poster> >>> import example
<gary_poster> >>> dir(example)
<gary_poster> ['__builtins__', '__doc__', '__file__', '__name__', '__path__']
<gary_poster> >>> import lazr
<gary_poster> >>> dir(lazr)
<gary_poster> ['__doc__', '__name__', '__path__']
<gary_poster> lazr is one of these namespace things
<gary_poster> so, barry, I think what I can do instead is look for __file__
<gary_poster> no
<gary_poster> that might not match some C modules, right?
<barry> right
<gary_poster> argh
<gary_poster> barry: But C modules would actually have something exposed *other* than just those three, so the approach as given works.  What it *can't* do is actually recognize if there's a problem (e.g., lazr.uri was imported before) and complain, unless I switch to just hardcoding the namespace packages.
<gary_poster> I'd rather not do that.  a different kind of fragility
<barry> so inspect.getfile() actually tests for object.__file__ to see if it's a builtin module or not
<barry> right
<gary_poster> btw, I discovered why our deb zope packages don't have this problem: they are installed in debian with the egg-style __init__.py, not this crazy .pth thing.
<barry> maybe...
<barry> 1. look for module.__file__; if it exists it's fine
<barry> 2. if not, look for module.__package__; if it exists it's a builtin; if not it's a pth namespace package
<barry> >>> import _socket
<barry> >>> _socket.__package__
<barry>  
<gary_poster> huh
<barry> >>> for name in dir(_socket):
<barry> ...   if name.startswith('_'):
<barry> ...    print name
<barry> ... 
<barry> __doc__
<barry> __name__
<barry> __package__
<barry>  
<gary_poster> barry, cool.  That might work.  To make sure I understand, I am right that the presence of __package__ in this case is kind of random--that is, an implementation detail that might very well change?
<gary_poster> s/I am/am I/
<barry> gary_poster: i'd have to look at the code to know that ;)
<gary_poster> heh, ok
<barry> gary_poster: oh, let me also check py2.4
<gary_poster> For me on 2.4:
<gary_poster> >>> import _socket
<gary_poster> >>> _socket.__package__
<gary_poster> Traceback (most recent call last):
<gary_poster>   File "<stdin>", line 1, in ?
<gary_poster> AttributeError: 'module' object has no attribute '__package__'
<barry> fsk
<gary_poster> otoh, it has a __file__ ;-)
<barry> yeah, so "implementation detail that might very well change" == True
<gary_poster> right
<barry> how strange!  why did python 2.6 lose _socket.__file__?!
<barry> seems like that would still be pretty handy, and that _socket.__package__ is useless
<barry> yay for nothing in the NEWS file about this
<sinzui> barry: gary_poster: I have a small RC candidate for review: https://code.edge.launchpad.net/~sinzui/launchpad/noneable-bug-475433/+merge/14487
<barry> sinzui: looking...
<barry> sinzui: mp has no diff yet
<sinzui> oh bugger
<sinzui> I think we are waiting for both the branch scanner and the mp cron
<sinzui> barry: do you want a pastebin?
<barry> sinzui: sure
<sinzui> http://pastebin.ubuntu.com/310757/
<barry> sinzui: r-me
<barry> er, r=me
<gary_poster> barry: I think this will work.
<gary_poster> (1) if there's a __file__ leave it alone; it's fine.
<gary_poster> (2) if there is a name in the dir that is not __*__ and the full dotted name is not in sys.modules, leave it alone, it's fine.
<gary_poster> (3) Otherwise, we have a namespace package, and we can act on that.  Moreover, if there *were* any names in sys.modules, that means that _pythonpath was imported too late.
<sinzui> barry: thanks
<gary_poster> Going to sketch that
<barry> gary_poster: sounds reasonable.  i'm going to grab some lunch now.  feel free to paste anything for when i get back
<gary_poster> barry: thank you, will do.
<gary_poster> barry, fwiw: http://paste.ubuntu.com/310788/
<gary_poster> going to make an MP
<gary_poster> flacoste, barry: https://code.edge.launchpad.net/~gary/launchpad/pythonpath/+merge/14490
<flacoste> gary_poster: i'll leave barry do the actual code review
<gary_poster> flacoste: cool
<gary_poster_> barry: I'm on this nick for a sec :-)
<sinzui> flacoste: I have a small RC candidate for review: https://code.edge.launchpad.net/~sinzui/launchpad/noneable-bug-475433/+merge/14487
<flacoste> sinzui: approved, add it to CRB
<sinzui> thanks.
<barry> gary_poster: looking
<gary_poster> barry: thanks.  on call, so will have latency
<barry> gary_poster: r=me
<gary_poster> barry, thank you
<abentley> rockstar: I can has review? https://code.edge.launchpad.net/~abentley/launchpad/fix-rewrite/+merge/14498
<rockstar> abentley, is this for RC?
<abentley> rockstar: Yes.
<rockstar> abentley, I wonder if we should also fix the stdout flushing issue
<rockstar> abentley, also, r=me
<abentley> rockstar: thumper wanted to fix it by invoking the python interpreter with -u, so I've gotten mbarnett to do that.
<rockstar> abentley, ah, okay. great.
#launchpad-reviews 2009-11-06
<jtv> Urgent review needed, folks!
<jtv> It's for a good causeâunbreaking buildbot
<jtv> and enabling a re-roll
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<adeuring> jtv: what do you need reviewed?
<jtv> adeuring: https://code.edge.launchpad.net/~jtv/launchpad/db-bug-476218/+merge/14532
<jtv> tiny but urgent :-)
 * adeuring is looing
<jtv> dankeschÃ¶n
<adeuring> jtv: r=me
<jtv> adeuring: vielen Dank!
<adeuring> jtv: gern geschehen :)
<jtv> ...und gerne gesehen :-)
 * noodles775 r-c's
<noodles775> jtv: ok, land away :)
 * jtv approaches the landing strip
<gmb> adeuring: Good morning. Would you be able to take a look at https://code.edge.launchpad.net/~gmb/launchpad/subscriber-portlet-timeout-bug-393476/+merge/14502 for me?
<adeuring> Morning gmb, sure, I'll look
<gmb> Thanks.
<adeuring> gmb: r=me
<gmb> adeuring: Excellent, thanks
<wgrant> adeuring: Thanks.
<adeuring> wgrant: welcome 
<wgrant> adeuring: I have another really easy (sed) one there, if you have time...
<adeuring> wgrant: sure
<adeuring> wgrant: I am good in forgetting small things. So, don't hestitate to kick me next week, when PQM opens again, to submit you branch
<wgrant> adeuring: Heh, will do. Thanks.
<wgrant> adeuring: can you flip the MP status itself to Approved?
<adeuring> wgrant: done. (good example how i can forget things ;)
<wgrant> adeuring: Well, *everybody* forgets that.
<adeuring> wgrant: r=me for yoor brnach with the sample data fix
<wgrant> adeuring: Thanks muchly.
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
* barry changed the topic of #launchpad-reviews to: on call: adeuring,salgado,barry || reviewing: -,-,- || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<barry> salgado, adeuring would either of you care to review a fairly simple change to a contrib script in launchpadlib?  https://code.edge.launchpad.net/~barry/launchpadlib/475547-cmb/+merge/14482
<salgado> barry, sure, I'll take it
<barry> salgado: thanks
<salgado> barry, r=me
<barry> salgado: thanks!
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado,barry || reviewing: -,-,- || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> adeuring: salgado: barry: I have a small fix https://code.edge.launchpad.net/~sinzui/launchpad/cancel-gpg-bug-391630/+merge/14553
<adeuring> I'd like to duck out... working on an urgent bug...
* adeuring changed the topic of #launchpad-reviews to: salgado,barry || reviewing: -,- || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> my work is not urgrent
<salgado> sinzui, you might want to check test_logintoken.py in lib/c/l/browser/tests.  it has some infrastructure for testing the Cancel action of logintoken views
<salgado> I think your test could be moved there and benefit of such infrastructure
<sinzui> salgado: Does it know then the view is missing with the form so that we never get to the cancel action?
<salgado> missing with the form?
<sinzui> salgado: The test was pretty conclusive when I added the field that the user submits. I walked through the initialize() method and then validate() to make sure the cancel action was reached
<sinzui> salgado: The view's initialize() method  makes the field required. The form is always validated before the success action is called
<salgado> sinzui, right, but I don't see why you're telling me all that
<salgado> I mean, is this going to be a problem if you use LPFormHarness to test the view?
<sinzui> salgado: I think the view was at fault, and the test belongs in the view...
<sinzui> salgado: I do not see test_logintoken.py
<salgado> sinzui, it's on db-devel
<sinzui> so is my branch
<salgado> and test_logintoken.py is a view test
<salgado> lib/canonical/launchpad/browser/tests/test_logintoken.py
<sinzui> bugger, I think I branched from the wrong tree
<sinzui> i did
 * sinzui adds test
<sinzui> salgado: the test harness is bad
<sinzui> I added the test, then removed the change I made to the initialize() method. The test passed. 
<sinzui> Oh, I see, the test is indeed naive on the very point is was trying to make. salgado, The test assumes that no data is submitted during the cancel action. That is wrong.
<salgado> sinzui, does it assume that or does it just don't expect anything to be submitted?
<salgado> s/anything/any data/
<sinzui> The form data is hardcodes to an empty dict. I have a fix
<sinzui> I do not have a fix
 * sinzui looks again
<salgado> sinzui, don't bother with that if it's too much work.  I only suggested it because I thought it could make things simpler
<sinzui> I am bothered and I am going to make it work.
<EdwinGrubbs> salgado, barry: I have to go to lunch now, but I have a pretty easy branch for review at https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-234051-bug-project-info/+merge/14556
<barry> Edwin-lunch: i can do it
* barry changed the topic of #launchpad-reviews to: salgado,barry || reviewing: -,edwin || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> salgado: I will retrack my branch and set the bug as fix-released by you. Your validator changes were a global fix
<salgado> no way
<salgado> all of that just because you branched from the wrong tree. :/
<sinzui> yep
 * sinzui finds lunch
* sinzui changed the topic of #launchpad-reviews to: salgado,barry || reviewing: -,edwin || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<beuno> sinzui, I will wait for your review on Edwin's branch before I say anything
<barry> beuno: on edwin's branch, i like that he's added the pillars under the bugs, but why aren't they all linked?
<beuno> barry, good comment to make on the mp  ;)
<barry> beuno: exactly :)
<barry> beuno, Edwin-lunch branch reviewed
* barry changed the topic of #launchpad-reviews to: salgado,barry || reviewing: -,- || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> beuno: Edwin-lunch: I just commented about the alignment of the badges
* sinzui changed the topic of #launchpad-reviews to: salgado,barry || reviewing: -,- ||  queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> barry: can you take a brief look at https://code.edge.launchpad.net/~sinzui/launchpad/latest-release-portlet-bug-424893/+merge/14565
<barry> sinzui: sure, sec
* barry changed the topic of #launchpad-reviews to: salgado,barry || reviewing: -,sinzui ||  queue: [] || This channel is logged: http://irclogs.ubuntu.com
<sinzui> beuno: ping
<beuno> sinzui, pong
<sinzui> beuno: I am having last minutes doubts about https://bugs.edge.launchpad.net/launchpad-registry/+bug/400643
<sinzui> We agreed to do
<sinzui>     Ubuntu >> 10.04 >> "bzr" source package
<sinzui> But I think it should be
<sinzui>     Ubuntu >> Lucid (10.04) >> "bzr" source package
<beuno> sinzui, I prefer that, yes
<sinzui> oh actually, I see examples of this: 6.10 (Edgy)
<sinzui> beuno: What is the official way to present version and code name for series? products are 1.23 "code name"
<beuno> that's a good question
<sinzui> beuno: My example is bad, that was a milestone
<sinzui> products use bzr 2.1 series
<beuno> Karmic (9.10) or 9.10 (Karmic)
<beuno> I think those would be the choices
<beuno> I don't like the quotes really
<sinzui> Lucid LTS
<barry> sinzui: r=me
<sinzui> We have yet to accommodate LTS in the name
<sinzui> thanks barry
<beuno> sinzui, is LTS part of the string?
<barry> beuno, sinzui i can't exactly say why, but i think Karmic (9.10) looks better
<beuno> barry, it's more readable
<sinzui> barry" We like words more than numbers, which is what this bug is really about
<beuno> starting with numbers is hard to read
<beuno> and hard to compare
<barry> beuno sinzui is this a generic change though?  I'm not sure I'd like to see (for Mailman): Vital Signs (3.0.0a4)
<barry> well, maybe that doesn't looks so bad
<beuno> sinzui, I'd just bite the bullet, and it expose it as "CodeName (name)" everywhere
<sinzui> beuno: I was going to suggest that. I may need more than one branch, but it does not require me to think. just jfdi
<beuno> sinzui, +1
<sinzui> barry: I was pondering a change to series. Use a mix to define all things common to all kinds of series. define title as a combination of version and codename and enforce universal consistentcy
<barry> sinzui: that makes sense, as long as there is a code name.  of course we can't require a code name (i.e. your rc patch)
<sinzui> distroseries have a title attribute that users must provide. product series to not have a codename or a displayname.
<sinzui> This problem is really the same scope as fixing all the name, displayname, title, summary, description attributes on all objects
<barry> sinzui: those should really be in a common interface
<sinzui> Hes they should
<sinzui> yes
<sinzui> Instead of a UI fix next year, we should do a model fix. Solve the project and projectgroup problem
<barry> sinzui: +1
* barry changed the topic of #launchpad-reviews to: - || reviewing: - ||  queue: [] || This channel is logged: http://irclogs.ubuntu.com
#launchpad-reviews 2010-11-08
<thumper> lifeless: sure
<lifeless> thanks
<thumper> lifeless: done
<lifeless> thanks
<jtv> thumper: while you're at it, can you review this one for me?  https://code.launchpad.net/~jtv/launchpad/recife-policy-mixin/+merge/40300
<jtv> (that was deliberately phrased for a flippant rebuttal, so feel free if it's not convenient)
<thumper> jtv: if it was small I'd say yes, but I'm in the middle of something.  Sorry.
<jtv> no worries
 * jtv scans for victims
<jtv> bac!
<jtv> Are you up for a non-small review?
<bac> jtv i can be.  can it wait a little bit as i just started something?
<jtv> bac: yes it can
<lifeless> ahhh the sweet smell of real urls in the morning
<lifeless> it smells like victory
 * jtv finds this obvious crack in lifeless' sanity a good reason to ask:
<jtv> you're suggesting a standalone class to implement translation policy.  Were you thinking of a schema change?
<lifeless> jtv: I wasn't, but perhaps one is needed.
<lifeless> s/needed/possibly beneficial.
<jtv> I see it making a lot of sense at this point.
<lifeless> we have -way- to many mixins
<lifeless> when you've got 1 mixin its a problem. 20+ mixings is arggggh zomg mode
<jtv> Yes, we do have too many of them.
<jtv> And in this case, there's actual state that's being replicated into multiple classesâbasically a denormalized object design.
<lifeless> is there a good reason for that ?
<jtv> Best I can come up with is "progressing insight."
<jtv> In other words, "no but that may not have been so obvious at the time."
<jtv> There's 2 identical fields in Product, ProjectGroup, and Distribution.
<jtv> I'm trying to expose and unify the common patterns.
<jtv> Extracting it into a separate database table could be a lot of work, of course.
<jtv> Some joins will change shape.
<jtv> The new objects' lifetimes have to be managed etc.
<jtv> Too much work for now.  I think I'll go with a standalone non-db class first.
<jtv> lifeless: I'm looking at implementing this as an adapter.
<jtv> lifeless: so you'd say "ITranslationPolicy(pillar).makeSomeDecision()"
<jtv> but you can also adapt certain other objects to get the ITranslationPolicy that applies to them.
<jtv> Does that make sense to you?
<lifeless> that seems a decent start
<lifeless> as for adapter vs pillar.translation_policy - shrug, its largely just spelling (except those pesky lifetimes that you mentioned)
<jtv> Yes, and I don't just want to keep adding to the interfaces.
<jtv> bac, lifeless: reworking the policy object so that it's not a mixin becomes too much work for this branch; I'll have leave that for a separate branch.
 * jtv goes out for a bite
<stub> lifeless: Should https://code.launchpad.net/~stub/launchpad/update-storm/+merge/40264 go through without review? It consists entirely of unicode casts, updates to modern Storm syntax and delints at 2.4k lines of diff.
<bac> jtv: return the favor?
<jtv> bac: honour compels me.
<bac> jtv: lucky for you i'm not as prolific.  a measly 88 line diff.  https://code.launchpad.net/~bac/launchpad/bug-671539/+merge/40309
<jtv> bac: prolific?  I have more changes waiting that I carefully cut out of the diff to keep its size down.  :-)
<jtv> bac: I find the "if not person.is_team: enabled = True else: enabled = False" a bit hard to read.  Have you considered this alternative?
<jtv> owner_is_individual = (not person.is_team)
<jtv> â¦(â¦, enabled=owner_is_invidual)
<bac> jtv: yeah, i'll make that change.
<jtv> Thanks.
<bac> brain fart
<bac> any other comments, jtv?
<jtv> bac: whoops, got sidetracked!  Getting back to it now.
<jtv> bac: Shouldn't the section heading in the story test be "---"-underlined instead of "==="-underlined?
<jtv> (I do not like rest-style headings)
<jtv> bac: approved.
<bac> jtv: i'll look at the formatting.  i thought i had followed the style used in the file but perhaps i messed up
<jtv> bac: or the previous author of the file got it wrong, or we just have a harmless inconsistency.
<jtv> Hey, who's up for a review that I actually tried to make fun?  https://code.launchpad.net/~jtv/launchpad/independence-for-ihastranslationtemplates/+merge/40321
<jtv> matsubara maybe?
<jtv> gmb then?  you're always in for a laugh, no?
<matsubara> jtv, I'm not an official reviewer
<jtv> ah!
<jtv> well good morning, anyway.  :)
* 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
<matsubara> good morning :-)
<jtv> Still holding that T-shirt.  :)
<jtv> henninge, do you want to review it then?
<henninge> jtv: oh, yes I should ...
<jtv> Since it's your day and all :)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Thanks.
<henninge> Yes, I will have to find a replacement for next week.
<jtv> We're not usually that picky about missing a review day.
<henninge> I don't think I should be doing normal reviews *and* r-c reviews...
<jtv> At least not on this side of the world, where usually there aren't any OCRs anyway.  :)
<jtv> henninge: why do you say normal reviews and r-c reviews?  has there been a change that I'm unaware of?
<henninge> jtv: I am release manager so I will be the one doing r-c reviews next Monday.
<jtv> ah
<jtv> I forgot that you were rc
<henninge> rm
<jtv> sorry, rm
<jtv> rm -rf
<henninge> In my world: Returned Missionary ;-)
<jtv> (Seriously, I once saw Siemens advertise their OS called, if I remember the spelling correctly, RM/UNIX)
<henninge> :-D
<henninge> Those Germans...
<allenap> henninge: Fancy taking on another branch?
* allenap changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [jtv, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> henninge: I've put myself in the queue, but feel free to eject me.
<henninge> jtv, allenap: Sorry, lunch got in the way. I will relocate now and look at your branches after that.
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: r=me Thank you.
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: allenap || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> henninge: Thanks!
<henninge> allenap: I have not yet used ARRAY's
<henninge> s/'//
<allenap> henninge: Dang, I misread and thought that r=me was for me :) Thank you anyway!
<henninge> allenap: oh, I am sorry.
<jtv> thanks henninge
<allenap> henninge: Hehe, don't be sorry, it was my fault.
<abentley> henninge: is [jtv] already reviewed?
<jtv> abentley: yes I am
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: allenap, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> ups, chansubjupd error
<henninge> ups=oops(de_DE)
<abentley> henninge: We should have a web page we can use to set the channel topic :-)
<jelmer> abentley: Hi
<jelmer> abentley: Can I add a MP to your queue?
<abentley> jelmer: sure.
<jelmer> abentley: https://code.launchpad.net/~jelmer/launchpad/672476/+merge/40349
<allenap> henninge: I have to go soon, so ask soon if you have any questions about my branch :)
<henninge> allenap: sorry, I was about to write the review. I actually understand what's going on now ... ;)
<henninge> allenap: I don't have questions, though. You can look at it in the morning. Nothing serious just some suggestions.
<allenap> henninge: Awesome, thank you.
<abentley> jelmer: 59: s/iterabel/iterable
<abentley> jelmer: Personally, I find using "with" with files more elegant than try/finally.
<abentley> jelmer: Why do you test whether htpassword exists (and is a file) and delete it, instead of just overwriting it?
<abentley> jelmer: is it secure to use token.person.name[:2] as a salt?
<jelmer> abentley: Thanks, fixed.
<jelmer> abentley: I usually don't bother with "with" out of fear of missing a "from __future__ import with_statment" somewhere
<abentley> jelmer: we're all-2.6 now.  You shouldn't import with_statement anymore.
<jelmer> abentley: Overwriting it would work too I guess. That, and the token.person.name[:2] are simply the same as they were in the generate_ppa_htaccess.py script.
<abentley> jelmer: yeah, I realize this is mostly just reorganizing old code.
<abentley> jelmer: But using token.person.name[:2] as a salt is security-by-obscurity, and now that we're open-source, it's no longer obscure.
<jelmer> abentley: it'd break a few existing tests though, so I wonder if I should leave that for a separate branch and file a high-priority bug about it for now.
<jelmer> abentley: What do you think?
<james_w> salts can be known
<abentley> james_w: Oh?  I would have thought that would make it trivial to forge the password.
<james_w> abentley, no. Assuming you are talking about a real salt, and not something wrongly called that, they are not keys
<james_w> if they were then they wouldn't be separate
<abentley> james_w: we're talking about salts as used by the crypt C function.
<james_w> what they do is prevent exploiting all passwords at once. If there were no salts you could generate one rainbow table and then decrypt everyone's ciphertext at once
<james_w> adding a salt means that you have to attack each ciphertext in turn
<james_w> so they do no need to be secret, but they do benefit from being well-distributed
<abentley> james_w: okay, thanks for that clarification.
 * jelmer didn't know that either
<james_w> (if num salts << num ciphertexts then you can decrypt a lot of ciphertexts with on rainbow table)
<james_w> maybe a comment in the source?
<abentley> james_w: and happy belated birthday.
<james_w> thanks :-)
<james_w> http://en.wikipedia.org/wiki/Salt_(cryptography)
<james_w> it does say "For best security, the salt value is kept secret.", but if your system relies on the salt being secret, then the keys are probably too short anyway
<abentley> james_w: it seems like in our context, all the passwords are equivalent, so only one cyphertext is needed.  Therefore using distinct salts doesn't help?
<james_w> abentley, where can I look to see the use of this in the source?
<james_w> as I'm not sure what you mean by "all the passwords are equivalent"
<abentley> james_w: https://code.launchpad.net/~jelmer/launchpad/672476/+merge/40349
<james_w> EACCESS
<abentley> james_w: by "all the passwords are equivalent", I mean they all grant equivalent access AIUI
<james_w> ah
<james_w> so impersonation isn't really a problem
<james_w> except for users who aren't supposed to have access at all
<abentley> james_w: jelmer is refactoring this code: lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py
<james_w> abentley, yeah, I'd agree, in this case salts aren't particularly useful for their normal use like in unix passwd
<james_w> it adds a little benefit in that you wouldn't have to revoke every access if one token was compromised, but you may want to do that anyway to be sure
<james_w> in this case though the main requirement is that crypt requires a salt, and so I think that the value used is as good as any other
<abentley> james_w: Right.  Hard to imagine only one token being compromised in this scenario.
<abentley> james_w: okay.
<abentley> jelmer: should _setupHtaccess consider the case where htaccess is different from what it would write?
<jelmer> abentley: No, it sufficient that there is *some* htaccess there.
<jelmer> abentley: generate_ppa_htaccess should take care of updating it
<abentley> jelmer: ISTM that until generate_ppa_htaccess runs, the contents could be exposed.
<abentley> jelmer: because the .htaccess might not impose any content restrictions.
<jelmer> abentley: We only write out one kind of htaccess file at the moment
<jelmer> abentley: I guess it might make sense take into account that we might want something different in that file in the future.
<abentley> jelmer: so _setupHtaccess assumes that htaccess will not be different from what it would write.
<jelmer> abentley: Sorry, yes. It assumes the .htaccess file would be the same and that there is some .htpasswd
<abentley> jelmer: If an earlier process had died causing a partial write, that would not be true, but I think that's rare enough not to care about.
<abentley> jelmer: I guess I'm wondering whether skipping writing htaccess and htpasswd is worthwhile.
<jelmer> abentley: That's a good question, I've wondered about that as well.
<abentley> jelmer: it seems like it doesn't save an appreciable amount of IO, and it does introduce a corner case.
<jelmer> abentley: It saves us a single database query and a certain amount of IO.
<jelmer> abentley: I don't know what the maximum number of subscriptions for a PPA is.
<abentley> jelmer: At 243, I believe temp_filename is supposed to be on a new line.
<abentley> jelmer: similarly at 351.
<jelmer> abentley: Fixed
<abentley> jelmer: Your tests would be a bit simpler if you returned the contents of .htaccess and .htpasswd as strings or if your functions took a file parameter.
<abentley> jelmer: You could also just use useTempDir in your tests.
<abentley> jelmer: You can use dedent to write your multi-line strings more neatly at 451.
<jelmer> abentley: I haven't used dedent before, what is it?
<jelmer> abentley: is this textwrap.dedent?
<abentley> jelmer: Yes.  It strips leading indents.
<abentley> jelmer: Anyhow, those are just some ideas.  r=me.
<jelmer> abentley: Thanks, that was quite educational.
<abentley> jelmer: np.
<jelmer> abentley: I just realized that if we always write out the htaccess file we need to honor the blacklist that currently exists in generate-ppa-htaccess.py
<abentley> jelmer: I see.  It's your call.
#launchpad-reviews 2010-11-09
<stub> lifeless: Does https://code.launchpad.net/~stub/launchpad/update-storm/+merge/40264 need to go through review?
<stub> 2.4k of pretty much mechanical changes and delintings
<lifeless> stub: hi
<lifeless> uhm, I wouldn't, but I would get the *approach* discussed
<lifeless> stub: e.g. the cast in the core is a little concerning in general
<stub> lifeless: Didn't really seem to be any other sane way of doing it. I could have fixed the tests feeding raw strings in, but that could create too many brush fires on production with all our untested code paths (or even fixing some of our tests - gina.txt is utterly inscrutable).
<StevenK> gina.txt is a horrid test
<lifeless> sure
<lifeless> I guess the main thing to do is to behave sanely if a non-ascii str is passed in
* bac changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: allenap, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> as well as have some way to let us incrementally report and fix things
* bac changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> the former should be done before landing for safety IMO
<lifeless> the latter whenever
<stub> lifeless: Where I have a unicode() cast, I could change that to something else that raises a warning, but I suspect that would be YAGNI.
<lifeless> I suggest this:
<lifeless> if type(thing) is not unicode:
<lifeless>     try:
<lifeless>         thing = thing.decode('utf8')
<lifeless>     except UnicodeDecodeError:
<lifeless>         bail
<lifeless> shove that in a helper
<stub> Oh - nothing should be getting UTF8 that I changed. We do want it to explode if we get non-ascii.
<lifeless> just doing a plain cast to unicode is a problem because it can do nuts things
<lifeless> stub: sure, s/utf8/ascii/ there
 * stub wonders if there is an easy way to s/unicode/ensure_unicode/g in the lines changed in my branch
<lifeless> grep + interdiff might be able to do something
<stub> interdiff does the magic
* henninge 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
* 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
<henninge> bigjools, jtv: if either of you would be so kind (maybe a revie is optional here ... ;)
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/devel-difftacular/+merge/40419
<bigjools> henninge: sounds like a perfect no-review branch to me
<bigjools> (without looking that is)
<henninge> what is the tag
<henninge> ?
<henninge> [rs=henninge] ?
<bigjools> no idea, I'm waaaay behind on email
<henninge> ;)
<jtv> Something like r=noreview, but probably not that
<bigjools> rs=me ?
<bigjools> do the tools work with this brave new world?
<jtv> henninge: too late, I've already approved it
<henninge> w00t!
<henninge> jtv: thanks
<StevenK> gmb: Hai! If you're still reviewing, I have a small branch ready for you
<gmb> StevenK: Sure. Link me
<StevenK> gmb: https://code.edge.launchpad.net/~stevenk/launchpad/move-ppaexception/+merge/40408
 * gmb looks
<gmb> StevenK: 81 lines. My kinda branch.
<StevenK> Haha
<gmb> StevenK: r=me
* 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> StevenK, hi
<mars> morning gmb
<gmb> Hi mars
<StevenK> mars: Hi
<mars> gmb, have you started into the activereviews pile?
* mars changed the topic of #launchpad-reviews to:  On call: gmb, mars || Reviewing: -,jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> mars: No, I haven't yet.
<gmb> I will be doing shortly, though.
<gmb> mars: Feel free to make a start and I'll pick up whatever you're not working on.
<mars> gmb, ok, I'll update the topic as I go
<gmb> Cool
<gmb> mars: Also, using "Claim review" takes it out of the list for the review team.
<mars> gmb, ah!  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
<jtv> gmb: care for a small one?  The funny bits are in the larger follow-up branch though.  https://code.launchpad.net/~jtv/launchpad/recife-pre-cleanups/+merge/40434
<gmb> jtv: Sure.
<jtv> thx
<gmb> jtv: r=me
<jtv> gmb: thanks
 * gmb goes off call for to do some hackering
* 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
<jtv> mars: can you take my big one?
<jtv> After some wild swings, it came out at a neat 799 lines.
<jtv> https://code.launchpad.net/~jtv/launchpad/recife-policy-invites-allows/+merge/40438
<mars> jtv, I can, I'm helping deryck with something though, so it may be a bit before I can start it
<mars> first on the queue though
<jtv> mars: great, thanks.  I'll wander in and out for the coming few hours.
* jtv changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> (as agreed)
* benji changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [jtv, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> mars: when you get a chance, here's a small MP for your perusal: https://code.launchpad.net/~benji/launchpad/bug-621090/+merge/40474
<mars> thanks benji
<salgado> sinzui, I haven't been getting any UI reviews lately. is it just me or are devs not working on UI lately?
<sinzui> I have had only one request in 3 weeks, and I could not do it
<sinzui> salgado, I think lose of edge is causing us to rethink ui.
<salgado> sinzui, why's that?
<sinzui> I think people are learning feature flags.
<sinzui> I think weeks 4 and 1 were busiest for UI because we need to get UI changes on edge.
<sinzui> Since UDS was on a week 1, we may have had a natural decline in UI requests
<salgado> yeah, that's possible
<sinzui> salgado,  my team is at the end of a feature. so we have less UI work, but expect a lot in a few weeks
<sinzui> the code team will get a lot in a few weeks too because they will hack on blueprints
<salgado> sinzui, cool, I'm looking forward to some UI reviews.
<sinzui> You will get lots. henninge, you, and myself are the UI reviewers
<jtv> mars: hope you'll be able to get to my branch, because the next OCR shift is probably yours truly!
<mars> jtv, :)
<mars> jtv, I was just looking at it
<mars> jtv, isn't it three in the morning for you right now?
<jtv> mars: just about :)
* mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* 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
<thumper> https://code.launchpad.net/~thumper/launchpad/recipe-builds/+merge/40489 anyone?
#launchpad-reviews 2010-11-10
<thumper> lifeless: thanks
<lifeless> hmm?
<lifeless> oh, de nada
<lifeless> stub: hi
<stub> yo
<lifeless> https://code.launchpad.net/~lifeless/launchpad/soyuz/+merge/40499
<lifeless> I fucked ppa/+packages
<lifeless> this should fix it
<jtv> such a marvelous language, Strineâ¦
<StevenK> lifeless: You removed a XXX, did you address it?
<lifeless> StevenK: look up the bug :)
<StevenK> lifeless: Which is Invalid, and filed against lazr.delegates ...
<lifeless> right
<StevenK> lifeless: Commented.
<lifeless> stub: are you up to do a review on this?
<stub> yo
<stub> Ahh... steve beat me so I don't have to pretend to understand soyuz
<lifeless> stub: StevenK isn't a full reviewer yet
<stub> Well he did a better job than I would have done ;)
<lifeless> stub: so rs it :)
<stub> Yer. Looks fine. My eyes glazed over a little during the tests but she'll be right.
<stub> Need more coffee
<lifeless> thanks - can you clicky clicky on approved then ?
<lifeless> [as a review I mean]
<allenap> gmb: Would you have 5 minutes to take a look at https://code.launchpad.net/~allenap/launchpad/trac-fixreleased-bug-667340/+merge/40507? It's a very simple externalbugtracker branch.
<gmb> allenap: Sure
<gmb> allenap: You need to update the commit message (the remote status is 'fixverified' rather than 'fixreleased'). r=me on the code, though.
<allenap> gmb: Good spot, and thank you :)
* 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
<gmb> jelmer_: Hi, I've got a branch for you if you'd be so kind: https://code.launchpad.net/~gmb/launchpad/add-bnl-to-structsubs-bug-672507/+merge/40522
 * jelmer_ is in a kind mood today
<gmb> :)
* jelmer_ changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: gmb  || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer_> gmb: hi again
<bigjools> jml: https://code.launchpad.net/~julian-edwards/launchpad/buildd-manager-fixes
<bigjools> when the email hits the MP will get created
<gmb> jelmer_: Hi
<bigjools> jml: ok MP got created, it's all yours.  I need to eat before I faint now.
<jelmer_> gmb: r=me, I just have one comment. It seems like it would be useful to have some infrastructure for running tests against all things that support structural subscription. I reviewed another branch a while back that also did it manually using a seperate TestCase subclass for each.
<gmb> jelmer_: I agree with you wholeheartedly, but I think fixing that here is out of scope. I'll be happy to file a bug to follow up at a later date, though (we're doing lots of work around structural subs, so it should get dealt with rather than just being ignored).
<jml> bigjools: done
<jml> bigjools: although, irritatingly this makes more work in my testtools-experiment branch.
<jelmer_> gmb: It'd be great if you could file a tech-debt bug about it. Anyway, just a thought.
<bigjools> jml: thanks - and why?
<jml> (that's what I deserve for a large branch unlanded over a course of weeks)
<gmb> jelmer_: Agreed. Will do.
<bigjools> jml: oh I know nothing about large unlanded branches :)
<jml> bigjools: it changes all of the Trial tests to use testtools. You're changing the test class structure, so I have to merge in & fix
<bigjools> jml: ah right - yeah I had to do that because the old structure was running tests twice
<jml> bigjools: yeah, it's the right thing to do.
<jml> I just wish I had more time to hack, and that easy things were easy
<jml> and that there was peace on earth, and ponies for all
<bigjools> you didn't spot the obvious fsckup tho :)
<jml> bigjools: oh?
<bigjools> super(TestBuilderWithTrial, self)
<bigjools> wrong class name now
<bigjools> my bad for not running the whole test file before submitting
* 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
<jml> bigjools: ahh, right.
<jml> bigjools: well, no point humans spending energy doing things machines can do :)
<bigjools> :-|
<bigjools> I wonder what else this change broke - lunch comes at a good time to find out :)
<abentley> jelmer: could you please review https://code.launchpad.net/~abentley/launchpad/select-owner/+merge/40556 ?
<abentley> sinzui: Would you mind doing a UI review of https://code.launchpad.net/~abentley/launchpad/select-owner ?
 * sinzui looks
<sinzui> salgado, do you have time to do a UI review of https://code.launchpad.net/~abentley/launchpad/select-owner/+merge/40556 ?
<salgado> sinzui, sure
<salgado> abentley, can you get a screenshot of a page which shows the UI changes you've done?
<abentley> salgado: already did, it's linked in the merge proposal.
<salgado> abentley, really?  I don't see it there
 * salgado refreshes the page
<salgado> See: https://code.launchpad.net/~abentley/launchpad/select-owner
<abentley> salgado: my bad, pasted the wrong link.
<salgado> abentley, was that supposed to be it?
<abentley> salgado: http://people.canonical.com/~abentley/suggest-owner.png
#launchpad-reviews 2010-11-11
* 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
* 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> soyuz: Could you review https://code.launchpad.net/~lamont/launchpad/sigill/+merge/40569? It's very short, but I think it could do with your seasoned eyes :)
<bigjools> looking
<bigjools> approved
<allenap> bigjools: Cheers.
<bigjools> allenap: I can explain what's going on if you want
<danilos> allenap, in an effort to stop you from being bored on your OCR day, I've prepared a branch just for you: https://code.launchpad.net/~danilo/launchpad/bug-662552-fast-pofile-selection/+merge/40615
<danilos> allenap, I hope you can find some place in your heart for this branch :)
* danilos changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [danilo] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<danilos> allenap, it's measly 89 lines of diff, there must be room for that :)
<allenap> danilos: There certainly is. But first lunch.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: lunch || queue: [danilo] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> bigjools: Thank you for the kind offer, but the pub beckons.
<jml> phwoar
<jml> pub lunch
 * jml wants one
<bigjools> what an awesome idea
<bigjools> damn you guys for seeding that idea in my head
<jml> no pub lunch for me.
<jml> eating Tuesday's dinner instead.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: danilo || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> Hi allenap
<jelmer> allenap: Are you up for some more Soyuz reviewing?
<allenap> jelmer: Oh, yes please :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jelmer || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> :-)
<jelmer> allenap: The MP is https://code.launchpad.net/~jelmer/launchpad/cronjob/+merge/40629
<gmb> allenap: I've got another one for your queue: https://code.launchpad.net/~gmb/launchpad/fix-advanced-bug-sub-ui-bug-673288/+merge/40631
* gmb changed the topic of #launchpad-reviews to:  On call: allenap || Reviewing: jelmer || queue: [gmb] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> gmb: Tip-top.
<allenap> gmb: The merge proposal is ejecting toys: Text conflict in lib/lp/bugs/browser/tests/test_bugsubscription_views.py
<gmb> allenap: The mp says it has conflicts. This is a lie.
<gmb> I'll re-merge devel, but I've already fixed those.
<gmb> allenap: Hang on; let me re-merge and get you a clean diff.]
<gmb> allenap: Oh, bottom. I forgot to push the conflict fixage. Doing so now.
 * gmb doesn't know how the voodoo with updating MP diffs works.
<gmb> allenap: I've pasted a diff into a comment on the MP.
<allenap> gmb: The diff should be regenerated once the branch scanner has done its thing, I believe.
<gmb> allenap: Yep, it's cleared up now.
<allenap> gmb: But there's no indication of branch scanning in merge proposals, only on branch pages.
<gmb> Yeah. That's an annoyance.
<jelmer> allenap: Thanks for the review.
<jelmer> allenap: I think underscores are much fashionable than dashes, but I'll bow to the masses.
<jelmer> *much more
<abentley> salgado: who is your mentor?
<salgado> abentley, sinzui
<abentley> sinzui: Could you please mentor salgado's review of https://code.edge.launchpad.net/~abentley/launchpad/select-owner/+merge/40556 ?
 * sinzui looks
<abentley> allenap: could you please review https://code.edge.launchpad.net/~abentley/launchpad/select-owner/+merge/40556
<sinzui> abentley, is the menu subordinate to (*) Other?
<abentley> sinzui: the drop-down is the "other" option.
<sinzui> I wonder if we can show it aligned with the option then
 * sinzui looks for css class
<sinzui> abentley, I think other_selection_widget should have the "subordinate" css class like the radio buttons. I think we need to pass cssClass='subordinate', but I do not see exactly where to do it in the diff
<abentley> sinzui: does this need to be passed to the widget constructor, or can we just set it later?  What is its effect?
<abentley> sinzui: should it also be applied to the target branch selector?
<allenap> jelmer: Hehe :)
<sinzui> I see it in the constructor. I think I have set it afterwards recently. the subordinate class will indent the menu to align it with the radio button that controls it
<allenap> abentley: Sure.
<sinzui> abentley, I will only ask you to do this if I can remember how I did it
* allenap changed the topic of #launchpad-reviews to:  On call: allenap || Reviewing: abentley || queue: [] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> allenap, thanks.
<sinzui> abentley, I think cssClass is only used on render. I think we can set it after __init__.
<sinzui> so after line 49: self.other_selection_widget = getMultiAdapter(
<sinzui> we add
<sinzui> self.other_selection_widget.cssClass = 'subordinate'
<abentley> sinzui: okay, I'll give it a shot.
<abentley> sinzui: doing it there screws up the target branch widget, but it looks like this: http://people.canonical.com/~abentley/suggest-owner-2.png
<sinzui> :(
<abentley> the target branch widget has the subordinate widget on the same line, so the extra indenting doesn't make sense.
<sinzui> abentley, I think then this is a bigger css form issue. I now recall I had to write a template to solve that.
<sinzui> abentley, r=me for your original effort
<abentley> sinzui: I can do it for the RecipeOwnerWidget only, by providing a constructor.
<abentley> sinzui: This patch doesn't break TargetBranchWidget: http://pastebin.ubuntu.com/530114/
* 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
<abentley> allenap: what are the line-wrapping conventions for list comprehensions?
<allenap> abentley: Same as for braces?
<abentley> sinzui: I've discovered that the items in the dropdown get indented even further, so I'll revert that change.
<sinzui> abentley, thanks for trying. I think we want create some contextual CSS rule that solves this layout for us. Your branch is good to land
<abentley> sinzui: Cool.
<wallyworld> thumper: oops fix. pretty please: https://code.launchpad.net/~wallyworld/launchpad/link-checker-oops/+merge/40680
#launchpad-reviews 2010-11-12
<thumper> wallyworld: looking
<wallyworld> ta
<thumper> wallyworld: there is no diff
<wallyworld> ? i pushed the branch
<wallyworld> thumper: https://code.launchpad.net/~wallyworld/launchpad/link-checker-oops
<thumper> wallyworld: yeah, I can see that
<wallyworld> thumper: i should have looked more carefully at the console when i bzr pushed
<wallyworld> thumper: there was a zope error xmlrpclib.Fault: <Fault -1: 'Unexpected Zope exception: AssertionError: '>
<wallyworld> what's the best way to fix it - repush or delete and start again?
 * thumper tries something
<thumper> wallyworld: here is where we need the regenerate diff button
<wallyworld> yep :-)
<wallyworld> so are you going to invoke the scan job again manually?
<thumper> wallyworld: done
<wallyworld> thanks
* 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
<gmb> allenap: Thanks for your review; I've replied.
<allenap> gmb: I'll go and take a look now.
<gmb> Ta
* sinzui changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [sinzui] ||  This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> adeuring, I have a branch that fixes 4 trivial bugs
<sinzui> adeuring, do you have time to review https://code.launchpad.net/~sinzui/launchpad/headings-and-words-0/+merge/40677
<adeuring> sinzui: I'll look
<sinzui> thanks
<adeuring> sinzui: approved
<sinzui> thank you very much
* 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
<danilos> adeuring, hey, still around? I've got a branch up for review but it also needs a DB review that I am probably not getting before Monday, so it's not urgent, but I don't mind getting a code review for now :)
<adeuring> danilos: sure, I'll look
<danilos> adeuring, https://code.launchpad.net/~danilo/launchpad/drop-variants/+merge/40735
<danilos> adeuring, thanks
<danilos> adeuring, it should be very simple, mostly removals
<adeuring> danilos: ok, that sounds good ;)
<adeuring> danilos: r=me
<danilos> adeuring, thank you very much
<danilos> adeuring, thanks for grepping yourself, I did miss a case ;)
<adeuring> danilos: well that's what reviews are for, aren't they ;)?
<danilos> adeuring, totally :)
* adeuring 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> EdwinGrubbs, about your branch, the comment is included in rendered markup. It looks like the table will be show to everyone. Is that not so? does "view/configuration_links" always do the right thing; it was the progress bar table that was the problem?
<EdwinGrubbs> sinzui: good point. I need to add a method in the view to loop through view/configuration_links to see if any of the links are enabled, so that the table can be removed.
<sinzui> EdwinGrubbs, brian added the permission launchpad.BugSupervisor maybe we only need to change a launchpad.Edit
<EdwinGrubbs> sinzui: sure, I was making it re-usable in case there was another group of people that only got to see some subsection of the links, but there will probably never be anyone else besides the BugSupervisor.
<EdwinGrubbs> sinzui: I pushed up the suggested changes.
<sinzui> EdwinGrubbs, can you look at https://code.edge.launchpad.net/~sinzui/launchpad/ds-getcurrentreleases/+merge/40756
<sinzui> EdwinGrubbs, r=me
<EdwinGrubbs> thanks
<EdwinGrubbs> looking at your branch now
<EdwinGrubbs> sinzui: r=me
<sinzui> thanks edwin.
<henninge> So, how is everybody?
<henninge> Anybody in need of r-c approval before PQM closes? ;-)
<lifeless> https://code.edge.launchpad.net/~sinzui/launchpad/ds-getcurrentreleases/+merge/40756
<lifeless> hmm, thats an edfe url
<lifeless> sinzui: how come you're still using edge? :)
<lifeless> henninge: ^
<sinzui> lifeless, I seem to have some bookmarks. I am not every trying to use edge
<sinzui> It might be more email since I purged browser history and update bookmarks
