=== Ursinha-bbl is now known as Ursinha === Ursinha is now known as Ursinha-afk === lantash is now known as 36DAAN8QF [10:03] hi jtv [10:03] hi bac [10:03] jtv: so i was thinking about the goals stated in the ArchitectureGuide === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:03] one was new tests under 2 seconds [10:04] i knew the one i recently wrote for testing noindex,nofollow was really slow. turns out 42 seconds. [10:04] whoa! [10:04] jtv, so i just spent about 15 minutes reorganizing it and now it runs in under 11 seconds [10:04] good, right? [10:04] All because of the rendering? [10:05] but at what cost? [10:05] http://pastebin.ubuntu.com/512987/ [10:05] Yes, much better—though apparently still not enough [10:05] for one, i avoid the costly setup but putting everything in one test. so it is much faster but violates the idea that we only test one thing at a time [10:06] caching of some properties helps too [10:06] * jtv hates piling implicit state into test cases [10:07] yeah, so i think it is not as explicit or readable but four times faster [10:07] I don't think test_robots is so bad… it's still only a few lines, and very readable. [10:08] it isn't that it is unreadable but that it is doing too much [10:08] Oh crap, I need to get out before the rain [10:08] ok, well i'd like to chat some more later [10:08] funny the rain just hit here too [10:09] it's quite loud crashing on hundreds of tin roofs [10:09] Must be the same cloud. The test is not doing _that_ much. What if you made the implicit state explicit as local variables, then turned the actual testing into a data-driven loop? [10:10] i don't follow [10:10] if you need to go we can talk later [10:10] A dict could map "for this enum value, I expect the noindex bit to present: {True, False} [10:10] " === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:11] Then test for the various enum values in a loop. [10:11] Figure out the url etc. once at the head of test_robots, and keep in local variables. [10:11] Pass those as explicit parameters to your assertion helper. [10:12] Then test_robots will consist of 3 parts: [10:12] * Setup [10:12] * Definition of inputs and expected outcomes [10:12] * Loop verifying outcome for each input. [10:12] * jtv checks cloud cover [10:13] yes, I can still just make that === jtv is now known as jtv-afk [11:15] gmb: Would you be able to review a branch for me? It's a teensy bit long, but I've added a guide to the changes, and it's mostly moves and deletions anyway. https://code.edge.launchpad.net/~allenap/launchpad/ditch-get-bug-notifications-recipients-bug-659085/+merge/38325 [11:16] allenap: Sure. Tell you what; I'm just writing a cover letter for my branch now. Let's swap :) === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:16] gmb: Perfect :) === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jelmer || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:20] allenap: https://code.edge.launchpad.net/~gmb/launchpad/make-mailnotification-care-bug-656759/+merge/38404, FTR. [11:20] allenap: I like your definition of "A teensy bit." [11:21] jelmer: What signal handler does Python install by default? signal.getsignal(SIGPIPE) return SIG_IGN normally, and I can't find a reference to new signal handlers in subprocess? (I'm just interested, not questioning.) [11:21] gmb: :) [11:25] allenap: Hi! [11:25] jelmer: Hi :) [11:25] allenap: Yep, but it needs to be set to SIG_DFL rather than SIG_IGN. [11:26] Colin does a much better job at explaining it than I can in this blogpost : http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html [11:26] jelmer: Oh cool, thanks. [11:37] allenap: r=me [11:37] gmb: Woohoo, thank you! I'm just about to start yours, just been finishing off Jelmer's. [11:37] Cool === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:50] allenap on line 76 of the diff I've got BugNotificationLevel.COMMENTS as a default value for subscribe(level=). I've pushed a fix for the just now. [11:51] gmb: Spooky, I was just looking at that exact thing. What should it be? [11:54] allenap It should be level=None [11:55] (It's bad form to pass values other than None as defaults, since they're then references to the original, which can cause things to blow up) [11:55] foo=[] is the canonical example of how not to do it. [11:57] gmb: Ah, I was looking at something else then (I didn't check the line numbers). However, in the branch of mine you just reviewed the level defaults to NOTHING. Should we choose the same default? I don't know which one makes more sense. [11:57] allenap: Let me look again at your branch... [11:58] Oh, arse. [11:58] allenap: So, you're right. [11:59] The default for subscribe should be COMMENTS (i.e. subscribe to *everything*) but the default for get*Subscribers() should be NOTHING, as you say. [11:59] I'll update it. [11:59] Nice catch! [11:59] allenap: Thanks for the review and useful feedback. Your first comment has me confused though - since I'm calling assertContentEqual, is it still necessary to sort the lists I'm comparing? [12:00] jelmer: Ah, I've never used assertContentEqual. I'll take a look... [12:00] jelmer: Ah, no, that does it for you. Ignore [1] then. [12:01] gmb: I have to go out now, but I'll finish the review as soon as I'm back. [12:02] allenap: Ok. [12:27] gmb: got a review for you, just cleanups really, but close to the 800-line limit. Would that be okay? === jtv-afk is now known as jtv [12:58] jtv: I'm not on-call today, allenap is. [12:58] jtv: If he doesn't have time to look at it I'll try and take a peek later. [12:59] gmb: ah, sorry, this client makes the topic too hard to read [12:59] np [12:59] allenap: got time for a branch near the 800-line limit? It's all cleanups, so nothing particularly hard. [13:37] jtv: Sure :) === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:39] allenap: thanks—I just pushed it slightly over the limit though: https://code.edge.launchpad.net/~jtv/launchpad/recife-pre-sharing-permissions/+merge/38407 [13:41] Hi, I need a review: https://code.launchpad.net/~malaria/launchpad/fixes-bug-608631/+merge/37996 [13:42] \topic On call: allenap || Reviewing: gmb || queue: [jtv, Malaria] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:43] Sorry === matsubara-afk is now known as matsubara === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [jtv, Malaria] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:43] Malaria: Okay, I'll get to it after jtv. [13:44] allenap: ok [13:44] allenap: can I add to your queue pls? [13:44] bigjools: Sure, I can probably get to it. === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [jtv, Malaria, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:44] allenap: it's small, cheers [13:44] https://code.edge.launchpad.net/~julian-edwards/launchpad/slow-ppa-search-bug-659129/+merge/38307 [14:29] allenap: I have to be off! === Ursinha-afk is now known as Ursinha [14:39] allenap: how stuffed do you feel your queue is? room for another? === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue: [Malaria, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:40] jcsackett: Mmm, I don't think I'll get to it to be honest. I have to finish a bit earlier than usual today. [14:41] dig, i'll hunt down someone else. thanks allenap. [14:41] jcsackett: Sorry about that :-/ [14:41] allenap: no worries, i understand. === salgado is now known as salgado-dr [14:48] allenap: Ta for the review. [14:48] gmb: Pleasure :) [14:51] gmb: Sorry it took so long; I have been interrupt driven all afternoon. [14:55] allenap: No worries. === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: Malaria || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:27] allenap, i'd like to add https://code.edge.launchpad.net/~leonardr/launchpad/automatically-calculate-request-token-expire-time/+merge/38423 to the review queue [15:28] leonardr: I don't think I'll get to it today. [15:28] ok, np [15:28] salgado, maybe you want to take it? [15:28] leonardr: Sorry. rockstar is also down as OCR today, so maybe he's up for it too. [15:29] cool === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: [bigjools] || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === allenap 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 [15:41] benji, why did you mess around with qt widgets rather than using the keyring library? [15:43] leonardr: if you mean http://pypi.python.org/pypi/keyring, I tried to use it but it ended up just having too many issues [15:43] hmmm [15:43] can we talk about the issues? do you remember? [15:44] the qt widgets look like the result of dealing with some issues as well [15:45] and even if you can't use the keyring lib, i'd like to see kwallet/gnome keyring/disk hidden behind a uniform keyring lib-style interface [15:46] sure, let's see: the package is hard to use for people using easyinstall or buildout because it sniffs for the various build dependencies and enables/disables the various interfaces (Gnome, KDE, crypto support for simple files) without any feedback so you have to scower the code to figure out what its dependencies actually are [15:46] once you do that it's not obvious how the things its sniffing for map to packages you can actually install [15:47] ok... [15:48] and using the deb packages doesn't help because the one that supports KWallet doesn't actually work: https://bugs.edge.launchpad.net/ubuntu/+source/python-keyring/+bug/657070 [15:48] <_mup_> Bug #657070: python-keyring-kwallet generates IOError

[15:49] since kde doesn't use dbus, i guess keyring_impl got set to gnome by mistake? [15:50] ok, so keyring has problems [15:50] Can you elaborate on "kwallet/gnome keyring/disk hidden behind a uniform keyring lib-style interface"? I don't think user of launchpadlib currently has to worry about the different storage options. [15:51] the user doesn't have to, but you've got a lot of conditionals in the code [15:51] try gnome, then try kde [15:51] i'd like to see that stuff localized to one place [15:51] keyring has problems, but we're in a good position to have those problems taken seriously [15:52] we've got a feature that a lot of people want [15:52] ok, let me refactor it a bit [15:52] benji, i've got a call with gary, let's pick this up afterwards. i might end up saying 'let's fix keyring' and then your refactoring would be for naught [15:53] leonardr: ok; you may want to discuss the keyring stuff with Gary, he was heavily involved in the decision to ditch it [15:53] yeah [16:05] benji: ok, i've talked to gary briefly [16:05] * gary_poster is on call. I think we need to see if we can convince leonardr, benji, as a validation. I also think we need to have doc/bug(s) (I know you have one already). That said, my underlying concern is that the underlying implementation decisions of keyring (using the -dev) seem problematic [16:06] so fixing is questionable to me [16:06] but we need to be prepared for concerns like these [16:06] which are reasonable === james_w` is now known as james_w [16:08] thanks allenap [16:13] benji: having trouble giving detailed thoughts, so let me share a couple gut reactions [16:13] 1. i believe you that the keyring library has problems, but it works for the people who wrote it, and i don't want to recreate that situation ourselves [16:14] (ie. launchpadlib works for us but is difficult for others to use) [16:14] i am ok with the gnome keyring code in your earlier branch, but i am squicked by the widget code in the kwallet branch, even though i assume that's the only way to get access to the kde wallet, and keyring probably does something similar [16:16] (that was 2) [16:16] 3. we don't have to land this code right away. i don't want to do a launchpadlib release with this code until we do desktop-wide integration by default [16:16] so that the cleanup code can just look in .launchpadlib, and won't also have to scan through the wallet for per-application keys [16:17] 4. if you haven't already, you should file bugs about the setup problems with the keyring lib [16:17] 1) Not wanting to reinvent the wheel is certainly a good impulse. I would argue that the keyring implementor did just that though; instead of using the existing Python interfaces to the Gnome keyring and KDE wallet they used their C and C++ (!?) interfaces instead to build their own Python interfaces [16:18] i agree that that's dumb, and possibly worthy of another bug (since it introduces dependencies most people don't have) [16:19] 2) not being a QT expert I can't be certain if there is a way to avoid the widget or not. I tried pretty hard to eliminate it, but KWallet seems pretty intent on having one to talk to. [16:19] 3) ok [16:19] so, maybe we should bring in a QT expert [16:20] 4) can do; I didn't bother because after talking to the mentor of the Google Summer of Code student that implemented the library I realized that he wouldn't see it as a bug, they decided to do it that was as a learning excersize for the student [16:20] :-/ [16:21] * gary_poster tallies another GSoC -1 :-/ [16:21] well, the student has learned, so now we can improve it? [16:21] I'd be all for improving it, but convincing them of that may be a challenge. [16:22] My version of "improving" it would be to throw most of it away. [16:23] ok, the other route goes in the direction of making our own competitor to the keyring library [16:24] I wouldn't to go that far. Not every bit of code has to be an external library. [16:25] it's still a competitor, even if we're the only ones who use it [16:25] and if it's better than alternatives, people may start using it--some people use beautiful soup just for the unicode conversion sub-library [16:25] so, a plan is taking shape in my mind [16:26] [16:27] let's talk to the developers i've talked to in the past: didrocks for ubuntu and riddel for kubuntu [16:27] riddell, i mean [16:27] riddell can give you an expert opinion on interacting with the kde wallet. talk to him first. see if the widget is necessary and if you wrote it correctly [16:28] also get a sense of whether the kubuntu developers would be happy with the keyring library as it is, if they would like the same changes we'd like, or if they want some other changes that would change the way both we and the keyring developers do things [16:29] you can show your gnome branch to didrocks but i'm pretty sure it's correct. talk to him about the keyring library [16:30] this seems like a lot of effort for a feature that was an afterthought [16:30] it's only an afterthought because you and i use ubuntu instead of kubuntu [16:30] one way this could go is that we land your gnome code and tell the kubuntu developers to give us something for kde [16:31] how about this. show your kde branch to riddell so we can have an expert evaluate your widget code. don't bother asking him about the keyring library [16:32] prepare a section in the launchpadlib TODO or something, 'why we're not using keyring' [16:32] refactor the code so that the "try gnome/try kde/try disk" loop is isolated to one place [16:32] we'll go with that for now [16:34] ok [16:34] but, i think "why we're not using keyring" will be more convincing if you could say "both kde and gnome developers we talked to agree with us on this" [16:35] that would also give us a starting point for either improving or competing with keyring, should we decide to do so in the future [16:36] i don't see any fundamental reason why we can't all get along [16:51] benji, https://code.edge.launchpad.net/~benji/launchpadlib/kwallet/+merge/38366 [16:55] benji, i'll talk to riddell about reviewing your widget code [16:55] ok === deryck is now known as deryck[lunch] === matsubara is now known as matsubara-lunch === benji is now known as benji-lunch [17:44] benji, if you want to, you could do a branch that just upgrades setup.py [17:44] that could land immediately === matsubara-lunch is now known as matsubara [18:30] leonardr: (I think you mean bootstrap.py) I'll do that === deryck[lunch] is now known as deryck === salgado-dr is now known as salgado === benji-lunch is now known as benji [18:36] salgado: ping [18:40] hi jcsackett [18:40] salgado: regarding the ui review i sent your way--i saw that you were needing reviews in the reviewer meeting, but i thought i would check that you are able to hit them right now. [18:42] jcsackett, do you need it done right now? I'm have some catching up to do from the two holidays I had earlier this week, but I should be able to do it later today or tomorrow if that's ok with you [18:43] salgado: that's just fine. just wanted to make sure i had read it right that you still wanted them sent your way. [18:43] I do, definitely! [18:43] excellent. i'll probably have another (small one) to send your way tomorrow. :-P [18:49] cool. :) [20:08] rockstar, i heard a rumor you were ocr today? [20:20] leonardr, yeah, I am... === rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [20:20] leonardr, where's your branch merge proposal? === sinzui changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:03] rockstar: https://code.edge.launchpad.net/~leonardr/launchpad/automatically-calculate-request-token-expire-time [21:03] rockstar, can you review https://code.edge.launchpad.net/~sinzui/launchpad/obsolete-add-edit/+merge/38377 ? I am not is a hurray since I am not working today [21:03] sinzui, okay. I'll look at it after leonardr's branch. === rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:33] rockstar: think you could do a review for me today? [21:33] jcsackett, yup. Hop on the queue. === jcsackett changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:34] thanks, rockstar. MP is here: https://code.edge.launchpad.net/~jcsackett/launchpad/branch-collection-links-652126/+merge/38456 [21:45] leonardr, review sent. r=me with some changes requested. [21:46] rockstar, thanks [21:47] rockstar: hi [21:47] Hi jelmer [21:47] rockstar: can I add another branch to your queue? [21:48] jelmer, sure! [21:48] rockstar: Great! https://code.edge.launchpad.net/~jelmer/launchpad/135610-duplicated-ancestry/+merge/38444 [21:58] jcsackett, if you want to land this today, I can do a ui review for you, and then you don't need two ui reviews [21:59] jcsackett, the code looks fine. [22:00] jcsackett, as far as UI though, I think you should put the commit count, branch count, and user count on the same line. [22:00] Active reviews should be its own little section. [22:01] rockstar: i tried that, but you end up with either a really weird block of text, or it looks like the branches and users *also* occured in the commit time period. [22:01] rockstar: you think move active reviews back into its own side thing? [22:01] jcsackett, X branches, owned by X people. There were X commits in the last X period. [22:02] jcsackett, the intention of the X commits in the last X period is to show how active the project is. [22:02] rockstar: yeah, that just looked wierd to me, but i'll deal with the text block instead of dividing it up as it is. [22:02] It doesn't do a good job, but I think laying it out that way would be better. [22:02] rockstar: regarding active reviews; side thingy, or just a separate line? [22:03] Active reviews need to be obvious. Putting text near it makes it hard to identify. [22:03] jcsackett, separate line should be fine. [22:03] rockstar: dig. [22:03] i'll make those changes. [22:03] jcsackett, for bonus points, it doesn't show if there are now active reviews. [22:03] rockstar: i'll look into it. didn't want this growing beyond the scope of bridging the gap, but if it's easy to throw in (and it should be) i'll hit it. [22:04] jcsackett, yeah. UI reviews often cause feature creep. Truthfully though, I think it stays in line with the original purpose of the bug. [22:04] rockstar: dig. [22:05] i'll switch that up and then ping you when i've made the changes, rockstar. [22:05] jcsackett, great, thanks. [22:38] rockstar: i've updated. you can see the new looks here: http://people.canonical.com/~jc/images/noreviews.png and http://people.canonical.com/~jc/images/withreviews.png [22:39] jcsackett, MUCH better. [22:41] yeah, thanks for the suggestions. :-) === matsubara is now known as matsubara-afk [23:00] thanks for the review, rockstar! === rockstar 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 === _thumper_ is now known as thumper