=== Ursinha is now known as Ursinha-afk [01:49] anyone feel like reviewing some branches? [01:51] thumper: maybe [01:51] https://code.edge.launchpad.net/~thumper/launchpad/rename-created-job/+merge/38681 [01:52] https://code.edge.launchpad.net/~thumper/launchpad/needs-review-event/+merge/38686 [01:52] https://code.edge.launchpad.net/~thumper/launchpad/defer-wip-email/+merge/38791 [01:52] a pipeline to defer the initial review email until it needs review [01:59] thumper: why do you reset the root message id in the last one? [01:59] mwhudson: because it will start a new threaded conversation [01:59] if someone had commented on a work in progress [02:00] that sets the root message id [02:00] i guess that makes sense [02:00] by resetting to send out the main message, we get a new thread [02:00] also if you flip between needs review -> wip -> needs review again, i guess a new thread is appropriate [02:00] yeah, that's my thought too [04:11] mwhudson: were you going to comment on the third one above? [04:11] thumper: oh right, yeah, one sec [04:11] otp [04:11] ack [04:17] another simple one for someone: https://code.edge.launchpad.net/~thumper/launchpad/kill-bad-form-preloads/+merge/38800 === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk [07:48] hi lifeless [07:48] jtv, could i trouble you for a review? [08:08] hi bac [08:08] hi lifeless, you'd pinged me earlier. so i have been unavailable until now [08:08] I did? [08:09] yeah, during the middle of my night. [08:09] lifeless: so i was just replying [08:09] I don't see a ping from me [08:09] are you thinking of the email one ? [08:10] hi bac, still need that review? [08:11] lifeless: no, but i am in the wrong channel. at 19:15Z in #launchpad-dev there was "lifeless: bac: hi" [08:12] lifeless: i'm not making it up, but if you don't need to chat that's a-ok too. :) [08:16] hmm, oh right it was re that mail I think [08:18] lifeless: i'm replying to your email now, so i guess we are covered. [08:18] coolio [08:23] jtv: https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705 [08:23] gotcha [08:24] jtv: henninge grabbed it yesterday but i don't think he actually did anything [08:24] bac: he won't be back for another few hours [08:29] bac: is it okay to inherit both from LaunchpadFormView and LaunchpadView? [08:30] jtv: probably not. i didn't realize that was happening. [08:30] BranchVisibilityPolicyView inherits from LaunchpadView. [08:33] jtv: thanks for catching that. i think it was a late change. i'll investigate. [08:33] bac: it looks as if for your use you really want BVPV to be a mixin. [08:34] jtv: yeah, i may have to go that route. it pre-existed and i thought i could get lucky and re-use it as is [08:34] It might work just fine, it just scares me a bit. [08:34] yes, i think it should [08:44] bac: wasn't it possible for TestProjectGroupBranchesPage to render the view directly, without creating a browser? [08:45] jtv: i was burned so badly by trying to get a rendered translations view last week that i went with what i knew would work [08:46] bac: IIRC it should (knock on wood) be a matter of soup = BeautifulSoup(view()) [08:46] Seems worth trying despite the trouble with our views! [08:47] jtv: good point. i will try. [08:47] thanks [08:55] bac: on a sidenote, there's something in the docstring for that test that confuses me: "This is the default page shown _for a person_ on the code subdomain." Should that be "to" a person (who by implication I guess must be logged in)? [08:57] jtv: fixed [08:57] cool [10:03] bac: also in your test, since you're using the whitespace-ignoring assertion helper, can you get away with multi-line strings for the text you expect in the privacy portlet? [10:04] jtv: what do you mean? [10:04] and why would i want to? [10:04] bac: the indentation is a bit ugly and uses up a lot of horizontal space that you could use for meaningful content. [10:05] 75+ expected = ("By default, new branches you create for projects in .* "76+ "are Public initially. Individual "77+ "projects may override this setting.")78+ self.assertTextMatchesExpressionIgnoreWhitespace(79+ expected, text) [10:05] Hmmm... I bet I can do better: [10:05] 75+ expected = ("By default, new branches you create for projects in .* " [10:05] 76+ "are Public initially. Individual " [10:05] 77+ "projects may override this setting.") [10:05] 78+ self.assertTextMatchesExpressionIgnoreWhitespace( [10:05] 79+ expected, text) [10:05] How about: [10:06] expected = """ [10:06] By default, new branches you create for projects in .* [10:06] are Public initially. Individual projects may override this setting. [10:06] """ [10:06] I don't know if moving the line breaks is a problem…? [10:08] bac: ^^ [10:08] is what I meant [10:09] jtv: it is not a problem [10:10] Then I think the multi-line string is slightly more natural. [10:10] Also, if the BeautifulSoup tests you did are a problem, might it be worth putting more of the portlet's logic in the view and unit-testing the view directly? [10:11] FWIW I got an order-of-magnitude speedup on one of our slowest pages that way. [10:12] I don't know if those tests are still a major time sink for the test though; just a thought. [10:12] they are not [10:13] the portlet isn't really doing much [10:15] ok nm === bac` is now known as bac [11:03] bac: sorry, I totally forgot your review ... [11:03] henninge: np [11:03] you were busy === 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 [11:37] henninge: could i get you to do a UI review of the branch from yesterday? https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705 [11:37] henninge: i'll provide some screenshots in just a moment [11:43] bac: I already saw it. [11:44] henninge: actually hold off a bit on a UI review. i need to tweak something [11:44] bac: I remember that the link in the portlet looked odd. [11:44] i.e. the icon was not properly aligned with the text === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:34] gmb: I've got a large MP with a sadistic title… interested? https://code.launchpad.net/~jtv/launchpad/recife-pofile-owner-privs/+merge/38827 [13:36] jtv: Sure; I'm always willing to have pain brighten my afternoon. === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:36] * jtv laughs [13:41] jtv: there are text conflicts in your mp. Did you merge the latest stable into recife and push that? [13:41] henninge: ah, not yet—thanks [13:42] I merged db-stable into recife earlier today. [13:42] …so recife got lots of changes. [13:43] * jtv merges & twiddles [13:56] gmb: just pushed an update to resolve two very minor conflicts. [13:56] jtv: Okay, thanks. [14:01] My diff has updated. [14:16] gmb: have time for another review today? [14:24] jtv: r=me [14:24] jcsackett: Sure. [14:24] gmb: great, thanks! === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:24] gmb: great, thanks. mp is here: https://code.edge.launchpad.net/~jcsackett/launchpad/new-releases-636060/+merge/38773 [14:24] * gmb looks [14:30] jcsackett: A couple of nitpicks w.r.t styling but otherwise r=me. [14:30] https://code.edge.launchpad.net/~jcsackett/launchpad/new-releases-636060/+merge/38773 [14:31] gmb: cool, thanks. I'll make those changes. === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk === 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 === Ursinha-afk is now known as Ursinha [15:25] mars, i need to go to a doctor's appointment, but i'd like to put https://code.edge.launchpad.net/~leonardr/launchpad/temporary-integration/+merge/38836 on the queue, with the understanding that if it turns out you need any input from me to do the review, you can just put it on hold until i come back [15:27] leonardr: Can I assume that that message applies to me as well? [15:27] gmb, yes indeed [15:28] leonardr: Okay, thanks. === leonardr_ is now known as leonardr [15:33] gmb: can I get a code review? I've already gotten ui reviews on this branch: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-652232-person-code-action-links/+merge/38574 === leonardr is now known as leonardr-afk [15:34] EdwinGrubbs: Sure. I'll put it in the queue after leonardr-afk's branch === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [leonardr, EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:34] * gmb -> getting a drink. bbi10m. [15:37] morning gmb [15:38] gmb, I'll take leonardr-afk's branch === mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,leonardr || queue: [EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:42] mars: Cool. === gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: EdwinGrubbs, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:45] * gmb hates at the '+junk' moniker for the umpteenth time. === salgado is now known as salgado-lunch [15:49] EdwinGrubbs: r=me with a tiny little nitpick === gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:50] gmb: thanks === mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -, leonardr || queue: [matsubara(mars)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:54] thanks mars === matsubara is now known as matsubara-afk === matsubara-afk is now known as matsubara-lunch [16:05] could I get https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-modify-distro-official-tags/+merge/38743 added to the review queue? === deryck is now known as deryck[lunch] [16:22] bdmurray, sure === mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -,afk || queue: [bdmurray,matsubara(mars)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On call: mars || Reviewing: afk || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === benji is now known as benji-lunch === deryck[lunch] is now known as deryck === matsubara-lunch is now known as matsubara [17:09] gmb: did my branch get reviewed or dropped from the queue? [17:13] bdmurray: Whoops, I stomped on it. Sorry. === gmb changed the topic of #launchpad-reviews to: On call: mars || Reviewing: afk || queue: [bdmurray, matsubara] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:13] bdmurray: By way of making it up to you I'll take a look now :) [17:13] gmb: it should be an easy one [17:14] bdmurray: Indeed. r=me [17:15] thanks! [17:23] need a review on a very simple CP: https://code.edge.launchpad.net/~jml/launchpad/codebrowse-config-cp/+merge/38858 === salgado-lunch is now known as salgado === benji-lunch is now known as benji === leonardr-afk is now known as leonardr === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: afk || queue: [matsubara] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [matsubara] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: matsubara || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === Ursinha is now known as Ursinha-lunch [18:25] mars, any advice on where to put the screenshot for the ui review? and who might be around to do it right now? [18:29] leonardr, sinzui is the head reviewer, he may punt to mrevell. people.c.c or devpad are good for screenshots, edit the description to add the link. [18:30] I am a head reviewer? [18:30] * sinzui wonders what super powers that conveys [18:30] * mars anoints sinzui as one of the head UI reviewers [18:31] leonardr, screenshots are often linked in the review comments. the images/movies may be bug attachments or uploaded to people.canonical.com [18:31] sinzui: it's like a phrenologist [18:31] sinzui, it conveys the superpower of mentee delegation, the ability to move mountains by giving the task to other people. [18:31] leonardr, :) [18:31] sinzui: if you can do the review today-ish, maybe i can just put the screenshot on my personal site? [18:32] leonardr, yes and yes === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:37] sinzui, the mp is https://code.edge.launchpad.net/~leonardr/launchpad/temporary-integration/+merge/38836 [18:37] Out for a lunch errand, back shortly === Ursinha-lunch is now known as Ursinha === bryceh changed the topic of #launchpad-reviews to: On call: mars || Reviewing: lunch || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:47] salgado, are you about [19:48] hi sinzui [19:48] bryceh, wow, uhm, a 14,000 line diff - might take me a while [19:48] salgado, do you have time for a UI review? [19:48] bryceh, is any of this stuff generated? [19:48] sinzui, not today, but I can do it early tomorrow if it's not urgent [19:49] mars, no but that seems excessively large [19:49] mars, it's against db-devel, but perhaps the diff was made against devel? [19:50] bryceh, it was made against your previous branch [19:50] and db-devel [19:50] bryceh, have a look: https://code.edge.launchpad.net/~bryce/launchpad/lp-617691-retrieve/+merge/38804 [19:50] leonardr, I would like salgado to do the first UI review of your branch? Can you wait until tomorrow? [19:50] bryceh, it /really/ looks like a lot of generated JavaScript arrays and generated HTML [19:50] sinzui, no problem [19:50] * sinzui is feeling ill [19:51] thanks leonardr [19:51] mars, oh right, there's a testfile html file which is a copy of a page from bugs.freedesktop.org for testing the parser [19:51] bryceh, and the JavaScript looks strange. Is line 840 even valid JS? [19:52] mars, it's intentionally broken, however come to think of it I don't think I added a test to check that so guess I could just drop the file [19:52] * bryceh doublechecks [19:53] lets see what lsdiff says here [19:55] ok, that's better [19:55] mars, ok dropped [19:56] bryceh, http://pastebin.ubuntu.com/516417/ [19:56] 803 lines, much nicer [19:57] henninge: so here; I'm surprised you want me to land code in a higher entropy state than I presented it; thats something I am finding very hard to understand [19:59] lifeless: sorry, entropy? [19:59] ;) [20:00] henninge: worse off ;) [20:00] henninge: you want me to split two things that are tightly related [20:01] in preparation for future work that would combine them [20:01] why is that better ? [20:01] lifeless: well ... [20:01] ... it could serve as an incentive to clean it up quickly. [20:01] thats not the goal of reviews though [20:02] I have tonnes of incentive to fix things in the code base [20:02] I need more time in the day, not more incentives [20:02] same here ;) [20:02] *we* need understandable code and what you are proposing makes things less understandable until the later action is take. [20:03] True. I would very much prefer to do the move first. [20:03] I think that that needs a benefit that is greater than the cost [20:03] What I suggested was the second-best solution. [20:03] in which case we're holding an improvement to the system hostage to existing tech debt [20:04] lifeless: I won't force this on you. Please add the files as you propsed but also add a bug for moving the code. [20:04] I'm delighted to do that [20:04] I'll put an XXX in the config/__init__ and in the new files. [20:04] thanks [20:04] would you like to see an incremental of the XXX ? [20:05] I realise it will be getting late for you [20:05] lifeless: I started late today [20:05] let me look at the mp I think there were a few other issues that I did not mention yet. [20:07] lifeless: ah yes, can you please use "dedent" in fixture.py and test_fixture.py? [20:07] henninge: not easily [20:08] why not? [20:08] henninge: it has to be byte-for-byte [20:08] my experience with dedent in the past on that sort of thing has been traumatic [20:08] is the tag techdebt or tech-debt [20:09] tech-debt, I believe [20:09] yup [20:10] lifeless: I am not aware of those problems with dedent. [20:10] ok, will try [20:10] The other option would be to but the string in a global variable that can be defined at file level. [20:11] has less indention. [20:11] It's just not very readable the way it is now. Thanks for trying. [20:11] I think thats a good idea regardless [20:11] perhaps class scoped. [20:12] could work, too. [20:13] but as I said, I'd like to know more about those dedent problems you mentioned. I never heard of those. [20:14] lifeless: also, I try to avoid multiline if conditions at all costs, as they read badly, too. [20:14] that's in scripts/__init__.py and logger.py [20:14] I don't love them either, but its better than a temp variable most of the time [20:14] why is a temp variable not a good solution? [20:16] they generally wrap as well [20:16] thanks for converting from unittest.TestCase to testtools, btw. ;) [20:16] and you then have an assigned name which can be (mis)used, because its scoped too broadly [20:16] I'd prefer to extract a helper method if the if gets *that* hairy [20:17] but we often have a lot of infunction state in LP, or so it seems to me [20:17] but that is even more overhead. [20:17] yes, but it can be directly tested [20:17] Also, a temp variable can help documentation. [20:17] and isn't bound to a local so can't be misued [20:17] good point. [20:18] for instance those ifs that you notice [20:18] but I don't see such a big risk of misuse [20:18] in another pass at this, I'd be inclined to push that condition onto config [20:18] 'if config.is_testing: [20:18] or 'if config.is_testing():' [20:19] Yes! Brilliant! ;) [20:20] lifeless: Also, I think you can do without the second "if not BaseLayer.persist_test_services:" in layers.py but I may be misreading that. [20:20] probably fallout, I wavered a couple of times each way there [20:20] ok, dedent is working for this [20:20] cool [20:21] I'm using the python reference doc style [20:22] thanks for spotting that unneeded if [20:22] yes, I'd missed nuking it [20:23] I've pushed this up; I haven't done the config change - its fairly small, but I'd want to look around for similar stuff etc, and I'm sure there's more, so its not trivial to do well. [20:28] lifeless: looks good now. Thanks a lot. r=me ;-) [20:29] my pleasure, thanks for the feedback [21:46] mars, have room for another in that queue? [21:57] or, actually, does anyone have time? it's 89 lines, but most of that is headings updated in a doctest. === matsubara is now known as matsubara-afk [22:14] jcsackett, link? === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [bryce, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [22:15] mars: https://code.edge.launchpad.net/~jcsackett/launchpad/add-member-648476/+merge/38887 [22:15] thanks [22:16] jcsackett, rs=mars [22:16] jcsackett, no tests? [22:17] jcsackett: debatable need--it's exactly the same snippet that's tested elsewhere. [22:17] er, mars ^ [22:17] * jcsackett must need more coffee. [22:17] ah, ok, [22:17] jcsackett, well, I approved it [22:17] thanks, mars. === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: bryce || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [22:41] abentley: I've updated the nomination branch per yesterday's discussion === henninge_ is now known as henninge [23:00] bdmurray: past EOD, but I'll look at it first thing before me. [23:03] abentley: okay, thanks === mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: bryce || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === Ursinha is now known as Ursinha-afk