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