| bac | hi henninge | 08:27 |
|---|---|---|
| henninge | Hey bac! ;) | 08:28 |
| bac | henninge: so did you get UI graduated? | 08:28 |
| henninge | bac: so rockstar tells me. Is it his call alone? | 08:28 |
| bac | henninge: no, he talked to me and sinzui. i just wasn't sure. the discussion of it in the meeting notes last week was a little anti-climatic | 08:29 |
| bac | henninge: so, congratulations! | 08:29 |
| henninge | bac: Thank you! | 08:29 |
| bac | henninge: thanks to you. i've updated the wikis to show it. | 08:30 |
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | hi bac, do you feel like reviewing my branch? 24 lines of diff but a huge cover letter. :-) https://code.launchpad.net/~jtv/launchpad/bug-662552-defer-potmsgset-filter/+merge/39426 | 10:32 |
| bac | jtv: ok | 10:34 |
| jtv | thanks bac | 10:35 |
| bac | jtv: who is UCP? | 10:44 |
| jtv | Universitat Politècnica de Catalunya | 10:44 |
| jtv | Which I believe means Universidad Politécnica de Cataluña. | 10:45 |
| jtv | Great database professor there. | 10:45 |
| jtv | Made things really accessible, and got his team to implement a real-world innovation in postgres instead of messing about with theory and simulations. | 10:48 |
| bac | that's cool | 10:49 |
| bac | thanks for the branch and explanation. | 10:49 |
| jtv | thanks for the review—"mysterious" is about right. :-) | 10:51 |
| henninge | hey jtv, I am unsure atm: Will this code also return existing languages or throw an exception if the code exists? | 12:07 |
| henninge | http://paste.ubuntu.com/520708/ | 12:07 |
| henninge | from LangugeSet | 12:08 |
| jtv | henninge: well it constructs a Language. | 12:08 |
| jtv | So there's your answer. :) | 12:08 |
| henninge | jtv: you are not helping | 12:08 |
| jtv | It'll create a new Language object, and if one of the same code or englishname already existed, then you'll get a unique violation when it gets flushed to the database. | 12:09 |
| henninge | jtv: now you are helping ;-) | 12:09 |
| henninge | I suspected that but forgot... | 12:09 |
| * jtv regrets it already :) | 12:09 | |
| jtv | It's just a constructor. Constructors don't look for existing objects. | 12:09 |
| === jtv 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 | ||
| === matsubara-afk is now known as matsubara | ||
| === henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | jtv: It's a pitty you just eod'ed. You would have liked this one ... ;-) | 12:55 |
| jtv | ? | 12:56 |
| henninge | https://code.edge.launchpad.net/~henninge/launchpad/devel-productseries-views-tests/+merge/39437 | 12:56 |
| * jtv checks channel name | 12:56 | |
| henninge | But I will go to lunch myself now ... | 12:56 |
| jtv | yes, I just eod'ed. :) | 12:56 |
| henninge | :-P | 12:56 |
| lifeless | https://code.launchpad.net/~lifeless/launchpad/zope.testing/+merge/39423 | 12:59 |
| === jtv is now known as jtv-afk | ||
| jtv-afk | henninge: someone who probably isn't here right now just came by and spontaneously reviewed your branch | 14:17 |
| henninge | jtv-afk: that is awfully nice of him (or her?) ! | 14:31 |
| === henninge 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 | ||
| === adeuring1 is now known as adeuring | ||
| jcsackett | is someone available to do a ui review on https://code.edge.launchpad.net/~jcsackett/launchpad/hidden-project-configuration-636000/+merge/39306 | 15:12 |
| leonardr | as long as we're asking for reviews, i need a code review for https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/ (writing mp now) | 15:16 |
| leonardr | jcsackett, i'd swap reviews with you if i were rated for ui reviews | 15:16 |
| jcsackett | leonardr: i'd take you on that if i were a reviewer. :-P | 15:16 |
| leonardr | like two ships passing in the night | 15:17 |
| * jcsackett laughs. | 15:22 | |
| henninge | jcsackett: I'll take it. | 15:27 |
| jcsackett | henninge: thanks! | 15:27 |
| leonardr | salgado, maybe you want to take a minute and review https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 ? | 15:31 |
| henninge | rockstar: when you get up, can you please add me to launchpad-ui-reviewers? ;-) | 15:35 |
| === Ursinha is now known as Ursinha-afk | ||
| rockstar | henninge, ah! | 15:36 |
| henninge | Hi rockstar! ;) | 15:36 |
| rockstar | henninge, hello sir. | 15:36 |
| rockstar | henninge, done. | 15:37 |
| henninge | rockstar: great, thank you! Now I can claime that review ... | 15:38 |
| henninge | s/claime/claim/ | 15:39 |
| henninge | jcsackett: That looks very good, thank you! I just wonder if "Configuration links" is the best title. | 15:40 |
| henninge | jcsackett: how about "Configuration steps"? | 15:40 |
| jcsackett | not sure; that implies a sequence, and you don't really have to do any of them in sequence. | 15:40 |
| jcsackett | in all truth, we just really *want* people to set things up. if they don't care they don't have to do any of it. | 15:40 |
| henninge | "Configuration details" | 15:41 |
| jcsackett | henninge: yeah, i can go with that. | 15:41 |
| henninge | It's just that "links" are all over the place on a webpage | 15:41 |
| jcsackett | I was also thinking "Configure services" | 15:41 |
| henninge | Or that. | 15:41 |
| jcsackett | preference, oh ui reviewer? :-) | 15:41 |
| jcsackett | i'm good with either, personally. | 15:42 |
| jcsackett | does one seem better or more in keeping with convention? | 15:42 |
| henninge | what was it you copied from? | 15:43 |
| henninge | ah, "extra options" | 15:45 |
| henninge | May "Configuration options"? | 15:45 |
| jcsackett | that works. and that does seem to follow the convention. | 15:46 |
| jcsackett | i'll make that change push it up. thanks, henninge. | 15:46 |
| henninge | jcsackett: cool, let's use that. | 15:46 |
| henninge | jcsackett: there is something else | 15:46 |
| henninge | I just ran the branch and it looks different to the screenshot. | 15:46 |
| henninge | s/to/from/ ? | 15:46 |
| * jcsackett looks alarmed. | 15:46 | |
| henninge | There is the blank line missing after "Configuration links" | 15:47 |
| jcsackett | ah, yes. i killed the <br> from the code review and didn't update the screenshots. | 15:47 |
| jcsackett | sorry. | 15:47 |
| jcsackett | you prefer with the blank line? | 15:47 |
| jcsackett | i wasn't sure either way to go on that. | 15:47 |
| henninge | Actually, that was a bit too much. Could we have half a line? | 15:48 |
| * henninge hacks | 15:48 | |
| === Ursinha-afk is now known as Ursinha | ||
| jcsackett | henninge: i'm good with half a line, but i'll confess my css-fu is weak. | 15:50 |
| henninge | jcsackett: turn "legend" into a div and give it a "margin-bottom: 0.5em;" . That looks fine. | 15:54 |
| jcsackett | henninge: dig. will do. | 15:55 |
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| henninge | jcsackett: you will have to script your js code because this breaks it. | 15:56 |
| henninge | s/script/fix/ | 15:56 |
| jcsackett | henninge: yeah, i think that javascript should just be #legend not span#legend anyway. | 15:57 |
| henninge | jcsackett: or maybe just use a class? | 15:57 |
| jcsackett | henninge: your right. ".legend" it is. | 15:57 |
| henninge | jcsackett: call it ".configuration_optoins" | 15:58 |
| jcsackett | henninge: okay. | 15:58 |
| henninge | although ... | 15:59 |
| henninge | yeah, do that. I think it's better to give such specific classes specific names. | 16:00 |
| henninge | jcsackett: .config-options | 16:01 |
| henninge | last word, promised ... ;-) | 16:02 |
| henninge | r=me | 16:02 |
| jcsackett | thanks henninge. i'll make those changes. :-) | 16:02 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-lunch | ||
| === matsubara-lunch is now known as matsubara | ||
| === jcsackett|afk is now known as jcsackett | ||
| jtv | Edwin: free for a review? https://code.launchpad.net/~jtv/launchpad/bug-662552-get-tm-or-dummy/+merge/39467 | 20:01 |
| jtv | A lot of it is lint. | 20:03 |
| StevenK | jtv: I have a branch too, shall I add you to queue? | 20:10 |
| jtv | StevenK: yes please | 20:10 |
| === StevenK changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jtv, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === noodles775 changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jtv, StevenK, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Hi Edwin-afk, could you please review the following when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/627608-p3a-token-race/+merge/39469 | 20:20 |
| EdwinGrubbs | jtv: sorry, I didn't see your comment. I'll start reviewing it now. | 20:20 |
| jtv | EdwinGrubbs: cool, thanks | 20:20 |
| noodles775 | (one-line change with a new test) | 20:20 |
| EdwinGrubbs | noodles775: sure. I guess I'll look at that first then. | 20:21 |
| noodles775 | EdwinGrubbs: thanks! | 20:21 |
| EdwinGrubbs | noodles775: what happens if the scripts fails to run? For example, if it can't connect to the database but the next minute it runs fine. | 20:25 |
| noodles775 | EdwinGrubbs: ah, you mean if date_started *is* set, even though it didn't complete... good point. | 20:27 |
| noodles775 | I'll check script-activity and ensure we have a successful record. | 20:29 |
| noodles775 | EdwinGrubbs: OK, I need to update to ensure date_completed is not null in the query. I'll update the test, make sure it fails and fix. | 20:30 |
| leonardr | Edwin: pls ad https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447 to the queue? | 20:33 |
| noodles775 | EdwinGrubbs: actually, afaics, script activity is only ever recorded on success (IScriptActivitySet has only a recordSuccess method). | 20:33 |
| noodles775 | EdwinGrubbs: Assuming that's true, the current code is fine as is. | 20:33 |
| EdwinGrubbs | noodles775: ok. Is the htaccess file the only thing that this script updates, so you are completely dependent on the ArchiveAuthToken.date_created and the last_success.date_started running on machines whose clocks are in sync? | 20:40 |
| noodles775 | EdwinGrubbs: yes (actually the htpasswd). And again, that's a good point about needing to be in sync. Thinking. | 20:43 |
| mwhudson | oh eh, i just reviewed that branch :-o | 20:44 |
| noodles775 | Thanks mwhudson - do you know if the issue Edwin mentiones above could be a problem? (ie. the token is created on an app server, but the script activity will be on germanium). Can we trust the timestamps? | 20:46 |
| mwhudson | noodles775: i *think* i've asked is about this before, and we can rely on ntp running | 20:46 |
| mwhudson | noodles775: you might want to check though :-) | 20:46 |
| * noodles775 checks | 20:47 | |
| mwhudson | noodles775: there is also a chance that all times will be from the db | 20:47 |
| noodles775 | mwhudson: date_started and date_completed are required args to IScriptActivitySet.recordSuccess, so I don't think they will be set by the db. | 20:48 |
| mwhudson | yeah, indeed | 20:48 |
| mwhudson | noodles775: you _could_ probably pass UTC_NOW for date_completed | 20:49 |
| mwhudson | but we don't | 20:49 |
| noodles775 | mwhudson: yeah, and we're checking date_started here. | 20:49 |
| mwhudson | also true | 20:49 |
| mwhudson | noodles775: i guess a fudge would be to check all tokens created >= date_started - 1 | 20:50 |
| mwhudson | second | 20:50 |
| EdwinGrubbs | noodles775: I think mwhudson fudge factor is reasonable. I'll let you two decide on the final implementation and start reviewing something else. | 20:53 |
| noodles775 | EdwinGrubbs: so we can assume NTP is running (ie. minimal skew), but I'll add a 1-second.... | 20:54 |
| noodles775 | :) | 20:54 |
| noodles775 | Thankyou EdwinGrubbs. | 20:54 |
| StevenK | EdwinGrubbs: Are you still reviewing | 21:00 |
| StevenK | ? | 21:00 |
| EdwinGrubbs | StevenK: yes, I'll get to yours after jtv's. | 21:01 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jtv, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| StevenK | EdwinGrubbs: Excellent, thank you. I have two. :-) | 21:09 |
| === matsubara is now known as matsubara-afk | ||
| === Ursinha is now known as Ursinha-bbl | ||
| EdwinGrubbs | jtv: review sent | 21:52 |
| jtv | thx | 21:52 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK, leonardr, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | StevenK: which branch would you like me to review first. | 21:55 |
| StevenK | EdwinGrubbs: The qafail | 22:00 |
| jtv | EdwinGrubbs: I'm definitely eod, but just put up another one: https://code.edge.launchpad.net/~jtv/launchpad/bug-662552-view-permissions/+merge/39481 | 22:13 |
| EdwinGrubbs | jtv: I doubt that I will have time to get to it. | 22:14 |
| jtv | no worries | 22:14 |
| EdwinGrubbs | leonardr: wouldn't you want the OAuthAuthorizer.user_agent_params to include the oauth_consumer, especially if the application_name isn't set? | 22:48 |
| leonardr | OAuthAuthorizer: application_name defaults to oauth_consumer | 22:49 |
| EdwinGrubbs | ok, I missed that | 22:49 |
| leonardr | maybe we also want the oauth_consumer, i dunno. gary, maybe you can weigh in when you get a minute? (poss. in comments on https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/enhanced-consumer/+merge/39447) | 22:50 |
| gary_poster | looking | 22:52 |
| StevenK | EdwinGrubbs: Thank you for the review! | 22:53 |
| EdwinGrubbs | leonardr: it looks like you have a merge conflict in src/lazr/restfulclient/docs/operations.txt, although it will be trivial to fix. | 22:54 |
| leonardr | ok | 22:55 |
| gary_poster | argh must run | 22:57 |
| gary_poster | leonardr: I'm sorry, I have to run. Do you need me to check back in later, or is it alright? | 22:58 |
| leonardr | gary, it's ok | 22:58 |
| gary_poster | ok thanks | 22:58 |
| gary_poster | g'night | 22:58 |
| jcsackett | leonardr: i added support for qastaging to launchpadlib if you would like to take a look: https://code.edge.launchpad.net/~jcsackett/launchpadlib/add-qastaging-to-uris/+merge/39483 | 23:08 |
| jcsackett | having to manually throw in the url when qaing was bugging me. | 23:09 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | StevenK: I'll finish your other branch later tonight. | 23:11 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| StevenK | EdwinGrubbs: Excellent, thank you | 23:11 |
| leonardr | jcsackett, i'll take a look tomorrow. remind me if i forget | 23:51 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!