/srv/irclogs.ubuntu.com/2009/11/18/#launchpad-reviews.txt

=== ursula is now known as Ursinha
stubhttps://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14980 for staging and dogfood rebuild goodness. Pretty trivial.03:57
thumperstub: shouldn't you be closing cursors?04:15
stubthumper: Nah- they close themselves when they drop out of scope and the script is in autocommit mode anyway04:15
thumperok04:16
thumperdone04:16
thumpertrivial review: https://code.edge.launchpad.net/~thumper/launchpad/branch-visibility-fubar/+merge/1498206:58
mwhudsonthumper: is not specified in the private case really appropriate?07:03
mwhudsoni guess it's consistent with our approach in general07:04
thumpermwhudson: yes, it is consistent with what we have08:14
mwhudsonthumper: fair enough08:15
* mwhudson approves08:15
thumpermwhudson: thanks08:15
* mwhudson wonders if submitting the review is going to time out08:15
mwhudsonah, not quite08:15
=== matsubara-afk is now known as matsubara
gmballenap: Could you do a JS review of https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-471974/+merge/14955 for me? bac suggested I get someone to review the JS specifically.11:59
allenapgmb: Okay.12:00
gmballenap: I'll request a review.12:00
allenapgmb: Cool.12:00
gmbAAAAGH.12:00
* gmb hates at chromium's behaviour12:01
gmballenap: requested. It doesn't say JS, but I guess you can figure that out for yourself.12:01
allenapgmb: Why does it try to load the portlet after the ids?12:32
allenapgmb: I'm going to go to lunch now, so I'll catch up with you later :)12:36
gmballenap: Because it... needs to load the portlet?13:08
gmbI suppose we could load the portlet first, then grab the IDs before setting up the subscribe / unsubscribe links. That might make more sense.13:09
sinzuihenninge: r=me14:02
henningesinzui: cheers14:03
allenapgmb: Ah, I meant, why after? Why not fire off the request at the same time?14:10
abentleyflacoste: Can I get an RC on this for cherry-picking? :14:14
abentleyflacoste: https://code.launchpad.net/~abentley/launchpad/parse-binary-2/+merge/1491514:14
flacosteabentley: approved14:18
abentleyflacoste: Thanks.14:19
gmballenap: Well, we need the IDs before we set up the portlet, or at least before we set up the subscribe / unsubscribe links.14:24
gmballenap: So it might make more sense to load the portlet and then, when that's loaded, get the IDs before setting up the links.14:25
allenapgmb: If the IDs fail to load, and the portlet is loaded anyway, I assume the links will not be set up?14:26
gmballenap: Correct.14:27
gmballenap: In fact, we can use that to our advantage by only setting up the links if the IDs are present.14:27
gmbInstead of trying to set them up anyway with unpredictable results.14:27
allenapgmb: Sounds good.14:28
gmballenap: Cool. I'll go do that now.14:28
allenapgmb: Instead of using a place-holder template into which you put some JSON, and which then you have to parse at client end to get the JSON back out, you could override LaunchpadView.render() to simply return some JSON.14:38
gmballenap: Ooh, sweet. I'll do that, thanks.14:41
allenapgmb: The request.response mime type might need setting too, but it'll probably work without (and I never said that).14:43
gmballenap: Hah. What to, assuming that that's what I need to do?14:44
allenapgmb: application/json I think.14:45
gmballenap: Right, thought as much; wasn't sure about the first bit though.14:45
flacosteabentley: talk to salgado, he also has a r-c14:52
flacosteabentley: you should land it together14:52
* flacoste hopes it's not too late14:52
=== jamalta_ is now known as jamalta
abentleyflacoste: Sorry, it's too late.14:52
bachi allenap14:54
allenapbac: Hi there.14:54
bacallenap: what do you think of http://pastebin.ubuntu.com/321559/14:55
allenapbac: Looks good. Instead of cd'ing in and cd'ing out again, I'd run it in a sub-shell, like (cd $$subdir && bzr clean-tree ...)14:58
=== salgado_ is now known as salgado
bacallenap: hey i'll give that a try14:58
* bac lacks makefile-fu and even bash-fu14:58
bacallenap: hey that worked:  http://pastebin.ubuntu.com/321567/15:02
allenapbac: The exit $$? will only exit the sub-shell I think, but if you move it outside of the braces it should work.15:03
allenapbac: So (cd $$subdir && bzr clean-tree --ignored --force) || exit $$?;15:04
allenapbac: r=me though. And, does this get called as part of a make clean on the top-level Makefile?15:05
bacallenap: it does not15:05
bacallenap: i think cleaning sourcecode is something we should do only rarely and on-demand15:05
bacwhat do you think?15:06
baci just wanted a target to make it easier without a lot of fuss15:06
allenapbac: That's fair.15:06
bacallenap: if we change our minds it's an easy fix later15:07
allenapbac: Yes. If we wire it in now, it probably annoys a lot of people who keep having to rebuild stuff.15:07
bacallenap: and normal tree updates shouldn't need a clean and rebuild.  we only experienced it this week as the 2.4->2.5 change could not trigger a rebuild15:08
bacthanks for the review and improvements15:08
allenapbac: Yep.15:08
allenapbac: While you're there, do you normally reply to the "Project License Submitted" messages?15:08
allenapbac: As in, should I leave them alone?15:08
=== salgado is now known as salgado-lunch
bacplease continue to point them out to me15:09
baci do not regularly review the feedback queue15:09
bacsinzui will often reply to them whether he is CHR or not15:09
allenapbac: Okay. And I've just noticed https://wiki.canonical.com/Launchpad/PolicyandProcess/CommunityHelpRotation/LicensingIssues which I can work through.15:10
allenapbac: Thanks.15:10
bacnp15:10
sinzuiI do, but I only looked once last week. I had to do some project review catchup over the weekend too15:11
=== abentley1 is now known as abentley
gmballenap: I've updated https://code.edge.launchpad.net/~gmb/launchpad/subscribers-timeout-bug-471974/+merge/14955 with an incremental diff of the suggested changes.15:21
allenapgmb: Top banana.15:22
gmbbac: See above about the branch you reviewed yesterday.15:33
bacgmb: will do15:34
gmbThanks15:34
bacgmb:  i'll update the MP.  thanks for adding the test.15:35
gmbbac: No worries. Working on trying to get windmill working now.15:35
bacgmb: my use of "fragility" was from frustration at not being able to figure out why the icon alignment issue was there.  your explanation that it was pre-existing is good.15:36
gmbAh, right.15:37
gmbbac: Yeah, I can't figure that one out, either. That reminds me - I'll file a bug now.15:37
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-lunch
leonardredwin, can you please review https://code.edge.launchpad.net/~leonardr/lazr.restful/double-your-enjoyment/+merge/14967 ?15:43
EdwinGrubbsleonardr: sure15:43
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: leonardr || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
allenapgmb: That's a beaut. One final question: why do you need to do the .replace('bugs.', '') bit?15:45
gmballenap: I think it's to do with the correct path to the bug. I admit, I've cribbed that from what's already there. I'll remove it, see what breaks.15:47
allenapgmb: Cool.15:47
gmballenap: Answer: nuffink. I'll remove it.15:51
allenapgmb: Even cooler.15:51
gmballenap, bac: Thanks.15:52
gmbbac: FTR, the windmill tests break on this machine, but not because of the changes I've made; the changes are tested before the failure happens and I'm seeing the same failure on other branches, so think it's a not-enough-CPU issue with this box.15:58
gmbI *hate* having to say "It fails, but that's okay"15:58
bacgmb: that's fine, in a despicable sort of way15:59
gmbYeah, I feel kinda dirty.15:59
gmbI'll submit it to EC2 then take a shower.15:59
allenapEdwinGrubbs: Will you have time for a small clean-up review? https://code.edge.launchpad.net/~allenap/launchpad/remove-striped_class/+merge/1499616:07
EdwinGrubbsallenap: sure16:08
allenapEdwinGrubbs: Thanks.16:08
=== allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: leonardr || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado-lunch is now known as salgado
EdwinGrubbsleonardr: what does this do? taggedValue(LAZR_WEBSERVICE_NAME, dict(singular="contact", plural="contacts"))16:48
leonardredwin, that sets the human-readable singular and plural name used by lazr.restful in wadl generation16:49
leonardrordinarily it happens automatically when the class is generated16:49
leonardrbut since i'm not using the class generation features, i need to do it manually16:49
EdwinGrubbsleonardr: is there a reason that ContactSet.path="contact" is singular?16:56
leonardredwin: no, that's a mistake. i'll fix it but i probably won't be using .path anyway16:56
EdwinGrubbsleonardr: why do the ITestDataSet.get() methods take a new request parameter but the getAll() methods do not?17:05
leonardredwin: because get() is part of the implementation of ITraverseWithGet (which i made the classes implement instead of Traversable), and getAll() is not part of any interface17:05
EdwinGrubbsleonardr: I see that TraverseWithGet has the request parameter, but ITraverseWithGet does not. Does that need to be updated in this branch?17:10
leonardrmaybe...17:10
leonardredwin: yes, i'll change that while i'm at it17:10
=== matsubara-lunch is now known as matsubara
EdwinGrubbsleonardr: review sent17:14
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
leonardredwin, here's my incremental diff. i fixed everything we talked about and added a tiny bit more tested code17:44
leonardrhttps://pastebin.canonical.com/24847/17:44
leonardri also cleaned up the prose such as unfinished sentences17:44
EdwinGrubbsleonardr: looks good. r=me17:47
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk
intellectronicaEdwinGrubbs: can i ask for a small review, please?21:04
EdwinGrubbsintellectronica: of course21:06
intellectronicaEdwinGrubbs: great. https://code.edge.launchpad.net/~intellectronica/launchpad/bugs-homepage-searchbox/+merge/1500721:11
=== intellectronica changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [allenap, intellectronica] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
intellectronicaEdwinGrubbs: looks like i can get a face-to-face review from deryck here, so don't worry about it22:09
EdwinGrubbsintellectronica: is there a purpose to having a bug supervisor and security contact listed in launchpad if they are not using the launchpad bug tracker? or at least should the edit buttons be removed?22:29
EdwinGrubbsintellectronica: oops, I didn't see your last comment. nevermind.22:33
=== intellectronica changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
sidneiEdwinGrubbs: got time for a review?23:14
EdwinGrubbssidnei: how big is it?23:14
EdwinGrubbssidnei: it looks like I will have a bunch of time between meetings tomorrow at UDS, so I might be able to do it then also.23:15
sidneiEdwinGrubbs: the mp says 2100, but 70% of that is under 'examples' in lazr-js23:16
sidneiEdwinGrubbs: https://code.edge.launchpad.net/~sidnei/lazr-js/combo-service/+merge/1500923:16
EdwinGrubbssidnei: I'll start on it now, but I probably won't finish until tomorrow.23:19
sidneiEdwinGrubbs: im more than happy with that!23:19
al-maisanHello intellectronica, do you mind reviewing a simple and innocuous Soyuz branch? Life at the UDS :)23:58
al-maisan*live23:58
intellectronicaal-maisan: there's no such thing as a simple and iccocuous soyuz branch ... but sure, where are you? :)23:59
bigjoolsal-maisan: he's sat next to me, shall I hit him? :)23:59

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!