| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| allenap | henninge: Fancy an easy one to start? | 09:47 |
|---|---|---|
| allenap | https://code.edge.launchpad.net/~allenap/launchpad/remove-propertycache-adapters-bug-628762/+merge/39249 | 09:47 |
| henninge | allenap: sure! ;) | 09:47 |
| allenap | henninge: Thanks :) | 09:47 |
| henninge | allenap: He! 872 lines ... | 09:47 |
| henninge | ;-) | 09:47 |
| allenap | henninge: It's nearly 900 lines, but almost all of it is search-n-replace. | 09:47 |
| allenap | henninge: The interesting files are propertycache.py and propertycache.txt. | 09:48 |
| henninge | allenap: np, I'll be fine | 09:48 |
| henninge | allenap: You are the native speaker but is the "in" not misplaced here: | 09:55 |
| henninge | 557+The property cache for an object can be cleared by passing in the | 09:55 |
| henninge | 558+object to `clear_property_cache()`. | 09:55 |
| henninge | "by passing the object into ..." | 09:55 |
| henninge | ? | 09:56 |
| allenap | henninge: I think both work, but your suggestion is better. | 09:56 |
| henninge | allenap: Also, I first thought this was the same test twice. | 09:56 |
| allenap | henninge: I think I can trim it to make it look a little different. | 09:57 |
| henninge | allenap: or just make it clearer in the narrative that this is an alternative use of the same function. | 09:57 |
| allenap | henninge: Okay. | 09:57 |
| henninge | Wow, the new code looks much simpler. Good job! | 10:00 |
| allenap | henninge: Cheers :) I should have done it this way from the start. Never mind, I learnt a lot... the hard way. | 10:01 |
| henninge | allenap: that really was an easy one. Thanks! r=me | 10:02 |
| allenap | henninge: Cool, thanks | 10:02 |
| bac | hi henninge | 11:44 |
| henninge | Hey bac! ;) | 11:44 |
| bac | henninge: hey we're in testfix mode and i wonder if you'd sanity check this patch: http://pastebin.ubuntu.com/519603/ | 11:45 |
| bac | normally i'd think the geo data has changed out from under us. but best i can tell it hasn't, as we're getting the GeoIP Lite data from our PPA | 11:45 |
| bac | henninge: so, do you think the approach here of accepting both until we sort it out is sane? | 11:46 |
| * henninge looks at the failure | 11:46 | |
| henninge | bac: oh yes, I think that is sane. | 11:47 |
| bac | ok, well i'll just land it rs=bac then and not implicate you | 11:47 |
| henninge | bac: I have never worked with geopip before. | 11:48 |
| bac | only minimal for me | 11:48 |
| henninge | but if you say "I know we may get one of these two values and we have yet to figure out why", I say your patch is a good way to get us going again. | 11:48 |
| henninge | bac: r=me in that case. | 11:49 |
| bac | henninge: that is what i'm saying | 11:50 |
| bac | thanks | 11:50 |
| bac | hi jtv. | 11:55 |
| jtv | hi bac! | 11:55 |
| jtv | happy end-of-buddhist-lent day | 11:55 |
| henninge | Hi jtv! | 12:09 |
| jtv | Hi henninge. I'm not here. | 12:10 |
| lifeless | henninge: https://code.edge.launchpad.net/~lifeless/launchpad/edge/+merge/39233 | 14:25 |
| * henninge looks | 14:25 | |
| henninge | 186-On launchpad.net, the version and revision numbers are presented only in an | 14:27 |
| henninge | 187-HTML comment. | 14:27 |
| henninge | lifeless: How is that handled now? Are they always visible? | 14:28 |
| henninge | I don't mind, just curious. | 14:28 |
| lifeless | they are in a <-- section | 14:28 |
| lifeless | hit ctrl-U on launchpad.net | 14:28 |
| lifeless | down the bottom | 14:29 |
| lifeless | but also I've put the revno on prod after chatting with mpt | 14:29 |
| lifeless | (by taking it outside a tal:condition) | 14:29 |
| henninge | Ok, I knew that. I just thought it had changed because the test is deleted. But now that I looked closer at it, the test is not about them being in a comment but about them *not* being displayed. | 14:31 |
| henninge | So that's ok. ;) | 14:31 |
| === adeuring1 is now known as adeuring | ||
| henninge | lifeless: thanks for removing the "bleeding edge" ... ;-) | 14:35 |
| lifeless | I haven't done a full test run yet, this may have some breakage, but shoud be minor | 14:35 |
| henninge | lifeless: this still mentions edge. Is that OK? | 14:36 |
| henninge | 921- - is_edge/is_lpnet etc (thunks through to the config) | 14:36 |
| henninge | 922+ - server.is_edge/is_lpnet etc (thunks through to the config) | 14:36 |
| lifeless | bah, I missed that; thanks | 14:37 |
| lifeless | it should be server.lpnet | 14:37 |
| lifeless | IIRC | 14:37 |
| henninge | lifeless: also, you seem to have removed all references to "staging" from the on-edge script but left edge in there. | 14:38 |
| henninge | I think that script needs a complete renaming now. | 14:39 |
| lifeless | henninge: I filed a bug about it | 14:40 |
| lifeless | uhm, I think you're reading the hunks wrongly | 14:40 |
| henninge | ah, cool ;) | 14:40 |
| lifeless | We should, i think, delete the script entirely | 14:40 |
| henninge | possibly | 14:40 |
| henninge | why am I reading them wrongly? | 14:40 |
| henninge | or rather "how" | 14:41 |
| lifeless | it still calls staging_revision = on_staging() | 14:41 |
| lifeless | its just that theres no longer a use case for 'just edge' | 14:41 |
| lifeless | because edge is approximately gone | 14:41 |
| lifeless | + staging_revision = get_staging_revision() | 14:42 |
| henninge | yes, I saw that but the command line option for staging is still there. | 14:42 |
| lifeless | - '--staging-only', action='store_true', | 14:43 |
| lifeless | 1022- help="Only show revisions not on staging. Do not consult edge.") | 14:43 |
| lifeless | its gone :) | 14:43 |
| henninge | 1023+ '--edge', action='store_true', | 14:43 |
| henninge | 1024+ help="Show revisions on edge.") | 14:43 |
| lifeless | thats for edge | 14:43 |
| henninge | While we still have it? | 14:43 |
| lifeless | right | 14:44 |
| henninge | ah! | 14:44 |
| lifeless | first we remove all the differences | 14:44 |
| lifeless | then we put a redirect in place | 14:44 |
| lifeless | and a that point we need qa stuff to move from edge to qastaging | 14:44 |
| lifeless | so theres future tweaks to do here | 14:44 |
| henninge | So, this script was just about easily figuring out how far you revision had propgated through the various branches. | 14:45 |
| lifeless | yeah | 14:45 |
| henninge | that may not be needed anymore now. | 14:45 |
| lifeless | so devel -> stable -> deploy qastaging -> qa'd -> lpnet | 14:45 |
| lifeless | and db-devel -> db-stable -> staging -> ??? -> monthly release | 14:45 |
| henninge | Speaking of which: I just created a draft for the 2011 release calendar (see your mail). We will still be having monthly releases, right? | 14:46 |
| lifeless | yes | 14:47 |
| lifeless | until we get db flexability in place | 14:47 |
| henninge | ok | 14:49 |
| henninge | lifeless: r=me | 14:49 |
| lifeless | thank you | 14:50 |
| allenap | henninge: Do you fancy another easy one? | 15:25 |
| henninge | allenap: please! ;) | 15:29 |
| allenap | henninge: https://code.edge.launchpad.net/~allenap/launchpad/ditch-get-bug-notifications-recipients-bug-659085-devel/+merge/39277 | 15:29 |
| allenap | henninge: It's big... at first glance. | 15:29 |
| allenap | henninge: But it's actually almost all reviewed. Just a little diff (in the description) to review. | 15:29 |
| === abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | allenap: I have never seen a doc test import from a unit test. Is that a good(tm) practice? | 15:36 |
| allenap | henninge: I don't think it's a problem, but it is a bit odd. Certainly BugTaskSearchBugsElsewhereTest.__init__() smells bad. | 15:38 |
| allenap | henninge: Arguable those helper methods should have been put in a mixin or just a separate class, but as it is it's reasonably easy to figure out what's going on. I don't think there's much harm in it. | 15:40 |
| henninge | Argh! Now I get what that is doing. | 15:40 |
| henninge | allenap: Luckily it's not part of this review ... ;-) | 15:41 |
| allenap | henninge: Yes, that's what I thought :) | 15:41 |
| henninge | allenap: could you call the result "naked_bugtasks" here? | 15:42 |
| henninge | + bugtasks = [removeSecurityProxy(bugtask) for bugtask in bugtasks] | 15:42 |
| allenap | henninge: Sure. | 15:42 |
| henninge | I think there was an agreement to clearly mark naked entities as such ... | 15:43 |
| henninge | allenap: r=me for the handy diff. ;-) | 15:44 |
| allenap | henninge: Thanks :) | 15:46 |
| rockstar | henninge, abentley, can I send a review to one of you? | 16:13 |
| henninge | rockstar: I am almost done. Sorry. | 16:14 |
| henninge | In fact, I am done ... ;) | 16:14 |
| rockstar | henninge, it's VERY short. :) | 16:14 |
| === henninge 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 | ||
| henninge | rockstar: sure | 16:14 |
| henninge | ;-) | 16:14 |
| henninge | short & fun - always! | 16:14 |
| leonardr | abentley, pretty easy branch for your consideration: https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-unicode-error/+merge/39279 | 16:18 |
| abentley | leonardr: Okay. | 16:19 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | leonardr: you have conflicts. | 16:20 |
| rockstar | abentley, can you also review my branch? | 16:20 |
| rockstar | abentley, https://code.edge.launchpad.net/~rockstar/launchpad/fix-queue-permissions/+merge/39278 plz. | 16:20 |
| abentley | rockstar: Sure. I thought henninge was looking at it. | 16:20 |
| henninge | rockstar, abentley: I am! | 16:20 |
| rockstar | henninge, oh! I thought you were being sarcastic. :) | 16:21 |
| henninge | rockstar: not my nature ... :) | 16:21 |
| leonardr | abentley: argh, coordinating with benji | 16:22 |
| === deryck_ is now known as deryck | ||
| abentley | leonardr: I am a bit confused by line 187; shouldn't that have a unicode escape, not \xc3\xa7? | 16:24 |
| henninge | rockstar: Have you started your own Canonical spin-off? A bit obvious, don't you think? | 16:25 |
| henninge | 136-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the | 16:25 |
| henninge | 137+# Copyright 2009-2010 aanonical Ltd. This software is licensed under the | 16:25 |
| henninge | ;-) | 16:25 |
| rockstar | henninge, huh. I'll fix that... | 16:25 |
| leonardr | abentley: no, because the test suite (i think, benji might know better) decodes the unicode string | 16:25 |
| abentley | leonardr: Did you consider making this a unit test? | 16:27 |
| benji | actually it's the other way around; it doesn't attempt to decode the string, so you just get the bytes | 16:27 |
| henninge | rockstar: r=me ;-) | 16:27 |
| rockstar | henninge, thank you sir! | 16:27 |
| abentley | benji: so that's a platform-specific value, then? | 16:28 |
| leonardr | abentley, that's a huge pain, but i'll try | 16:29 |
| abentley | benji: looks like utf-8 to me, not utf-16, which AIUI is the internal representation on Ubuntu python. | 16:30 |
| benji | abentley: theoretically it could vary, but practically shouldn't | 16:31 |
| abentley | benji: I don't think pretty sure it's the plain bytes of the unicode string, because on Ubuntu, the internal representation is utf-16, and that's utf-8. | 16:32 |
| abentley | benji: I think it's more likely that it was encoded as utf-8. | 16:32 |
| leonardr | if i'm able to make it a unit test i should also be able to control the encoding | 16:33 |
| allenap | salgado: Are you available to do UI reviews? It's not urgent. | 16:33 |
| abentley | leonardr: true, but as a unit test, you can just compare against a unicode string. | 16:33 |
| benji | abentley: I forget the exact details, but Python doesn't use utf-8 or -16 internally, it uses 16 or 32-bits per character (depending on how it was compiled) | 16:33 |
| salgado | allenap, not really, I'm attending some UDS sessions remotely while I wait for my flight to the US | 16:34 |
| allenap | salgado: Okay, thanks anyway. Have a good trip :) | 16:34 |
| salgado | thanks. :) | 16:35 |
| abentley | benji: In this bug report, Adam Olsen asserts that it uses utf-16, not UCS2: http://bugs.python.org/issue3297 | 16:36 |
| benji | abentley: yep I was thinking of UCS-2/UCS-4, but it does indeed use UTF-16/UTF-32 | 16:37 |
| abentley | leonardr: I'm not saying you have to use a unit test, but in Launchpad, the policy is that doctests should be testable documentation. | 16:41 |
| gary_poster | abentley: five-line-diff version-bump review when you have a moment: https://code.edge.launchpad.net/~gary/launchpad/storm-0.18/+merge/39286 | 16:48 |
| abentley | gary_poster: r=me | 16:49 |
| gary_poster | thanks abentley | 16:49 |
| === benji is now known as benji-lunch | ||
| leonardr | abentley: behold: http://pastebin.ubuntu.com/519760/ | 17:31 |
| abentley | leonardr: r=me. | 17:34 |
| === 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 | ||
| === Ursinha is now known as Ursinha-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| === salgado is now known as salgado-lunch | ||
| === benji-lunch is now known as benji | ||
| === matsubara-lunch is now known as matsubara | ||
| === Ursinha-lunch is now known as Ursinha | ||
| === matsubara is now known as matsubara-afk | ||
| leonardr | benji: i named launchpadlib 1.7.0 in anticipation of your kwallet branch. you don't need to call this branch 1.8.0 | 20:38 |
| leonardr | we usually do 'import cStringIO as StringIO' | 20:40 |
| benji | leonardr: (assuming that comment was for me) ewww :) | 20:45 |
| leonardr | benji: actually, we do 'from cStringIO import StringIO', if that makes you feel better | 20:46 |
| benji | much better | 20:46 |
| leonardr | benji, why do you have callback_called as a list instead of a boolean? i imagine it has something to do with the possibility that the callback might be called multiple times? | 20:49 |
| leonardr | but if so, wouldn't you check the list to make sure it was only called once? | 20:49 |
| benji | leonardr: nope, it's because I can't rebind callback_called, just mutate it | 20:50 |
| leonardr | ah, ok | 20:50 |
| benji | perhaps a note to that effect is warrented | 20:51 |
| leonardr | benji: the code is weird. bool([False]) is True | 20:51 |
| leonardr | i think you should check that the list is [True] | 20:51 |
| benji | mmm, yeah that sounds like a bug | 20:52 |
| leonardr | bool([]) is False, but your use of booleans _inside_ the list strongly implies that you are checking those values | 20:52 |
| leonardr | you could have the callback append object() to the list, and make sure the list was nonempty | 20:53 |
| benji | leonardr: oh, well it's not a bug per se; I'm signaling truth by there being something lin the list | 20:53 |
| benji | yeah, I'd say appending None would be the most straight-forward | 20:53 |
| leonardr | +1 | 20:53 |
| === Ursinha is now known as Ursinha-afk | ||
| leonardr | benji: can you run this code in bin/py on your kwallet branch and see if it works for you? i get an error but it might be a version mismatch | 21:05 |
| leonardr | >>> from launchpadlib.launchpad import Launchpad | 21:05 |
| leonardr | >>> f = Launchpad.login_with("bar baz", "edge") | 21:05 |
| benji | sure | 21:05 |
| benji | leonardr: I get this: TypeError: __init__() takes at most 5 arguments (6 given) | 21:06 |
| benji | I'll dig into it. | 21:06 |
| leonardr | ok, cool | 21:06 |
| leonardr | benji, review sent. i'm very glad that you were able to fix keyring so that we could use it | 21:10 |
| benji | leonardr: oh, I meant to mention that I was able to do away with the Qt widget bit (I still had to create a Qt "application" but that's not too bad) | 21:11 |
| leonardr | benji: and that's in keyring, not in our code, right? | 21:11 |
| benji | right | 21:11 |
| benji | I expect Tarek to make a release of keyring this week that includes my changes. | 21:12 |
| leonardr | great, where's it coming from now? | 21:12 |
| leonardr | ie. if i didn't get that TypeError would i be using your code? | 21:12 |
| leonardr | or what would happen? | 21:12 |
| leonardr | benji -^ | 21:18 |
| benji | leonardr: you'd be using the version before my changes (because that's what buildout gets from PyPI), which probably couldn't build the GNOME bits, and it probably couldn't build the encyption bits, so more than likely you'll just be using a file | 21:20 |
| leonardr | benji: ok, sometime tomorrow i'd like to get the whole thing running with your code so i can do an end-to-end test | 21:22 |
| benji | sounds good | 21:22 |
| jcsackett | abentley: have time for a review? | 21:23 |
| jcsackett | https://code.edge.launchpad.net/~jcsackett/launchpad/hidden-project-configuration-636000/+merge/39306 | 21:23 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | jcsackett: Prolly not. | 21:40 |
| jcsackett | abentley: fair. i wouldn't be able to get in to merge today anyway. | 21:41 |
| === jcsackett 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 | ||
| === Ursinha-afk is now known as Ursinha | ||
| === StevenK changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| StevenK | abentley: ^ If you have time | 22:27 |
| abentley | StevenK: Sorry, I've EODed. | 22:27 |
| === 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 | ||
| StevenK | No fair dropping me from the queue | 22:28 |
| === StevenK 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 | ||
| abentley | StevenK: It's an on-call review queue. No one has agreed to review you, so you can't be in the queue. | 22:29 |
| thumper | StevenK: paste the link | 22:32 |
| StevenK | thumper: https://code.edge.launchpad.net/~stevenk/launchpad/cronscript-idsjob-testfix/+merge/39321 | 22:33 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!