[02:07] mwhudson: probably not tested [02:10] thumper: do you think it should be? [02:10] mwhudson: yes [02:10] damn :) [02:11] a simple click should be enough :) === stub1 is now known as stub === jtv changed the topic of #launchpad-reviews to: on call: — || reviewing: — || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ [09:03] hello stub, did you have a chance to look at https://code.edge.launchpad.net/~al-maisan/launchpad/psds-399186/+merge/13959 ? [09:04] al-maisan: It looked fine. I'll do the real review now. [09:04] stub: thanks -- that's very much appreciated :) [09:19] al-maisan: There is mention in the notes about renaming the tables to DistributionPackageSet and DistroSeriesPackageSet - is that going ahead at some point? [09:20] (I'm quite happy with the current names myself) [09:20] stub: yes, but the renaming will occur later in a separate branch. [09:21] it is critical for this to go in and the renaming will increase the amount of work (particularly on the python side); that's why it's postponed. [09:21] al-maisan: Can the same packageset in different distroseries have different names? [09:22] stub: the idea is to have a package set for each distro series i.e. we won't share package sets across distro series === danilos is now known as danilo-brb [09:23] I'll rephrase that. Can two packagesets in the same packagesetgroup have different names? [09:23] I'm just confirming that the name column should remain on the packageset table rather than move to the packagesetgroup table. [09:23] stub: yes [09:24] we expect that package sets will get renamed over time [09:24] i.e. each package set should have its own 'name' column independent of any others in the same group [09:25] Yup. Just got to that in the meeting notes :) [09:25] :) === danilo-brb is now known as danilos [10:15] Hi jml - if we've got a database constraint (say for a unique name), do we need to test that constraint? [10:15] if so, what's the best way to do so... I'm having to do the following which looks ugly: [10:15] http://pastebin.ubuntu.com/302669/ [10:16] (because the unique constraint isn't actually checked during the transaction until a subsequent db query is run). [10:16] or stub ^^ [10:16] noodles775, from my pov, I'd want to know how the constraint itself is going to be used / avoided [10:17] database constraints are safety nets that don't need to be tested. The python code that protects these constraints from ever being triggered needs to be tested (such as your validators). [10:17] noodles775, in an ideal world, you'd get a DuplicatePackageSet error or something. [10:17] noodles775, which is kind of what stub is saying :) [10:20] Great - thanks guys. Of course - given that this will be used via the api we need to raise an exception. [10:28] good morning === bac changed the topic of #launchpad-reviews to: on call: bac€” || reviewing: -€” || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ [10:28] bring out your dead! [10:29] fresh, hot reviews! [10:29] jml, so next question. Previously I've raised an exception by first hitting the db to check for a dupe - but is there not a way to rely on the unique constraint itself (but without committing the transaction - ie. something that does what the ugly hack in the above paste does - just accesses the db)? [10:29] hi bac :) [10:29] hi noodles775 [10:30] noodles775, what's the ugly hack in the paste above? [10:30] jml, see http://pastebin.ubuntu.com/302669/ line 13+ [10:31] oh I see. [10:31] perhaps just reloading an object - but it's still not descriptive of what I'm actually trying to do (just trigger the unique constraint). [10:32] noodles775, In other cases, we look before we leap [10:32] Yeah, that's all I could find too... ok. [10:32] noodles775, I wish I could think of something better [10:33] Hmm... al-maisan just found store.flush() - looks perfect. [10:36] hi jtv [10:37] stub: could you please push the "approve" button if you are done (and satisfied) with the schema patch? [10:40] al-maisan: Getting distracted with production issues [10:41] stub: oh, I see. [10:41] al-maisan: So packagesets can have a different owner to their packagesetgroup and other packagesets in the same packagesetgroup? [10:43] stub: I guess this will be true .. different people/teams creating package sets over time [10:43] al-maisan: So it is registrant really? Or does the owner get special privileges? [10:43] for package set groups the intention was merely to capture who created them [10:43] stub: it's more of a registrant thing .. yes [10:44] no elevated privileges for the owner as far as I am aware [10:46] stub: sorry .. for package sets: the owner has "permission = 'launchpad.Edit'" [10:56] hi jtv [10:56] bac: hi [10:56] jtv: your in the queue. is that accurate? for your scale-message-sharing branch? [10:57] bac: seeing some question marks in the subject line... did you edit it in a non-utf-8-aware client? [10:57] bac: that's the one, yes. [10:57] er, 'you are in the queue' [10:57] jtv: really? [10:58] jtv: i updated this client yesterday, so perhaps it is hosed. [10:58] bac: yes, I put emdashes in it (because I _can_, dammit!) and the question marks look like they might be what remains after deleting the respective first bytes of their utf-8 representations. [10:59] jtv: why don't you fix and i'll see what it looks like here === jtv changed the topic of #launchpad-reviews to: on call: bac || reviewing: — || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ [10:59] jtv: originally i saw an a+hat [10:59] bac: are you getting an emdash? [11:00] wow, that's definitely an encoding mismatch., :) [11:00] no, a with a hat [11:01] jtv: why don't you reset it and perhaps my editor won't mess it up going forward [11:01] jtv: i'll start your review now [11:01] bac: I did reset it... how doese it look now? [11:01] oh, sorry, missed your latest [11:01] * jtv resets === jtv changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ [11:02] better [11:04] jtv: your help message at line 67 says "remove some duplicates". does it remove all duplicates? if not, who picks? [11:04] should 'some' be deleted from the description? [11:04] bac: oh, I could've sworn I documented that. [11:04] maybe you did [11:05] The "some" is shorthand for "a particular class that if found, would otherwise get in the way of merging." [11:05] The class is: duplicates that are attached to "representative" potmsgsets. [11:06] The messages from less-representative corresponding potmsgsets in other templates are then added to the representative potmsgset. [11:06] Since that addition also eliminates duplication, any duplicates there will vanish as a side effect. [11:07] jtv: i don't think that's very well conveyed if a person looks at the help message. if i saw "remove some duplicates" i'd be pretty worried about which ones. [11:07] al-maisan: review in [11:07] al-maisan: Still need jml's stamp [11:08] bac: I'll get on that right away [11:08] stub: thanks -- the next patch will be more readable -- I promise :) [11:09] jtv: i see there is no action for that parser option. doesn't it need a a store_true? is there a default action it is using now? [11:09] bac: heh, hadn't even noticed, but it did seem to work this way. I'll add the action. [11:10] al-maisan: old habits - I understand ;) [11:10] bac: well, I mean it _did_ work this way... but then again I didn't look very closely at what was stored in that option. [11:10] stub: :P [11:12] bac: ah... it defaults to "take an argument and store it." So "-D -vvv" gobbled up the -vvv! [11:13] jtv: so it assumes it is a string option. not what you want! [11:13] right [11:14] bac: and for the option documentation... how about just "Remove *problematic* duplicate TranslationMessage"? [11:14] that's better [11:15] jtv: so if you run now with no action specified on the command line do you get the error message at line 95? [11:16] bac: "Select at least one action:" etc. [11:18] jtv: at 141 was the txn.begin() not needed? if not, why can you remove it now? [11:19] bac: it was never actually _needed_, and I had extensive discussions with stub about whether it was nicer to write it. [11:19] jtv ok [11:19] bac: if I write it, the new transaction (probably) starts right away. If not, creation of a new transaction (probably) gets deferred which may reduce transaction lengths a bit. === matsubara-afk is now known as matsubara [11:23] jtv: the assignment at 158 is not needed [11:26] bac: you're right, I think I should go back and extend my testing a bit. [11:26] jtv: how do you test for a superfluous assignment? [11:26] jtv: or did you intend to do something with it? [11:27] bac: this one would basically lead to the wrong superfluous message being deleted... hang on, I still have to figure out whether that has any consequences at all [11:28] jtv: really? 'representative' is just reassigned when the loop starts [11:28] bac: ahhh, I mis-read. You're completely right. [11:32] jtv: the comment at line 291 needs to be cleaned up. there may just be an extra 'the' in it but it makes little sense now. [11:33] bac: yup [11:33] jtv: r=bac with those changes [11:33] bac: just made. Thanks! === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ [11:34] jtv: unfortunately, i've written my comments referring to lines in the diff. when you fix the branch and repush it, the diff will be regenerated and my comments will no longer make any sense, rendering this review useless for historical purposes. :( [11:36] bac: oh well, it'll still be my mistake if something went wrong there. :) [12:09] bac: can you review https://code.edge.launchpad.net/~leonardr/launchpadlib/wsgi-fake-launchpad/+merge/14021 ? [12:09] leonardr: sure === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ [12:29] leonardr: this looks great! [12:29] leonardr: i'm still reviewing it, i'm just saying the ability to get rid of the browser interaction is cool. === henninge is now known as henninge-lunch [12:30] * leonardr dislikes it, but it's better than letting everybody who hasn't thought out the security problem write their own code === sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ [12:44] leonardr: wrong channel. what is a 209 response code? [12:47] deryck: maybe you'd like to review my windmill fixes? https://code.edge.launchpad.net/~bjornt/launchpad/windmill-problems/+merge/14024 [12:48] BjornT_, sure. [12:49] deryck: cool, thanks [12:49] bac: Conflict [12:49] it means someone else changed the thing you're trying to change [12:50] or, in this case, _you_ already changed the thing you're trying to change [12:50] leonardr: just curious, where is that defined? [12:50] bac: rfc 2616 [12:51] leonardr: it's not here http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html [12:52] bac: oops, i put the wrong number. it's actually 409 [12:52] ah, cool [12:53] BjornT_, in the updates for ensure_login, why didn't you use the timeout constants there? [12:53] leonardr: i wasn't being coy, i thought there may have been an extension i couldn't find === Ursinha-afk is now known as Ursinha [12:53] deryck: mainly to keep the diff size down. i'll update it now, though :) [12:53] bac: i think there is a 209 error code defined in an extension, but it's not conflict [12:54] BjornT_, ok :) [12:54] bac: yes, it's Content Returned, defined as part of PATCH [12:54] BjornT_, also, line 38 of the diff has an extra space after the opening paren. [12:54] leonardr: i wish we used the constants from httplib...but we don't. [12:55] leonardr: the src directory is now empty and can be removed, right? [12:55] bac: no, i moved it by mistake [12:55] launchpadlib should still be in src [12:55] i have no idea how that happened. i just changed it back because the tests were failing [12:55] so you'll move launchpadlib -> src/launchpadlib? [12:56] yes [12:56] ok. i was going to ask about impact on packaging but now that's moot. :) [12:56] in fact, i'm pushing that now [12:56] BjornT_, other than those two minors things, looks good to me. thanks for this! r=me [12:57] and your import of the symbols from uris in launchpad.py for compatability. that's just so they'll be exported, right? if so would you add them to __all__ [12:58] deryck: thanks. here are the changes: http://pastebin.ubuntu.com/302759/ [12:59] deryck: the extra space was there because i had joined the lines by accident. there was supposed to be a line break there [12:59] BjornT_, ok, cool. those changes look good too. [13:07] leonardr: r=bac === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ === mrevell is now known as mrevell-lunch === henninge-lunch is now known as henninge [13:40] bac: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/merge-directive-namespace/+merge/13867 ? [13:42] abentley: yes === abentley changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui || queue: [sinzui, abentley] || This channel is logged: http://irclogs.ubuntu.com/ [13:42] bac: [13:42] bac: thanks. === mrevell-lunch is now known as mrevell [13:55] bac: I'll fling a little 39 lines branch on top of the pile, if you don't mind. [13:56] henninge: fling away === henninge changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui || queue: [sinzui, abentley, henninge] || This channel is logged: http://irclogs.ubuntu.com/ [13:56] bac: cheers! === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: henning || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ [14:15] henninge: full stop on comment on line 8 [14:15] henninge: is the message at line 34 correct? is 'translated' the right verb? [14:16] bac: lemme look [14:17] bac: "have no translation" might be better as it is just a dummy translation. [14:18] henninge: ok. r=bac with those changes [14:18] bac: thank you! [14:22] abentley: the branch looks good. curious as to why you requested an r-c but didn't get it cherry picked. === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ [14:23] bac: I don't understand. I'm waiting on a code review (and test run) before I can land it on production-devel. [14:24] abentley: i was just confused that you got r-c approval four days ago but just now sought code review. [14:24] abentley: i was unclear whether you still wanted a CP for this. [14:24] abentley: either way, it is approved. thanks for the fix. [14:25] Ah. I finished it up around EOD on a Friday, and I was OCR on Monday, and kinda forgot. [14:25] abentley: that'll do it [14:26] bac: It's been sitting in the queue since Friday, but no one's taken the initiative to review it. [14:26] abentley: nudges are useful, as you see. [14:26] bac: can I add another MP to your queue? [14:26] adeuring1: sure [14:26] bac: indubitably. [14:26] bac: thanks! === adeuring1 changed the topic of #launchpad-reviews to: on call: bac || reviewing: henning || queue: [abentley, adeuring] || This channel is logged: http://irclogs.ubuntu.com/ abgeändert === adeuring1 is now known as adeuring === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [abel] || This channel is logged: http://irclogs.ubuntu.com/ === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ [14:34] adeuring: r=bac with one typo fix [14:34] bac: wow, that was fast! thanks! [14:41] bac: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/hide-numbers/+merge/13982 ? [14:42] sinzui: is this ready to be reviewed? https://code.edge.launchpad.net/~barry/launchpad/436503-privacy/+merge/13993 [14:43] sinzui: i think the 'abstain' knocked it off +activereviews. [14:43] abentley: ok === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ [14:43] bac: yes it is. These are the same images I reviewed yesterday. [14:43] bac: Thanks! [14:44] sinzui: i don't understand. you reviewed what? [14:44] sinzui: oh, you reviewed the screenshots... [14:44] bac: barry showed me these images yesterday. I requested a true private team image === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abentley || queue: [barry] || This channel is logged: http://irclogs.ubuntu.com/ [15:00] jml: ready for the call? [15:00] al-maisan_, indeed I am. [15:01] jml: please give me 3 minutes. [15:01] al-maisan_, their yours. [15:04] jml: rrring === absoludity is now known as noodles775 [15:11] abentley: regarding your changes to the dev config for mpcreationjobs, should those settings not be in production too? [15:12] bac: They are already in the production config. [15:12] bac: Production has been running mpcreationjobs just fine. I couldn't run them in devel mode because there was no config. [15:13] abentley: i don't see values for the OOPS prefix in the schema or in production configs [15:14] abentley: never mind [15:14] abentley: forgot the production configs are in a separate branch [15:15] bac: Ah, cool. [15:24] abentley: can you give me some help demoing this fix locally? [15:24] bac: Sure. [15:24] abentley: how do i create a MP with diff on lp.dev? [15:24] bac: Do "make run_all", so that you have a local codehosting. [15:25] done [15:25] bac: Push up two branches, propose them for merging. [15:25] bac: Run "make sync_branches" [15:25] abentley: can i just use a branch that mark owns in sample data? one of the firefox ones? [15:26] bac: You can use a existing branches, as long as they have actual bzr branches. === deryck is now known as deryck[lunch] [15:28] abentley: and i assume they do not. [15:29] bac: I don't know whether they do or not. [15:31] bac: You can certainly push branches to the firefox project as mark. That's what I did. [15:35] abentley: i've never tried that. i assume i need to set launchpad-login to be mark? [15:36] bac: No, you should already be set up for that, and launchpad-login doesn't affect lp://dev [15:38] bac: Your .ssh/config should have https://pastebin.canonical.com/23897/ [15:39] abentley: yep. i get connection refused on 5022 [15:39] did you do "run_all", not just "run"? Did it error? [15:40] abentley: yeah, i did run_all [15:40] bac: And it's still running? [15:40] abentley: rather than debug my setup, would you just confirm the toggly bits work with epiphany [15:40] abentley: i need to move on to other reviews [15:41] abentley: but i want to ensure webkit doesn't barf on the JS [15:41] bac: Checking... [15:42] bac: It works on Epiphany. [15:43] abentley: ok. it looks good. i'll finish up the review now. === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: barry || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ === matsubara is now known as matsubara-lunch === salgado is now known as salgado-lunch === beuno is now known as beuno-lunch [16:05] bac: another branch from me [16:05] https://code.edge.launchpad.net/~leonardr/launchpadlib/json-token-format/+merge/14037 [16:17] leonardr: in your test at line 217 of the diff you say you're testing a script but you're testing the class the script calls. i understand why but it would be nice if you stated that [16:19] hello bac, could you please have a look at a branch of mine as well? [16:19] It's here: https://code.edge.launchpad.net/~al-maisan/launchpad/psds-model-changes-399186/+merge/14038 [16:20] al-maisan_: yes, but i'm starting to wrap things up. === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ [16:20] bac: thanks a million! [16:32] leonardr: can i ask you for a mid-air review of my lazr.restful fix? [16:33] intellectronica, sure [16:34] leonardr: http://pastebin.ubuntu.com/302895/ (which is a fix for https://bugs.edge.launchpad.net/lazr.restful/+bug/438802 ) [16:34] Bug #438802: UnicodeDecodeError changing 'Assigned to' field when summary contains non-ascii

[16:45] intellectronica: ok, but what specifically was the problem? value.decode("utf-8") worked but unicode(value) didn't? [16:46] leonardr: yes, because unicode() will not use utf-8 [16:46] ok [16:46] r=me === salgado-lunch is now known as salgado [16:47] leonardr: as a reviewer i would have asked me to add a test ;) any tips on where i should test this? === matsubara-lunch is now known as matsubara [16:52] sorry, i guess i thought an existing test was failing [16:52] you have a change in the full HTML representation and in the field representation [16:52] yes [16:53] i would change example/base/tests/field.txt and example/base/tests/entry.txt [16:53] leonardr: cool. thanks a bunch [16:53] there should be tests in there that change a field and then verify that the new field shows up in the representation [16:53] change those tests so that you are setting the new field to a utf-8 value === deryck[lunch] is now known as deryck === leonardr is now known as leonardr-afk === bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ === beuno-lunch is now known as beuno === EdwinGrubbs is now known as Edwin-lunch [18:53] rockstar: Could you do a UI review for me? https://code.edge.launchpad.net/~abentley/launchpad/hide-numbers/+merge/13982 === leonardr-afk is now known as leonardr === leonardr is now known as leonardr-afk === Edwin-lunch is now known as EdwinGrubbs [20:17] abentley, done. [20:17] rockstar: Thanks. [20:17] barry: could you do a UI review for me? https://code.edge.launchpad.net/~abentley/launchpad/hide-numbers/+merge/13982 === salgado is now known as salgado-afk [20:18] abentley: barry is not available. I can do it though [20:18] sinzui: Cool, thanks. === leonardr-afk is now known as leonardr [20:30] abentley: sorry for taking so long. make schema is now past 10 minutes [20:32] sinzui: No worries. [20:52] jml, mwhudson: Before I go any further, can I ask you to have a look at something I knocked up earlier: http://pastebin.ubuntu.com/303104/. It moves lib/devscripts into a package of its own, though still in the LP tree, and does some buildout tomfoolery to make it run with the right deps and python2.5. [20:54] allenap: on a trivial level, the comment before boto = 1.8d should stick with the bzr = ... line [20:54] mwhudson: Ah yes, good spot. [20:55] jml, mwhudson: It also uses trial for tests. Maybe that's not a great plan, or maybe it is. I don't really know. [20:55] allenap: for another, i think you need to do something about the utilities/ec2 and utilities/update-sourcecode entrypoints? [20:55] At the time it just seemed a lot easier to get trial to work that to figure out the zope test runner. [20:55] trial is a decent enough test runner i think [20:56] mwhudson: I think I did the utilities/ec2 one, but update-sourcecode I need to do. Another good spot. [20:56] allenap: if you move the update-sourcecode entrypoint, you'll need to change the ec2 === matsubara is now known as matsubara-afk [20:56] mwhudson: Okay, I'll check that. [20:57] allenap: in general, it probably makes sense to separate the devscripts code and the launchpad code a bit more [21:13] abentley: I do not know how to create a merge proposal to view your branch. I created two real branches to test this, but branch-scanner does not seem to find them, and update_preview-diffs does not see anything to do === Ursinha is now known as Ursinha-afk [21:54] rockstar: https://code.edge.launchpad.net/~thumper/launchpad/bmp-inline-commit-message/+merge/13814 plz tick the ui review box :) [21:57] thumper, ack. [21:57] rockstar: ta [21:58] thumper, I still don't like the look of the unset commit message, but I don't know what to do about it. [21:58] thumper, have you put any thought into it? [21:58] rockstar: yes, I thought best not to show anything if there isn't anything there [21:59] rockstar: the multiline editor doesn't support prompt-cues, although perhaps it should [21:59] thumper, yea, but it looks crowded if there's nothing there. [21:59] rockstar: fyi a prompt cue is text in an empty field that goes away when it gets focus [22:00] thumper, yeah, I think I've heard you use that term before. === flacoste is now known as flacoste_afk [22:55] mwhudson, "decent enough test runner" indeed! [22:56] jml: i'm full of faint praise