| === Ursinha is now known as Ursinha-afk | ||
| lifeless | LFR https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/35774 | 05:23 |
|---|---|---|
| jtv | allenap: I've got a modest branch up for review as well, if you have time: https://code.launchpad.net/~jtv/launchpad/bug-594220/+merge/35770 | 05:51 |
| jtv | (bit early for you though innit?) | 05:51 |
| jtv | StevenK: is allenap really still reviewing your branch, or is the topic outdated? | 06:03 |
| lifeless | stale topic | 06:03 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | so there | 06:04 |
| jtv | and this system has gotten too weird for me; I'll reboot | 06:04 |
| jtv | hi henninge! | 07:33 |
| henninge | Hi jtv! | 07:34 |
| * StevenK gets more tempted to write a review helper bot that will manage the topic and MPs | 07:35 | |
| StevenK | I could use PoCo::IRC just to annoy people too :-P | 07:35 |
| allenap | jtv: Sorry, I forgot to take my name out of the topic :-/ | 09:13 |
| jtv | allenap: we figured it out, no worries :) | 09:13 |
| StevenK | allenap: And you didn't actually review my branch at all! | 09:13 |
| allenap | StevenK: I was a whisker from finishing your branch review last night but I had to go. I'll finish it now. | 09:13 |
| StevenK | Hehe | 09:13 |
| StevenK | allenap: It's perfectly fine, thanks. :-) | 09:13 |
| allenap | StevenK: Done, needs (a bit of) fixing :) | 09:20 |
| noodles775 | henninge: When you've time: http://launchpadlibrarian.net/55794871/635005-updated.png | 09:21 |
| noodles775 | There are two things I didn't do (indentation, download link) and one extra thing (binary descriptions). | 09:22 |
| noodles775 | henninge: I did try the indentation, but it seems that we explicitly set ul's and dl's to margin/padding 0px; so I think that's a site-wide decision that I didn't want to touch as part of this branch. | 09:23 |
| noodles775 | Regarding the download link, .... actually, I'll send the MP email, so your mentor can also read. | 09:23 |
| noodles775 | henninge: hrm, I'd already mentioned the link on the previous email. So, nothing more to say :) | 09:24 |
| StevenK | allenap: I can't use the ZCML for auth, sadly. Using soyuz-team is a stop-gap first-step. When we want other people to be able to use it, it will switch to require the user being a driver of the distribution. | 09:24 |
| allenap | StevenK: Can you explain why? | 09:24 |
| StevenK | allenap: Why we require only soyuz-team, or the user being a driver? | 09:25 |
| allenap | StevenK: No, why not ZCML? Is there a technical reason? I'm interested as much as anything. | 09:26 |
| StevenK | allenap: I didn't think you can use ZCML if it requires something more complex than the permission the user has. | 09:27 |
| allenap | StevenK: Permissions are given to user by a security adapter based upon the context. So, for an IDistroSeries, launchpad.Edit can be granted if the user is in ~soyuz-team. Take a look at some of the adapters in canonical.launchpad.security. | 09:28 |
| StevenK | allenap: Ah, sorry -- I was skipping ahead. ~soyuz-team is not the final solution. I don't think I can do it if it's dependant on what distribution the belongs to | 09:29 |
| StevenK | Sigh, words are not my friend today | 09:30 |
| StevenK | allenap: I have a stand-up starting, well, when bigjools notices what time it is. Would you like a 5 minute chat about it after that? | 09:31 |
| allenap | StevenK: :) Sure, ping me when you've got time. | 09:31 |
| henninge | noodles775: care to paste the mp url again, please? ;-) | 09:33 |
| noodles775 | henninge: https://code.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640 | 09:33 |
| StevenK | allenap: Ready when you are, are you mumble-enabled? | 09:43 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | noodles775: approved* ;-) | 10:04 |
| noodles775 | Thanks henninge | 10:04 |
| * henninge needs to reboot computer | 10:06 | |
| === bigjools changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [jtv, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | happy birthday henninge | 10:16 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | hey abel, stealth review—thanks! | 10:37 |
| === mrevell is now known as mrevell-lunch | ||
| noodles775 | Hi adeuring, I've got a 797liner if you've time. Just let me know if you'd prefer I split it up: | 12:23 |
| noodles775 | https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640 | 12:23 |
| adeuring | noodles775: nah, I'll look at the whole branch | 12:23 |
| noodles775 | adeuring: great, thanks. I'll just update the description explaining what the bloat is. | 12:24 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | adeuring: description updated. | 12:27 |
| noodles775 | adeuring: also (sorry), I've just realised that I no-longer need the generic CommentView... I'll remove it. | 12:33 |
| adeuring | noodles775: no problem -- i just realized that I hadn't have lunch yet, will make a lunch break before I start to look at your branch ;) | 12:34 |
| noodles775 | Great, enjoy :) | 12:34 |
| === mrevell-lunch is now known as mrevell | ||
| === matsubara-afk is now known as matsubara | ||
| bac | hi adeuring | 13:57 |
| bac | can i add one to your queue? | 13:57 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: noodles775, - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | adeuring: https://code.edge.launchpad.net/~bac/launchpad/lp-db-setup/+merge/35822 | 13:58 |
| gmb | adeuring, bac I have a very large (1300 lines) lazr-js branch that needs review. This adds a new Wizard widget. Can either of you guys review it? | 13:58 |
| gmb | (A lot of those lines are '},' of course ;)) | 13:59 |
| bac | gmb: i will but you may still want to run it by a JS expert | 13:59 |
| adeuring | bac: sure, let me first finish noodles775 review | 14:00 |
| bac | adeuring: no rush | 14:00 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: noodles775, gmb-js-from-hell || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | bac, Eyefankyoo. I'll see if I can find an eggspurt. | 14:01 |
| gmb | bac, https://code.edge.launchpad.net/~gmb/lazr-js/wizard-widget/+merge/35827, FTR. | 14:02 |
| === Ursinha-afk is now known as Ursinha | ||
| gmb | mars, Around? | 14:12 |
| adeuring | noodles775: r=me | 14:30 |
| noodles775 | Thanks adeuring | 14:30 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bac, gmb-js-from-hell || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews abgeändert | ||
| noodles775 | rockstar: Hi! When you get around to mentoring henninge's UI review, would you mind also taking a brief look at the code and make sure I'm not abusing the lp.services.comments in a way you guys didn't intend? | 14:31 |
| noodles775 | https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640 | 14:32 |
| mars | gmb, pong, what's up? | 14:45 |
| gmb | mars, I have a large (~1300-line) lazr-js branch that adds a Wizard widget (building upon work that rockstar had already done, but which hadn't landed). bac is reviewing it but he suggested that I should find a JS expert to take a look, too. Your name was top of my list. Do you have time to take a look? | 14:47 |
| mars | gmb, unfortunately I do not have time today. | 14:48 |
| mars | gmb, Paul and Edwin are two other good candidates | 14:49 |
| gmb | mars, Okay, no worries. I'll ask someone else. | 14:49 |
| gmb | Indeed. | 14:49 |
| gmb | So, rockstar, are you free to look at how I've mangled your widget? | 14:49 |
| gmb | ooh, that sounded wrong. | 14:49 |
| adeuring | bac: r=me | 14:49 |
| mars | lol | 14:49 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, gmb-js-from-hell || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | adeuring: thanks | 14:53 |
| bac | gmb: dumb question - should the examples work if simply loaded into ff | 15:12 |
| gmb | bac, Yes. And that said, let me check that I didn't break them, 'cos I've just realised that I might have. | 15:12 |
| bac | gmb: yeah, none of them do squat for me | 15:13 |
| bac | gmb: could you have messed them all up? | 15:13 |
| gmb | bac, Works for me (bar one small tweak; fixed and pushed). | 15:14 |
| gmb | bac, Did you make build in the root of the branch? | 15:14 |
| bac | gmb: gah | 15:15 |
| gmb | Hah | 15:15 |
| gmb | bac, It took me a while to figure that out, too. | 15:16 |
| gmb | JS shouldn't be this hard. YUI, I'm looking at you, here. | 15:16 |
| bac | gmb: ok, so i got it built and most JS examples are working now. your wizard doesn't seem to, though, as it just shows Step One | 15:23 |
| bac | gmb: am i doing something else wrong? | 15:24 |
| gmb | bac, So, the 'next' button doesn't work, then? | 15:24 |
| gmb | Note that I've spent more time on tests than on the example, so if it's lacking, that's why. | 15:25 |
| bac | gmb: there is no next button. just a "re-open the wizard" link | 15:25 |
| gmb | ... | 15:25 |
| gmb | bac, That's very, very odd. | 15:26 |
| gmb | Can you take a look at Firebug, see if there are any errors? | 15:27 |
| gmb | bac, In fact, I'd go so far as to say the example should be completely ignored, because it's rubbish compared to other examples. | 15:28 |
| bac | gmb: if i load the file i do not get an overlay | 15:31 |
| bac | but if i open firebug and then load the file then the overlay appears | 15:31 |
| * gmb tries to reproduce | 15:31 | |
| rockstar | gmb, I'm happy to look at it if you don't have a reviewer yet. | 15:32 |
| gmb | rockstar, bac's looking but having some issues; I'd appreciate your eye on the code itself (ignore the example, though, it's bobbins). | 15:34 |
| gmb | Huh. | 15:35 |
| bac | rockstar: i will do a full review per our discussion a few weeks ago that everyone should do JS reviews | 15:35 |
| gmb | bac, I can reproduce that. | 15:35 |
| gmb | bac, Try chromium. It works fine there. | 15:35 |
| bac | :) | 15:35 |
| rockstar | bac, great, thanks. | 15:35 |
| gmb | Meanwhile, I'll try to hammer firefox into submission. | 15:35 |
| bac | rockstar: but i suggested to gmb that he get a JS expert to look at it too | 15:36 |
| rockstar | bac, yeah, I'd at least like to look at it from a "is it YUI-y" standpoint. | 15:36 |
| gmb | bac, I know why it happens, I think. Hang on. | 15:36 |
| rockstar | gmb, before you land it (and after bac reviews it) I'd love to take a whack at reviewing it as well. | 15:39 |
| gmb | bac, Pull and rebuild. Fixed it by a cunning s/console/Y/g | 15:39 |
| gmb | rockstar, Please do. I'm sure it's littered with horrors. | 15:39 |
| gmb | And things I've forgotten to add tests for. | 15:39 |
| * gmb hates doing the tests second. | 15:39 | |
| rockstar | gmb, yeah, but I think testing this kind of interaction is hard without huge things like windmill. | 15:39 |
| gmb | rockstar, Agreed. | 15:41 |
| bac | gmb: works brilliantly now | 15:43 |
| gmb | bac, Well, it works 100% better than it did, anyway :) | 15:44 |
| === matsubara is now known as matsubara-lunch | ||
| bac | gmb: your testing is very thorough and easy to follow | 16:58 |
| bac | gmb: i am confused by test_wizard_needs_steps | 17:00 |
| gmb | bac, How so? | 17:01 |
| bac | gmb: in it you seem to be instantiating with no steps, which is a no-no, but assert that it instaniated properly | 17:01 |
| === benji is now known as benji-lunch | ||
| bac | gmb: am i misreading it? | 17:01 |
| gmb | bac, Ah, JS testing FTFL. | 17:01 |
| gmb | bac, See the _should definitions | 17:02 |
| bac | yeah, i did but clearly nothing clicked | 17:02 |
| gmb | bac, test_wizard_needs_steps is understood to have passed if it raises an error. | 17:02 |
| gmb | bac, I'll add a comment to make this clear, but it is beyond horrible that we can't do Assert.throws() or some such. | 17:03 |
| bac | so the assert does fail | 17:03 |
| bac | and that failure is noted in the _should | 17:03 |
| bac | so by failing it passes | 17:04 |
| gmb | bac, Yes | 17:04 |
| * bac cries | 17:04 | |
| bac | yes, i knew that at one time | 17:04 |
| bac | but my brain purged it as it was jibberish | 17:04 |
| gmb | Yeah. | 17:04 |
| bac | gmb: in _renderUIWizard: you check to see that there are some steps | 17:07 |
| bac | gmb: is that necessary since you did the check at instantiation? | 17:08 |
| gmb | bac, Oops, no. I'll remove that. | 17:08 |
| === deryck is now known as deryck[lunch] | ||
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | gmb r=bac but i hope rockstar has time to have a look | 17:13 |
| rockstar | bac, I will MAKE time. | 17:14 |
| gmb | bac, ta | 17:14 |
| === adeuring changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === salgado is now known as salgado-lunch | ||
| gmb | bac, It should be noted FTR that rockstar did many of the changes for which you've given positive comments. :). However, sadly, I can't blame any problems on him. | 17:28 |
| rockstar | gmb, the fact that you pulled it from the railroad tracks before the train came should also be noted. | 17:28 |
| rockstar | gmb, where is this proposal? | 17:29 |
| gmb | rockstar, Oh yes, that might help you... https://code.launchpad.net/~gmb/lazr-js/wizard-widget/+merge/35827 | 17:29 |
| allenap | bac: Could you sanity check this please? http://paste.ubuntu.com/495399/ - it's a change suggested by lifeless in https://code.edge.launchpad.net/~allenap/launchpad/bugsubscription-to-storm/+merge/35537 | 17:39 |
| allenap | bac: Argh, I've got to go! Don't worry about it. Thanks anyway. | 17:41 |
| === matsubara-lunch is now known as matsubara | ||
| === benji-lunch is now known as benji | ||
| === salgado-lunch is now known as salgado | ||
| EdwinGrubbs | bac: I have a 950 line branch. Do you want to review it? It will still need a follow-up branch to add the cronjob before it lands. | 18:42 |
| bac | EdwinGrubbs: i'll be happy to review it | 19:03 |
| === bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | EdwinGrubbs let me know when you've submitted a MP | 19:07 |
| === bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing:- || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha is now known as Ursinha-lunch | ||
| EdwinGrubbs | bac: here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-queue-addmember-emails/+merge/35862 | 19:23 |
| === bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: Edwin || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | bac: I'm headed to lunch. | 19:40 |
| bac | EdwinGrubbs ok | 19:40 |
| bac | don't go to that place where they yell at you | 19:40 |
| EdwinGrubbs | I wonder how they would do as code reviewers. | 19:43 |
| === EdwinGrubbs is now known as Edwin-lunch | ||
| === Ursinha-lunch is now known as Ursinha | ||
| EdwinGrubbs | bac: thanks for the review. I'll try to get the reply done shortly. I tried going to Starbucks after lunch, but freenode had banned its IP address. Stupid caffeinated hackers. | 21:34 |
| bac | really? hadn't heard of that before | 21:34 |
| bac | EdwinGrubbs: i would suggest bip but it's a pita | 21:34 |
| sinzui | bac ping | 21:48 |
| bac | hi sinzui | 21:48 |
| === bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| sinzui | bac: I am preparing a migration branch for mailman. Will you have time to review it? | 21:48 |
| bac | sinzui: how big? | 21:49 |
| sinzui | 417, 30 lines are not mechanical--two tests were broken by recent changes | 21:50 |
| bac | i can definitely do it tonight | 21:50 |
| bac | sinzui: around 6 we're leaving but i'm can do it in the car and send it when we arrive | 21:51 |
| sinzui | it is very boring. I was just disappointed to see the tests were broken in july/August | 21:51 |
| bac | sinzui: they were broken and then disabled? | 21:52 |
| bac | sinzui: or are they only run on demand? | 21:52 |
| sinzui | latter --layer=Mailman and they only pass on repeated tries | 21:53 |
| bac | outta sight...gonna break | 21:53 |
| sinzui | I am so angry with these tests. One that is broken does not need to be on the mailman layer. | 21:54 |
| sinzui | I do not know where to begin fixing these tests | 21:54 |
| bac | so when do you think the branch will be ready? | 21:55 |
| sinzui | 5 minutes | 21:55 |
| bac | oh, sweet | 21:55 |
| sinzui | bac: I am still waiting for the diff to appear: https://code.edge.launchpad.net/~sinzui/launchpad/discard-list-spam-0/+merge/35885 | 22:03 |
| * bac makes my own diff | 22:03 | |
| bac | sinzui: wow! that was boring | 22:20 |
| bac | r=me | 22:20 |
| sinzui | running the tests is much more boring | 22:20 |
| sinzui | I need caffeine, Guinness, and maybe LSD to find a way to make this testable. | 22:21 |
| EdwinGrubbs | bac: do you know if there is a helper function like assert that raises ValueError> My single line asserts turned into 4-line if-statements. | 22:23 |
| EdwinGrubbs | s/>/?/ | 22:23 |
| bac | EdwinGrubbs: i don't know. | 22:24 |
| bac | EdwinGrubbs: it sounds like a good helper, though | 22:26 |
| bac | sorry for the clutter | 22:26 |
| EdwinGrubbs | sinzui: I was looking at volunteering with Junior Achievement. Apparently, it would take a couple hours a week for eight weeks. Is it ok if I shift my schedule on those days, and is it ok if I miss the standup on those days? | 22:32 |
| EdwinGrubbs | sinzui: oops, this should go in the lp-registy channel. | 22:32 |
| === matsubara is now known as matsubara-afk | ||
| === bac 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 | ||
| benji | EdwinGrubbs: I'd have to look at the style guide to see if it's allowed, but removing the newline after the "if" seems reasonable: "if not value_is_good: raise ValueError('you suck')" | 22:48 |
| EdwinGrubbs | benji: well, the main reason that the ValueError is really long is that I was making a very clear error message. I guess I really only need to include the bad value, since the variable name and type can be found in the source code. | 22:57 |
| benji | EdwinGrubbs: I suggest a message like "foo_var must be a positive integer, got %r instead" % (foo_var,) | 22:58 |
| EdwinGrubbs | providing that long error message over and over again seems like a perfect reason for a helper function, but providing the variable and the variable name either means adding a redundant looking arg, using vars(), or crawling up the stack. bleh. | 23:02 |
| * benji goes afk | 23:05 | |
| lifeless | benji: EdwinGrubbs: FWIW: practicality over purity. | 23:21 |
| lifeless | our style guide is a *guide* not a straightjacket. | 23:21 |
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
