| mwhudson | lifeless: reviewed | 00:12 |
|---|---|---|
| lifeless | mwhudson: thanks | 00:14 |
| lifeless | mwhudson: feedback on my feedback? | 00:50 |
| mwhudson | lifeless: ok | 00:53 |
| lifeless | mwhudson: there was a question there :P | 00:55 |
| lifeless | mwhudson: for inlining the matcher | 00:55 |
| 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:56 |
| mwhudson | it was just meant to be a generic acknowledgement, though it was unclear | 00:57 |
| mwhudson | anwyay, typed up now | 00:57 |
| lifeless | thanks | 00:59 |
| mwhudson | a review for https://code.edge.launchpad.net/~mwhudson/launchpad/move-some-code-vocabularies/+merge/35974 woudl be nice | 01:58 |
| lifeless | done | 02:03 |
| lifeless | mwhudson: ^ | 02:05 |
| mwhudson | lifeless: thanks! | 02:06 |
| jtv | Anyone up for a really simple review? mwhudson maybe? | 06:51 |
| jtv | https://code.launchpad.net/~jtv/launchpad/bug-643240/+merge/35984 | 06:51 |
| lifeless | done | 06:54 |
| jtv | thanks! | 06:54 |
| jtv | That was fast | 06:54 |
| lifeless | de nada | 06:55 |
| === 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 | ||
| 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:02 |
| henninge | noodles775: I'd happy to do so but who is that 'nooles' guy? | 12:03 |
| henninge | ;-) | 12:03 |
| noodles775 | woops :) | 12:03 |
| === 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 | ||
| gmb | noodles775, Fixed that for you :) | 12:03 |
| noodles775 | Ta. | 12:03 |
| henninge | sleeping people will have to wait ... ;) | 12:07 |
| === 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 | ||
| henninge | noodles775: Why is the marker interface not defined in interfaces? | 12:15 |
| 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:16 |
| noodles775 | henninge: actually, hangon... | 12:17 |
| noodles775 | henninge: because that's where the code guys had them - I didn't create ICodeReviewNewRevisions (also only used for display). | 12:18 |
| 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:19 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:31 |
| 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 | 12:32 |
| === 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 | ||
| 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:32 |
| 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:33 |
| 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? ^^ | 15:34 |
| === 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] | ||
| abentley | bryceh, you should also request a DB review if you introduce a db patch. | 16:05 |
| === Ursinha-brb is now known as Ursinha | ||
| abentley | bryceh, around? | 16:11 |
| === 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 | ||
| bac | henninge, abentley: a trivial one: https://code.edge.launchpad.net/~bac/launchpad/bug-637102/+merge/36028 | 16:23 |
| === 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 | ||
| abentley | sinzui, would it make sense to extract your monkeypatch of XMLRPCRunner.get_mailing_list_api_proxy to a Context Manager? | 16:34 |
| sinzui | abentley, I do not understand your suggestion, but I welcome a graceful way to choose the proxy. | 16:35 |
| sinzui | ah, mailman is not running with zopisms so my favorite hack or registering something in the testrunners config will not work | 16:36 |
| === Ursinha is now known as Ursinha-lunh | ||
| === Ursinha-lunh is now known as Ursinha-lunch | ||
| 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! | 16:39 |
| === matsubara is now known as matsubara-lunch | ||
| abentley | sinzui, something like this: http://pastebin.ubuntu.com/497078/ | 16:40 |
| abentley | sinzui, it would be possible to generalize this to other mokeypatches. | 16:41 |
| sinzui | abentley, I love this | 16:41 |
| abentley | sinzui, great. | 16:42 |
| abentley | s/addContext/useContext | 16:42 |
| sinzui | notes | 16:42 |
| sinzui | noted | 16:42 |
| sinzui | I am do excited to have solved the mailman monkeypatch test strategy. No more anxiety when I hear mailman is broken | 16:43 |
| abentley | sinzui, for paranoia, it could make sense to check that self.mailing_list.getReviewableMessages().count() == 0 before you make any changes. | 16:47 |
| 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:48 |
| abentley | sinzui, r=me | 16:51 |
| sinzui | thanks. I will integrate the changes shortly | 16:52 |
| === 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 | ||
| henninge | gmb: approved. Sorry, I should have looked at it sooner as it was really niece and smooth reviewing ... ;-) | 16:52 |
| === 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 | ||
| gmb | henninge, No worries, and thanks. | 16:53 |
| abentley | bac, r=me | 16:53 |
| === 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 | ||
| 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:04 |
| bryceh | abentley-lunch, yep? | 17:07 |
| === benji is now known as benji-lunch | ||
| === Ursinha-lunch is now known as Ursinha | ||
| === matsubara-lunch is now known as matsubara | ||
| 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:18 |
| 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:19 |
| sinzui | you will need --merge3, but let me look locally | 17:20 |
| === salgado is now known as salgado-lunch | ||
| * sinzui pulls tip | 17:21 | |
| leonardr | sinzui, my revision is 11578 | 17:21 |
| leonardr | in case that wasn't clear | 17:21 |
| 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:23 |
| 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:43 |
| abentley-lunch | leonardr, though --merge3 is the default, so you only need it if you've overridden the default. | 17:44 |
| leonardr | abentley-lunch: i think i've got it... | 17:45 |
| === abentley-lunch is now known as abentley | ||
| 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. | 18:01 |
| === jtv is now known as jtv-zzz | ||
| abentley | bryceh, perhaps they hadn't landed at the time you proposed the merge? | 18:02 |
| 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:03 |
| rockstar | gmb, I'll have it finished when you get up tomorrow, is that good? | 18:04 |
| abentley | bryceh, I think the new style is to define permissions in terms of interfaces. Did you consider that option? | 18:05 |
| leonardr | sinzui: https://code.edge.launchpad.net/~leonardr/launchpad/revert-oauth-aware-website/+merge/36038 | 18:08 |
| === benji-lunch is now known as benji | ||
| bdmurray | abentley: Could I get https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-bug-activity/+merge/35897 added to the queue? | 18:23 |
| abentley | bdmurray, sure. | 18:24 |
| abentley | bryceh, OCR is meant to be interactive. Please ping me when you are up for an interactive review. | 18:24 |
| === 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 | ||
| abentley | bdmurray, what is a BugField? | 18:27 |
| bdmurray | abentley: double checking | 18:27 |
| bdmurray | abentley: its a field that allows one to enter a bug number so maybe its unnecessary | 18:30 |
| abentley | bdmurray, I would expect IBugActivity.bug to be a reference to the bug, but it sounds like it's actually an int? | 18:32 |
| bdmurray | abentley: yes, an int, the bug number, is correct | 18:33 |
| 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:34 |
| abentley | bdmurray, classes should have blank lines between their entries, e.g. export_as_webservice_entry(), datechanged, etc. | 18:35 |
| abentley | bdmurray, the description of message is copy-pasted. | 18:36 |
| bdmurray | abentley: could you elaborate on both of those points? | 18:36 |
| === salgado-lunch is now known as salgado | ||
| 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:39 |
| abentley | bdmurray, lines 33, 78, 80, 84, 88, 92, 96, 100 | 18:40 |
| bdmurray | abentley: hmm, there are not blank lines in IBug from interfaces/bug.py nor in IBugTask from interfaces/bugtask.py | 18:42 |
| abentley | bdmurray, that may be so. | 18:43 |
| 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:45 |
| sinzui | just delete the mp and be sure to choose /devel (launchpad's default is db-devel) | 18:46 |
| 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:47 |
| 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:52 |
| 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:53 |
| abentley | bdmurray, for the copy/paste, "message" is described as "The related property of the bug that changed." which is not accurate. | 18:57 |
| bdmurray | abentley: do you have an idea for a more accurate description? I couldn't find anything describing the field. | 18:59 |
| abentley | bdmurray, sorry, I don't know what the field is used for. | 19:01 |
| 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:02 |
| bdmurray | abentley: deryck and I've discussed the description some and have something better | 19:25 |
| abentley | bdmurray, great. | 19:25 |
| === salgado is now known as salgado-doctor | ||
| abentley | bdmurray, ping me when you've pushed your changes. | 19:31 |
| === 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 | ||
| 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:38 |
| 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:40 |
| 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:42 |
| abentley | rockstar, okay, let's fix getViewBrowser, then. | 19:43 |
| gmb | rockstar, That works for me, thanks. | 19:51 |
| rockstar | abentley, getViewBrowser's login stuff goes pretty deep... | 19:54 |
| 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:55 |
| abentley | rockstar, cool. | 19:56 |
| 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:20 |
| abentley | rockstar, r=me. | 20:22 |
| rockstar | abentley, yay. | 20:22 |
| 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:28 |
| 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:29 |
| abentley | bdmurray, I'll have a look in a minute. | 20:30 |
| 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:34 |
| abentley | gary_poster, if someone uncommits a revision. | 20:35 |
| 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:36 |
| abentley | gary_poster, np. | 20:37 |
| 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:44 |
| abentley | bdmurray, I notice that you're still using BugField rather than Int. Rationale? | 20:50 |
| 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:51 |
| abentley | bdmurray, so bug is a reference to the bug, not an Int? | 20:52 |
| bdmurray | abentley: yes, afaict | 20:52 |
| 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:54 |
| bdmurray | abentley: yes, when exported it as an Int I was seeing bug properties like latest_patch_uploaded in a dictionary. | 20:55 |
| abentley | bdmurray, r=me. | 20:56 |
| bdmurray | abentley: great, thanks! | 20:57 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!