[00:03] mwhudson: done [00:03] rockstar: I'm back if you are ready to talk [00:04] thumper: danke [00:05] thumper, I think this diff is good, just a few comments. [00:06] rockstar: skype me up [00:07] thumper, hold on, Mo just got home. [00:23] rockstar: https://code.edge.launchpad.net/~thumper/launchpad/use-last-rev-id/+merge/20023 [00:28] thumper: ta [00:28] oh, i already said that [00:52] :) [01:24] thumper: just threw a very simple branch at you [01:24] ok === jamalta-afk is now known as jamalta [01:32] mwhudson: done [01:33] thumper: ta [01:33] thumper: really? [01:33] mwhudson: via email [01:33] mwhudson: patience [01:33] thumper: ok [01:33] :-) [04:31] thumper: hey, one more review if you're around: https://code.edge.launchpad.net/~mwhudson/launchpad/pull-mirror-branches-separately-bug-520107/+merge/20032 [04:36] thumper: hm, initial email is probably wrong, hadn't pushed some revs === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [adiroiban(bug-509252-take-2), sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [09:31] jtv: still reviewing? [09:31] bigjools: yes, chuck it on the queue. otp. === bigjools changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [adiroiban(bug-509252-take-2), sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [09:32] ta [09:38] Hi adeuring, I do realise you were thinking about accessibility when you made the decision, I'm just not sure I understand what benefit an img+alt has over a span+title+content? Is it specifically that the alt will be read, but the title attribute won't? [09:39] noodles775: yes; screnreader should present images via their alt tag content [09:39] s/screnreader/screenreaders/ [09:40] adeuring: yes, but not the title? I always *assumed* (perhaps wrongly), that titles were read by default too. Hrm. [09:41] noodles775: I am also not 100% sure, but I think it is good to follow standards ;) [09:42] noodles775: also, the content and purpose of the title and alt attribute are, while similar, different in theier detailed meaning [09:43] adeuring: yes, theoretically, sure, but in practise I've usually seen people define both for images, and title's only for other elements (as they're just wanting the tool-tip). [09:45] noodles775: right, but "tooltip" is a good summary for the spec of the title attribute I quote in the MP. But that's different from a textual representation of an image [09:45] i.e, from the alt atttribute [09:45] yep, it's not an accessibility feature. [09:50] adeuring: so the way I was thinking about the issue, the "3 out of 4 heat flames" would be the textual content (displayed by default when no styles are used etc.), but switched to a graphical representation if stylesheets are enabled. Yes, it's not perfect (ie. content hidden by stylesheet), but it would have a title and content, and allow us to use our sprites infrastructure. [09:51] But I'll butt-out of the code discussion and leave that to bac/deryck and you to sort out. [09:52] noodles775: that works fine for browsers like Lynx. but many blind people use grapical software together with screenreaders. In this case your approach does not work: A screenreader should present only the visible elements of an htML page [09:53] adeuring: yes, I've used screenreaders before (a few years ago), and it was always just a setting, but I can't remember what the default was. [09:53] (eg. JAWS) === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: sinzui || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [10:54] bigjools: conflict in your branch [10:55] bah [10:55] blame noodles775 [10:55] one sec [10:55] it's yesterday's testfix branch [10:55] jtv: seems ok, I just merged with none [10:55] hmm [10:56] just pushing up a new rev [10:56] see if it fixes [10:56] bigjools: moving right along... is the "copy" in --copy-archive as a verb? [10:56] branch scanner may be having a spaz [10:57] noun [10:57] as in ArchivePurpose.COPY [10:57] that's what I figured... makes the option name a bit misleading [10:59] bigjools: is there any particular reason why process-accepted.py explicitly chooses read-committed isolation rather than default to it? [10:59] I believe serializable used to be the default, so maybe this is just historic. [11:00] jtv: are you looking at the right branch? [11:00] the p-a change is already reviewed [11:00] https://code.edge.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-publish-distro/+merge/19987 [11:00] is the one I am waiting on :) [11:00] bigjools: https://code.edge.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-publish-distro/+merge/19987 is the one I'm looking at. [11:01] This bit is all the way at the end. [11:01] * bigjools looks again [11:01] errr that diff is total crap [11:01] no it isn't... I need to search my desktops for a second review now [11:02] wtf - it has translations stuff in it [11:02] Yup... did you branch off devel and then propose for merging into db-devel? [11:02] nope [11:02] This MP is for db-devel. Doesn't sound like that's what you wanted for this change... [11:03] GAH [11:03] let me re-submit it [11:03] I'll take that as a "yes." [11:04] https://code.edge.launchpad.net/~julian-edwards/launchpad/publish-copy-archives-bug-520520-publish-distro/+merge/20049 [11:07] bigjools: now all of a sudden the "this is a simple branch" makes a lot more sense. [11:07] no kidding [11:07] I suck [11:09] bigjools: I'll refrain from commenting on that. There is, however another small matter. [11:09] haha [11:09] * jtv breathes sigh in relief that his joke wasn't taken seriously [11:10] 42 + if archives.count() == 0: [11:10] 43 + raise LaunchpadScriptFailure("Could not find any COPY archives") [11:10] Use "if archives.is_empty()" instead. [11:10] can't [11:10] it's sqlobj [11:10] grrr [11:10] otherwise I'd use bool() [11:11] but that's got a bug in it since FOREVAH [11:11] this inconsistency between our Storm and SQLObj objects is really annoying now [11:11] Stormicate getArchivesForDistribution? [11:11] yeah that'd be nice [11:12] but out of scope for this branch [11:12] I need to get this shit working by Friday [11:12] Okay, then the idiomatic way of doing this nowadays is to fetch one arbitrary row and check if you get None. [11:13] The implementation there is still crap in that it still sorts the query, but that's not your fault and may go away at some point. [11:18] bigjools: then there's this in the test docstring: "It should only publish copy archives." Come on, a bit more confidence! [11:18] Something like "In this mode, it only publishes copy archives." [11:18] it's a unit test, not a doc test [11:18] I am telling you what it's testing [11:18] not what it will do [11:18] Good answer :) [11:19] :) [11:19] I see now that the option name is much clearer in context. So I withdraw my objection to that. [11:20] good :)_ [11:20] brb in 5, I depserately need more coffee [11:20] you and me both, bro === jtv is now known as jtv-afk [11:25] ah Java [11:47] bigjools: did you find a more satisfactory solution for checking for an empty result? [11:48] nope [11:48] it's not the first time this came up === jtv-afk is now known as jtv [11:50] hmm... there is a __nonzero__ on the sqlobject compatibility wrapper [11:50] yes, it's fucked [11:50] oh :) [11:50] there's a bug about it [11:51] get a nice traceback when using it [11:51] Then can I trouble you to make your comment a XXX, referring to either that bug or a new one for stormicating the query? [11:52] bug 246200 [11:52] Bug #246200: SQLObjectResultSet.__nonzero__() implementation does not strip result ordering.

[11:52] sure :) [11:52] then with that, r=me [11:53] grassy ass [11:53] I just realised I need to tweak that script some more but I'll do a separate branch [11:55] de nutter === jtv is now known as jtv-eat [13:14] who wants to review a small and boring branch? === intellectronica changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [intellectronica] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [13:15] jtv-eat: if you're still reviewing when you're back maybe you can have a look? mp is https://code.edge.launchpad.net/~intellectronica/launchpad/use-max-heat/+merge/20054 [14:36] hello jtv-eat, you must be suffering withdrawal symptoms since you haven't reviewed Soyuz branches for a long time ;) [14:36] Here's one to make your day: https://code.edge.launchpad.net/~al-maisan/launchpad/oops-526969/+merge/20056 === al-maisan_ changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [intellectronica, al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === al-maisan_ is now known as al-maisan [14:47] intellectronica, i'm going to need yet another review of that launchpad integration branch [14:48] leonardr: hey, how about we trade reviews? [14:48] intellectronica, sure [14:48] excellent [14:49] leonardr: here's mine: https://code.edge.launchpad.net/~intellectronica/launchpad/use-max-heat/+merge/20054 [14:51] leonardr: can you maybe prepare an inc. diff for your changes? [14:52] intellectronica, i'm working on it [14:52] i need a few minutes to write a new comment [14:52] intellectronica: maybe you should remove yourself from the queue then? === intellectronica changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bigjools || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [14:55] al-maisan: good point [14:55] leonardr: cool === danilo__ is now known as danilos [15:10] intellectronica: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346 [15:19] intellectronica: it seems like having a MAX_HEAT in bugtask.py would still be useful. you still use it in two different files [15:20] leonardr: r=me [15:20] intellectronica: apart from that, r=me [15:20] leonardr: i use it in a test. i use the value as a default, but it will be gone with a following branch which will make sure we have max_heat populated [15:21] intellectronica: oh, one other question. why can't the bugheatview be a view on the bugtask? [15:21] (assuming views are desirable in general) [15:21] leonardr: it's just a case of yagni. it wasn't such a brilliant idea to use a view to begin with. [15:22] all we ever did with the view is initialize manually and call it [15:22] ok. i don't like views so i approve [15:23] :) === al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jtv-eat is now known as jtv === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === salgado is now known as salgado-lunch [15:56] al-maisan: your branch looks good, but one question: your tests do things to job.status to show how it affects dispatch_time_estimate_available, but it'd be nice to see buildstate affect it as well... is that in there somewhere? If not, is it doable? === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: al-maisan || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:58] jtv: I can add a test that manipulates the buildstate as you suggested above [15:59] al-maisan: something simple would do. I'm mostly worried about protecting my fascist reputation. ;-) [15:59] :-) [16:02] jtv: actually there are quite a few pre-existing tests involving builds in states other than NEEDSBUILD in xx-build-record.txt [16:03] and they ascertain that no dispatch estimate is requested and/or shown on the +build page [16:03] al-maisan: then that answers my first question. You're good to go. [16:04] (BTW the code is not just functionally better this way but the TAL is cleaner, too. Me like.) [16:04] jtv: man, I love these "soft" fascists who even listen to what other people say ;) [16:04] jtv: thanks :) [16:05] al-maisan: careful now... there _will_ be a next review and you want me in a good mood. :-) [16:05] oh-oh [16:05] sorry massa [16:05] ;) [16:06] al-maisan: btw if you request a "code" review (I set the type myself in this case) and set a commit message, you can combine the headless testing and the landing with a mere "./utilities/ec2 land " [16:07] jtv: great! [16:07] will try that [16:08] No need to spell out "[r=myfavoritereviewer][ui=thisisnotwindows][bug=123456][listening-to=Pet Shop Boys Best Slayer Covers] This branch fixes a bug." [16:13] jtv: very nice indeed. === matsubara is now known as matsubara-lunch [16:59] could somebody review my branch for bug 527174? [16:59] Bug #527174: logo_link missing from "team" object in API [17:16] al-maisan: ? [17:17] bdmurray: I am sorry but I am just about to finish the day. [17:17] jtv: are you still around? [17:17] al-maisan: yes [17:17] got another one? [17:17] bdmurray: has one ^^ [17:18] Good night folks! === matsubara-lunch is now known as matsubara [17:23] bdmurray: I'll get to it in a moment [17:24] bdmurray: can you put yourself on the queue? [17:24] jtv: okay, thanks [17:24] jtv: queue? === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [bdmurray] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:25] ah! that's not too complicated [17:35] bdmurray: wehre's your merge proposal? [17:36] jtv: apparently I missed that step - https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-person-logo_link/+merge/20074 [17:37] bdmurray: there are no tests! [17:37] on call: jtv || reviewing: bdmurray || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: bdmurray || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:38] oh, I see it. [17:40] bdmurray: looks simple enough... can't think of anything that might go wrong, but who else did you discuss the change with? [17:40] (Sorry to be a stickler, but I'm suspicious of things that can't possibly go wrong :-) [17:41] jtv: I haven't discussed it with anyone. Should I do that with someone on the registry team? Additionally, are the steps to contribute to Launchpad detailed anywhere? [17:42] bdmurray: they should be somewhere on dev.launchpad.net. We normally require a pre-implementation call where you discuss your approach with another developer as a basic sanity check. [17:43] You know, someone who might say "you can't do that! Someone is relying on this method 1,000× per second and including an image makes it too slow!" [17:43] bdmurray: ah, found it... https://dev.launchpad.net/PatchSubmission [17:44] (This is tailored to outside contributors, of course) [17:49] Normally we also write cover letters that explain the change to the reviewer (and future historians), say which tests to run, and what "make lint" had to say about the branch. [17:50] I didn't see that in the wiki page. This process seems really complicated to me. [17:50] It is. We have to be careful with the codebase or things can go really horribly wrong. But in this case tbh I can't think of any additional effort that might be useful. [17:51] Most changes aren't this easy, or they'd be riding along on the back of a larger branch. [17:53] bdmurray: I've approved it. [17:53] bdmurray: can you land it yourself or would you like me to do it for you? [17:54] jtv: okay, thanks. What happens next then? I'd like to run the ec2 test if possible - I was trying to set that up also. [17:54] bdmurray: several ways to do that. [17:54] When you've pushed all your changes, you can run "./utilities/ec2 test --headless" [17:55] right with -s "[r=jtv] ...?" [17:55] No, this is when you just want to run a test, before you've got approval to land. [17:56] It can be useful if you're expecting to wait a while before review, and you want to use the time to have EC2 figure out what tests you just broke. [17:57] Second, you can also test-and-land: "./utilities/ec2 test --headless -s '[r=jtv][ui=none][bug=1235] Frumbil the xeniogarb.' [17:57] That runs the tests and sends your branch off to pqm as soon as it's done. [17:58] But third, if you've met a few conditions, you can just "./utilities/ec2 land " [17:58] (Where --headless is implied) [17:58] This runs your tests, and if successful, composes the "[r=..." commit string and sends the branch to PQM for landing. [17:59] The conditions you need to meet are: [17:59] * You must have an Approve vote on the mp. [17:59] * The mp must be in Approved state. [18:00] * The Approve vote must have a review type of "code"—leaving it blank won't do, so try to request this specific review type if you remember to. [18:00] * The commit message on the MP must be set; write a short single-line description of the branch there, like I just did for you. [18:03] [18:06] jtv: okay, thanks for all that information [18:06] bdmurray: no worries... I think you have rights to land this; if not, let one of us know and we can do it for you. [18:09] jtv: okay, I've started the test and will let you / someone know [18:09] bdmurray: good luck, and thanks for the patch! [18:10] Doesn't always feel like people are grateful when you hit a process like this, I know. === jtv 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 [18:13] jtv: I just pushed my pottery-to-slave branch. I thought you might be interested ... ;-) [18:13] jtv: bug I am also about to EOD, so we can discuss it in the morning. [18:14] henninge: cool [18:14] jtv: pbuilder was really helpful when trying it out. [18:14] henninge: what work does it take out of your hands? [18:15] jtv: it builds a base hardy (or whatever you ask) chroot, keeps it nicely in a tarball. [18:16] oh, nice! [18:16] jtv: I just go pbuilder --login and get a shell in the chroot. [18:16] very nice [18:16] after exit, all is cleaned up. All changes forgotten (unless you ask for it not to). [18:19] henninge: anyway, my OCR day is done. [18:19] jtv: see you tomorrow! [18:19] bis morgen! [18:19] ;-) [18:20] * henninge has to learn these things in Dutch. === 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 === gary_poster is now known as gary-lunch === gary-lunch is now known as gary_poster [20:04] EdwinGrubbs, could you review this merge proposal, please? https://code.edge.launchpad.net/~abentley/launchpad/job-db-user/+merge/20081 === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: abentley || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [20:05] abentley: sure [20:05] EdwinGrubbs, thanks. [20:56] abentley: TwistedJobRunner.runFromSource() has an error_utility parameter that JobRunner.runFromSource() does not have, so I'm wondering why the dbuser parameter was added to JobRunner.runFromSource() despite that class ignoring that argument. [20:57] abentley: it seems like it would be good for JobRunner.runFromSource() to at least warn the user when they are passing in an argument that is ignored. [21:00] I added the dbuser parameter to allow the JobRunners to support polymorphism. Otherwise, we would have to switch how we called runFromSource according to the jobRunner class. [21:02] EdwinGrubbs, we always pass in the dbuser argument, so warning the user would result in endless warnings. === salgado is now known as salgado-afk [21:24] abentley: why wasn't the error_utility parameter added to JobRunner then? [21:24] EdwinGrubbs, it's not currently used. [21:25] abentley: but it seems like that would fall under the same polymorphism argument. All the JobRunner class' runFromSource() should have the same parameters. [21:26] EdwinGrubbs, I don't thing that it is true for optional parameters. [21:27] EdwinGrubbs, s/thing/think/ [21:28] abentley: I don't buy that. If someone is trying to call runFromSource() without checking whether the class supports the dbuser parameter, they could just as easily do that with the error_utility parameter. Otherwise, it will raise an exception when you pass in too many arguments. [21:31] EdwinGrubbs, the difference is that dbuser is a mandatory parameter. You need it for the TwistedJobRunner to work, which means you must supply it to all JobRunners. [21:31] EdwinGrubbs, however, I can remove the error_utility parameter. It just seems out of scope for the change I'm making. [21:34] abentley: If you just add error_utility=None to JobRunner, it will be optional and the parameter lists will match, so you won't have to check which subclass of JobRunner you have before calling removeFromSource(). [21:34] EdwinGrubbs, I don't have to check which subclass of JobRunner I have anyhow! [21:35] EdwinGrubbs, it's an unused parameter. === matsubara is now known as matsubara-afk [21:36] EdwinGrubbs, there are no callsites that supply that parameter, so there are no callsites that care about whether it's supported. [21:36] abentley: ok, that's fine too. I just didn't know if there were some callsites passing in an ignored error_utility, which would mean more work for you than a single line of code change. [21:36] abentley: you read my mind [21:37] EdwinGrubbs, I'm confused. What would you like me to do. [21:38] abentley: feel free to remove the error_utility. I just wanted the method parameters to match. [21:39] abentley: the only other change I would like is some info about the dbuser parameter in the docstring about whether it is ignored or not. [21:39] abentley: r=me [21:39] EdwinGrubbs, roger wilco. [21:48] EdwinGrubbs, changes pushed. [21:49] abentley: I don't need to see them. feel free to land it. [21:49] EdwinGrubbs, sure. Was just letting you know. [22:09] jtv, hi === jelmer__ is now known as jelmer === jelmer is now known as Guest75587 === Guest75587 is now known as ctrlsoft === EdwinGrubbs is now known as Edwin-brb