[00:12] <mwhudson> lifeless: reviewed
[00:14] <lifeless> mwhudson: thanks
[00:50] <lifeless> mwhudson: feedback on my feedback?
[00:53] <mwhudson> lifeless: ok
[00:55] <lifeless> mwhudson: there was a question there :P
[00:55] <lifeless> mwhudson: for inlining the matcher
[00:56] <mwhudson> lifeless: typing it up now, sorry :)
[00:56] <lifeless> ah de nada
[00:56] <lifeless> I thought you were saying 'ok do it go ahead'
[00:57] <mwhudson> it was just meant to be a generic acknowledgement, though it was unclear
[00:57] <mwhudson> anwyay, typed up now
[00:59] <lifeless> thanks
[01:58] <mwhudson> a review for https://code.edge.launchpad.net/~mwhudson/launchpad/move-some-code-vocabularies/+merge/35974 woudl be nice
[02:03] <lifeless> done
[02:05] <lifeless> mwhudson: ^
[02:06] <mwhudson> lifeless: thanks!
[06:51] <jtv> Anyone up for a really simple review?  mwhudson maybe?
[06:51] <jtv> https://code.launchpad.net/~jtv/launchpad/bug-643240/+merge/35984
[06:54] <lifeless> done
[06:54] <jtv> thanks!
[06:54] <jtv> That was fast
[06:55] <lifeless> de nada
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== mrevell_ is now known as mrevell
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bryceharrington || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== bigjools is now known as bigjools-afk
=== noodles775 changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bryceharrington || queue: [nooles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[12:02] <gmb> henninge, Good afternoon. Can I add a branch to your queue?
[12:02] <noodles775> Hi henninge, when you've time, could you review an incremental for an approved MP? I've linked to a diff from the last comment at: https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
[12:02] <henninge> gmb: sure, please do
[12:03] <henninge> noodles775: I'd happy to do so but who is that 'nooles' guy?
[12:03] <henninge> ;-)
[12:03] <noodles775> woops :)
=== gmb changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bryceharrington || queue: [noodles775, gmb]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[12:03] <gmb> noodles775, Fixed that for you :)
[12:03] <noodles775> Ta.
[12:07] <henninge> sleeping people will have to wait ... ;)
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: noodles775 || queue: [noodles775, gmb, bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: noodles775 || queue: [gmb, bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[12:15] <henninge> noodles775: Why is the marker interface not defined in interfaces?
[12:16] <noodles775> henninge: I'd put it there in the browser module because its only used for displaying comments, but right, that's probably an old habit.
[12:17] <noodles775> henninge: actually, hangon...
[12:18] <noodles775> henninge: because that's where the code guys had them - I didn't create ICodeReviewNewRevisions (also only used for display).
[12:19] <noodles775> I assume they had them there for that reason (see the comment for ICodeReviewNewRevisions in the diff)
[12:19] <henninge> noodles775: I see.
[12:28] <henninge> noodles775: I don't pretend to fully understand all effects of your change but what makes the attibutes move from the view to view.comment?
[12:29] <noodles775> henninge: that's not part of that diff - which is why it would be hard to understand. That was why a test failed :)
[12:29] <henninge> noodles775: ah!
[12:29] <noodles775> I'll just find the relevant lines of the MP diff...
[12:30] <noodles775> So from line 72ff on the MP diff, a bunch of attributes are moved from the view to the CodeReviewDisplayComment object.
[12:30] <noodles775> which caused the test failure because the templates were using the view :)
[12:31] <noodles775> henninge: sorry, I need to run out the door in a few mins for lunch... are there any other thoughts/questions that you've got?
[12:31] <henninge> noodles775: thanks, I understand
[12:32] <henninge> noodles775: no, I'll approve now. I didn't find anything ... ;) r=me
[12:32] <noodles775> Thanks henninge
[12:32]  * henninge runs to lunch, too
[12:32] <henninge> np
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [gmb, bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha-afk is now known as Urisnha
=== Urisnha is now known as Ursinha
=== sinzui changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [gmb, bryceharrington, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha is now known as Ursinha-brb
=== sinzui changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [gmb, bryceharrington, sinzui, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[15:32] <deryck> sinzui and salgado.  Hi!  bdmurray wasn't sure how to proceed, so I commented on his +subscriptions work that you guys reviewed.  If either of you could look and see if you agree with me, I'd appreciate it.
[15:32] <deryck> Just to help move him forward again.
[15:33] <sinzui> I was waiting for salgado to respond. I think your proposed path is in line with my suggestions
[15:33] <sinzui> I really hate the so-called story test though
[15:34] <salgado> deryck, sinzui, just saw it here, will reply
[15:34] <deryck> yeah, I agree.  I think it's premature at this point.
[15:34] <deryck> Maybe he should just do view unit tests until we have a complete user story.
[15:34] <deryck> sinzui, what do you think of that? ^^
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: gmb || queue: [bryceharrington, sinzui, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, - || queue: [bryceharrington, sinzui, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== deryck is now known as deryck[lunch]
[16:05] <abentley> bryceh, you should also request a DB review if you introduce a db patch.
=== Ursinha-brb is now known as Ursinha
[16:11] <abentley> bryceh, around?
=== abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, sinzui || queue: [bryceharrington, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, sinzui || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:23] <bac> henninge, abentley: a trivial one:  https://code.edge.launchpad.net/~bac/launchpad/bug-637102/+merge/36028
=== bac changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, sinzui || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:34] <abentley> sinzui, would it make sense to extract your monkeypatch of XMLRPCRunner.get_mailing_list_api_proxy to a Context Manager?
[16:35] <sinzui> abentley, I do not understand your suggestion, but I welcome a graceful way to choose the proxy.
[16:36] <sinzui> ah, mailman is not running with zopisms so my favorite hack or registering something in the testrunners config will not work
=== Ursinha is now known as Ursinha-lunh
=== Ursinha-lunh is now known as Ursinha-lunch
[16:39] <sinzui> abentley, If you can help me factor out the monkey patch, I will be delighted
[16:39] <abentley> sinzui, am writing up example.
[16:39] <sinzui> thanks!
=== matsubara is now known as matsubara-lunch
[16:40] <abentley> sinzui, something like this: http://pastebin.ubuntu.com/497078/
[16:41] <abentley> sinzui, it would be possible to generalize this to other mokeypatches.
[16:41] <sinzui> abentley, I love this
[16:42] <abentley> sinzui, great.
[16:42] <abentley> s/addContext/useContext
[16:42] <sinzui> notes
[16:42] <sinzui> noted
[16:43] <sinzui> I am do excited to have solved the mailman monkeypatch test strategy. No more anxiety when I hear mailman is broken
[16:47] <abentley> sinzui, for paranoia, it could make sense to check that self.mailing_list.getReviewableMessages().count() == 0 before you make any changes.
[16:48] <sinzui> abentley, I can add that. I have a lot of confidence since the team is new in each case, and I am only testing one message each time, that if that fails, then teardown failed
[16:48] <abentley> sinzui, okay.
[16:51] <abentley> sinzui, r=me
[16:52] <sinzui> thanks. I will integrate the changes shortly
=== henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, - || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:52] <henninge> gmb: approved. Sorry, I should have looked at it sooner as it was really niece and smooth reviewing ... ;-)
=== henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:53] <gmb> henninge, No worries, and thanks.
[16:53] <abentley> bac, r=me
=== abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== abentley is now known as abentley-lunch
=== henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== deryck[lunch] is now known as deryck
[17:04] <gmb> rockstar, Did you have chance to look at my lazr-js branch yet?
[17:04] <bac> thanks abentley-lunch
[17:04] <gmb> (Realising of course that Monday has just started for you... :))
[17:07] <bryceh> abentley-lunch, yep?
=== benji is now known as benji-lunch
=== Ursinha-lunch is now known as Ursinha
=== matsubara-lunch is now known as matsubara
[17:18] <leonardr> sinzui: i landed the branch you reviewed for me a few days back, and then immediately discovered that the requirements have changed so as to render the branch moot. can you help me a) review a branch that reverts the change i just made, and b) help me construct that branch in the first place?
[17:18] <sinzui> :(
[17:18] <sinzui> leonardr, do you want bzr command to do a reverse merge?
[17:19] <leonardr> sinzui: exactly
[17:19] <leonardr> i just tried bzr merge -r 11577..11578 and it ran, but i haven't checked if it's right yet
[17:19] <leonardr> no, that didn't work
[17:20] <sinzui> you will need --merge3, but let me look locally
=== salgado is now known as salgado-lunch
[17:21]  * sinzui pulls tip
[17:21] <leonardr> sinzui, my revision is 11578
[17:21] <leonardr> in case that wasn't clear
[17:23] <sinzui> bzr cdiff -r 11578..11577 | less -R
[17:23] <leonardr> yes, that looks like it will undo my change
[17:23] <sinzui> then...
[17:23] <sinzui> bzr merge --merge3 -r 11578..11577
[17:43] <abentley-lunch> leonardr, you need to specify the branch, unless you want 11578/11577 to be interpreted relative to the branch you normally merge from.
[17:43] <abentley-lunch> leonardr, so "bzr merge --merge3 -r 11578..11577 ."
[17:44] <abentley-lunch> leonardr, though --merge3 is the default, so you only need it if you've overridden the default.
[17:45] <leonardr> abentley-lunch: i think i've got it...
=== abentley-lunch is now known as abentley
[18:01] <abentley> bryceh, since this branch is proposed to land in db-devel, it's unlikely that the db patches would be shown if they were already in db-devel.
=== jtv is now known as jtv-zzz
[18:02] <abentley> bryceh, perhaps they hadn't landed at the time you proposed the merge?
[18:03] <rockstar> gmb, I did look at it enough to realize it needs more of my attention than I could give to it at the time.
[18:04] <rockstar> gmb, I'll have it finished when you get up tomorrow, is that good?
[18:05] <abentley> bryceh, I think the new style is to define permissions in terms of interfaces.  Did you consider that option?
[18:08] <leonardr> sinzui: https://code.edge.launchpad.net/~leonardr/launchpad/revert-oauth-aware-website/+merge/36038
=== benji-lunch is now known as benji
[18:23] <bdmurray> abentley: Could I get https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-bug-activity/+merge/35897 added to the queue?
[18:24] <abentley> bdmurray, sure.
[18:24] <abentley> bryceh, OCR is meant to be interactive.  Please ping me when you are up for an interactive review.
=== abentley changed the topic of #launchpad-reviews to:  On call: abentley || Reviewing: bdmurray || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[18:27] <abentley> bdmurray, what is a BugField?
[18:27] <bdmurray> abentley: double checking
[18:30] <bdmurray> abentley: its a field that allows one to enter a bug number so maybe its unnecessary
[18:32] <abentley> bdmurray, I would expect IBugActivity.bug to be a reference to the bug, but it sounds like it's actually an int?
[18:33] <bdmurray> abentley: yes, an int, the bug number, is correct
[18:34] <abentley> bdmurray, ISTM that specifying it as an Int will be clearer, and since users don't edit BugActivity, we won't lose any functionality.
[18:34] <bdmurray> abentley: right, that makes sense to me
[18:35] <abentley> bdmurray, classes should have blank lines between their entries, e.g. export_as_webservice_entry(), datechanged, etc.
[18:36] <abentley> bdmurray, the description of message is copy-pasted.
[18:36] <bdmurray> abentley: could you elaborate on both of those points?
=== salgado-lunch is now known as salgado
[18:39] <abentley> bdmurray, I am generalizing from the PEP8 requirement "Method definitions inside a class are separated by a single blank line."
[18:39] <bdmurray> abentley: I meant I don't see where the error is
[18:40] <abentley> bdmurray, lines 33, 78, 80, 84, 88, 92, 96, 100
[18:42] <bdmurray> abentley: hmm, there are not blank lines in IBug from interfaces/bug.py nor in IBugTask from interfaces/bugtask.py
[18:43] <abentley> bdmurray, that may be so.
[18:45] <sinzui> leonardr, I think the MP is for the wrong branch. I see your work and the work of others. Maybe you meant to merge into devel?
[18:45] <leonardr> sinzui, thanks, i was trying to figure out what was the problem
[18:46] <sinzui> just delete the mp and be sure to choose /devel (launchpad's default is db-devel)
[18:47] <leonardr> sinzui: try https://code.edge.launchpad.net/~leonardr/launchpad/revert-oauth-aware-website/+merge/36040
[18:47]  * sinzui waits for diff
[18:47] <abentley> bdmurray, it's possible that there's some skew in team styles here.  It's also possible that those files are not good examples, because they were created a long time ago.
[18:47] <bdmurray> abentley: okay
[18:52] <sinzui> leonardr, r=me
[18:52] <leonardr> sinzui: think i should ec2 test or is that overkill?
[18:52] <sinzui> leonardr, I would ec2 to cover the small case where someone made a change on top of your changes
[18:53] <leonardr> sinzui, ok
[18:53] <abentley> bdmurray, I can't see anything definitive in our style guide.  https://dev.launchpad.net/PythonStyleGuide  The 10-line example under Properties hints at it.
[18:57] <abentley> bdmurray, for the copy/paste, "message" is described as "The related property of the bug that changed." which is not accurate.
[18:59] <bdmurray> abentley: do you have an idea for a more accurate description?  I couldn't find anything describing the field.
[19:01] <abentley> bdmurray, sorry, I don't know what the field is used for.
[19:02] <abentley> I think the test at line 154 is not useful.  It would not catch a case where bug activity were exposed as /bug_activity, for example.
[19:25] <bdmurray> abentley: deryck and I've discussed the description some and have something better
[19:25] <abentley> bdmurray, great.
=== salgado is now known as salgado-doctor
[19:31] <abentley> bdmurray, ping me when you've pushed your changes.
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [bdmurray]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[19:38] <rockstar> abentley, here's a really little one.
[19:38] <rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/not-logged-in-recipes/+merge/36049
[19:40] <abentley> rockstar, don't introduce shout_at_engineer.  Use removeSecurityProxy if the usage is legit, or an XXX if there's a fix needed.
[19:42] <rockstar> abentley, ah, hadn't seen that.
[19:42] <abentley> rockstar, you can also use getViewBrowser instead of canonical_url/logout/setupBrowser.
[19:42] <rockstar> abentley, no, I can't.  getViewBrowser logs me in.
[19:43] <abentley> rockstar, okay, let's fix getViewBrowser, then.
[19:51] <gmb> rockstar, That works for me, thanks.
[19:54] <rockstar> abentley, getViewBrowser's login stuff goes pretty deep...
[19:55] <abentley> rockstar, can't we just add a no_login parameter and call setupBrowser instead of getUserBrowser?
[19:55] <rockstar> abentley, yup, that's what I'm doing.
[19:56] <abentley> rockstar, cool.
[20:20] <rockstar> abentley, updated (and removed the other instance of setupBrowser in another test) https://code.edge.launchpad.net/~rockstar/launchpad/not-logged-in-recipes/+merge/36049
[20:22] <abentley> rockstar, r=me.
[20:22] <rockstar> abentley, yay.
[20:28] <gary_poster> abentley: quick one, maybe (and if it's not, maybe we should ignore the branch for now, because it was a flyby): https://code.edge.launchpad.net/~gary/launchpad/bug643715/+merge/36060
[20:29] <bdmurray> abentley: I've pushed an updated branch leaving bug as a BugField as that is a link to the bug report and Int returns a dictionary of all the bug properties
[20:30] <abentley> bdmurray, I'll have a look in a minute.
[20:34] <abentley> gary_poster, your patch is treating revnos as if they were revision-ids.  It will fail to do an update if the definition of the revno has changed.
[20:34] <gary_poster> abentley: agreed, but when would that happen for this use case
[20:35] <abentley> gary_poster, if someone uncommits a revision.
[20:36] <gary_poster> ew, didn't know you could do that with a public branch but I guess that makes sense within the model
[20:36] <gary_poster> ok, that's a shame, but if that's a reasonable concern then we should just drop it.  thanks for the look abentley
[20:37] <abentley> gary_poster, np.
[20:44] <abentley> gary_poster, I think you also may not have accounted for the possibility that the branch has changed.  Also, it appears that your test will give false negatives if "revision" is not a revno.
[20:50] <abentley> bdmurray, I notice that you're still using BugField rather than Int.  Rationale?
[20:51] <bdmurray> leaving bug as a BugField as                   that is a link to the bug report and Int returns a dictionary of all  the bug properties
[20:52] <abentley> bdmurray, so bug is a reference to the bug, not an Int?
[20:52] <bdmurray> abentley: yes, afaict
[20:54] <abentley> bdmurray, okay.  "Int returns a dictionary of all  the bug properties" means that when you use it via the WS, you get a dict?
[20:55] <bdmurray> abentley: yes, when exported it as an Int I was seeing bug properties like latest_patch_uploaded in a dictionary.
[20:56] <abentley> bdmurray, r=me.
[20:57] <bdmurray> abentley: great, thanks!
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== abentley 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