| === abentley1 is now known as abentley | ||
| stub | bac: https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/16486 (cherry pick candidate addressing critical bug) | 03:08 |
|---|---|---|
| jtv | hi bac! is that topic line still current? | 03:17 |
| mwhudson | i doubt it | 03:20 |
| mwhudson | stub: how did that librarian-gc bug happen? | 03:23 |
| mwhudson | jtv: i can do a review for you if you want | 03:24 |
| jtv | mwhudson: very kind, but only asking | 03:24 |
| jtv | don't need anything reviewed | 03:24 |
| stub | It died with abel's recent refactoring (dropping LibraryFileContent.deleted, replacing with a NULLable LibraryFileAlias.content) | 03:25 |
| mwhudson | stub: ah ok | 03:26 |
| stub | Previously we just set the LibraryFIleContent.deleted for expired files if there are no other unexpired references to that content. That code was removed, and not replaced. | 03:27 |
| mwhudson | stub: would it be possible to write that query using storm's query builder? | 03:29 |
| === stub1 is now known as stub | ||
| mwhudson | stub: would it be possible to write that query using storm's query builder? | 03:30 |
| mwhudson | stub: also, i think a comment in the tests about the timescale of "recent" would be nice | 03:31 |
| stub | Yes, but I doubt it would be more readable and the rest of the file uses raw SQL | 03:31 |
| mwhudson | ok | 03:31 |
| stub | I didn't want to infect the tests with definitions for recent etc. because that can change | 03:32 |
| mwhudson | well | 03:32 |
| mwhudson | when i read the test i had no idea if a day ago was recent | 03:32 |
| stub | I suspect we will be turning it down from 1 week to 1 or 2 days shortly based on elmo's comments maintaining debian archives | 03:33 |
| stub | ok | 03:33 |
| mwhudson | i guess it would be better if both the code and the tests keyed off a config value | 03:34 |
| stub | I'll add comments to that effect (1 day old is certainly recent, 30 days is certainly well expired, and values in between depend on the current settings in the garbage collector but are not really that important) | 03:34 |
| mwhudson | stub: that would be great, thanks | 03:34 |
| stub | Yer - nobody ever gets around to adding it as a config value ;) | 03:34 |
| stub | This is a cherry pick so I have an excuse this time ;) | 03:34 |
| mwhudson | well, there's no time like the present! | 03:35 |
| mwhudson | heh, darn, foiled | 03:35 |
| * mwhudson reviews in the webapp | 03:37 | |
| stub | Ahh.... I should use self.recent_past actually | 03:46 |
| stub | And add the comment there. | 03:46 |
| mwhudson | heh | 03:46 |
| mwhudson | yes please :) | 03:46 |
| stub | https://code.edge.launchpad.net/~stub/launchpad/librarian-report/+merge/16522 | 04:40 |
| stub | (not at all urgent) | 04:40 |
| stub | (but short) | 04:40 |
| === stub changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| stub | jtv: https://code.edge.launchpad.net/~stub/launchpad/librarian-report/+merge/16522 still needs its bureaucracy dealt with. | 10:38 |
| jtv | stub: oh, I thought bac approved that one? | 10:38 |
| jtv | stub: I still hate you for using slashes in dates, but given that it's a matter of "whatever the db accepts"... | 10:39 |
| stub | click the button and stop wining like a bitch | 10:40 |
| jtv | stub: I already did, nigga | 10:40 |
| stub | Thank you my good man. | 10:42 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jpds | https://code.edge.launchpad.net/~jpds/launchpad/fix_489363/+merge/16531 - is good to go. :) | 11:30 |
| jpds | jtv: Maybe you could take a look?^ :) | 11:54 |
| jtv | jpds: I'm OCR today, so that sounds reasonable. :-) | 11:54 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: jpds || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | jpds, a pattern I've found massively useful to know about: when you find yourself retrieving the current time, isolate it in a method that your test can override. | 11:57 |
| jtv | jpds: for instance, I think your test will fail after december 31st, 9999 and you can avoid that by dictating the time. | 11:57 |
| jtv | (hurry!) | 11:57 |
| jtv | jpds: I'd suggest two separate tests—one with the real time to ensure that logging doesn't break due to differences between "naïve" and "timestamp-aware" timestamps and such, but doesn't really care about the output; and another that fakes the time and looks for an exact match. | 11:58 |
| henninge | jtv: fancy a review of the intltool detection code? | 12:00 |
| === henninge changed the topic of #launchpad-reviews to: on call: jtv || reviewing: jpds || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | henninge: cool! | 12:00 |
| henninge | jtv: actually, the biggest part is translation domain extraction now ... | 12:00 |
| jtv | jpds: I do like your logging mixin, and it makes it very easy to get this testing right | 12:00 |
| jtv | jpds: taking a step back though, why not the standard logging module? The log files from our scripts do have timestamps. | 12:02 |
| jpds | jtv: Those are interesting senarios, will look into them. | 12:04 |
| jpds | jtv: Which module is that? | 12:04 |
| jtv | jpds: "import logging" | 12:04 |
| jtv | I'm sure it has a facility somewhere for directing to a file as well. | 12:04 |
| jtv | it seems the sort of wheel you're doomed to reinvent here | 12:04 |
| jpds | jtv: Hmm, well, the changes I've made are the ones sinzui suggested. | 12:05 |
| jtv | jpds: then I defer to his wisdom... he's not the sort to miss a thing like that | 12:05 |
| jtv | jpds: I'd definitely try to move log_file into LoggingMixin entirely, so ArchiveMirrorProberCallbacks doesn't need to know about it; and make the test just pass a StringIO in its place. | 12:06 |
| jtv | Then afaics you don't need to create that fake logging class for your test | 12:08 |
| jpds | jtv: OK, I'll look into that. | 12:16 |
| jtv | jpds: meanwhile I'll go have my team standup :-) | 12:17 |
| allenap | jtv: Oops, I just remembered I'm OCR today too. | 12:28 |
| bac | hi henninge | 12:28 |
| henninge | bac: in call atm | 12:28 |
| jtv | allenap: the more the merrier... I can start work on henning's branch soon though | 12:29 |
| bac | henninge: ok. i looked at your 'deactivate a user' branch yesterday but it still had test failures | 12:29 |
| allenap | jtv: Okay, cool. I think it makes more sense for you to do his branch in any case. | 12:30 |
| jtv | y | 12:30 |
| henninge | bac: Hi! | 12:34 |
| henninge | bac: yes, I am aware of that. | 12:34 |
| jtv | jtv หิว | 12:34 |
| === jtv is now known as jtv-eat | ||
| henninge | bac: I had meant to pass the branch on to intellectronica but never got around to it. | 12:35 |
| henninge | bac: We just agreed that I will fix and get this done today, so it can land. | 12:36 |
| bac | henninge: ok, cool. if you need me to look at it tomorrow i'd be happy to. | 12:36 |
| bac | (i'm off today) | 12:36 |
| henninge | bac: great! I feared I might not get a reviewer tomorrow ... ;) Although, we may not have much overlap since I am off in the afternoon. | 12:37 |
| bac | henninge: i can review it and if it needs no revisions could even land it for you | 12:38 |
| henninge | bac: cool, thanks. | 12:38 |
| === jtv-eat is now known as jtv | ||
| jtv | henninge: I find check_potfiles_in a bit hard to read... why not: | 13:16 |
| jtv | command = ( | 13:16 |
| jtv | "cd '%s' %% rm -f missing notexist && intltool-update -m" % path) | 13:16 |
| jtv | ? | 13:16 |
| jtv | (Although with variables on command lines in gnu systems it's usually a good habit to add a "--" to avoid accidental injection of command-line options) | 13:17 |
| jtv | Speaking of which, I suppose path needs escaping. | 13:17 |
| jtv | henninge: did you get my note about check_potfiles earlier? | 13:41 |
| jtv | jpds: any progress? | 13:41 |
| jpds | jtv: Sorry, popped out for lunch. | 14:04 |
| jtv | jpds: I'm asking since I won't be sticking around for too much longer | 14:04 |
| jpds | jtv: So far I have http://paste.ubuntu.com/345345/ - now I *think* I have to create something like DummyProductReleaseFinder in lib/lp/registry/tests/test_prf_finder.py / | 14:06 |
| jtv | jpds: the method looks fine... why the DummyProductReleaseFinder? | 14:07 |
| jpds | jtv: So I can make _getTime return a predetermined datetime? | 14:08 |
| jtv | jpds: in the tests? You can just re-assign the method as if it were a member variable. | 14:08 |
| jpds | jtv: That's what I meant, like: http://pastebin.ubuntu.com/345349/ | 14:09 |
| jtv | jpds: why not instantiate a normal LoggingMixin in the test (no special class) and insert a fake _getTime there? | 14:10 |
| jtv | jpds: same for setting log_file (and btw I don't see why the logfile variable is needed there) | 14:10 |
| jpds | jtv: special class> Can you give me an example? | 14:19 |
| jpds | jtv: logfile> Copied that from what Curtis suggested. | 14:19 |
| jtv | jpds: something like prober = LogginMixin() ; prober.log_file = StringIO() ; prober._getTime = fake_gettime" | 14:20 |
| jtv | ...where fake_gettime is a function that you create in your tests, which returns a fixed datetime. | 14:21 |
| jtv | jpds: as mentioned, it'd be nice if the log_file could be passed to the LoggingMixin as a constructor argument—that'd also remove ugliness from the test. | 14:23 |
| henninge | jtv: I am back | 14:30 |
| henninge | jtv: saw your note, let me look at it | 14:30 |
| jtv | henninge: I'm working on a reply in the MP right now | 14:30 |
| henninge | jtv: I copied that call to intltool-update from danilo's damned lies code and he was using dicts with format strings all the time. | 14:33 |
| jtv | henninge: it can be a nice thing to do, but in this case in _my_ opinion it harms readability more than it helps. | 14:34 |
| henninge | jtv: but I have no problem writing it the way you suggested, except for the single vs. double quotes. I don't think they are as equal as they are in python. | 14:34 |
| jtv | jpds: note fake_gettime is private, so better start its name with an underscore | 14:34 |
| jtv | henninge: no they aren't... double-quotes allow the variable to pull a lot more tricks. That's why I usually use single quotes in shell scripts. | 14:35 |
| henninge | I see | 14:35 |
| jtv | but ultimately the only way to make it safe is to escape the variable | 14:35 |
| jpds | jtv: Correct fake_gettime. | 14:38 |
| jpds | corrected* | 14:38 |
| jtv | jpds: what was the problem? | 14:38 |
| jtv | you said something about it getting overridden? | 14:39 |
| jpds | jtv: Oh, just the underscore thing. | 14:39 |
| jtv | ah | 14:39 |
| henninge | jtv: hm, do what do we have to do the escaping. I am a bit stumped atm. | 14:39 |
| jtv | henninge: good question :/ | 14:40 |
| jtv | I'm running out of time here, but you could google for shell escaping in python | 14:40 |
| henninge | jtv: that's fine. | 14:41 |
| jtv | or alternatively, prove to me that what's in that variable isn't going to be a problem in our use case :-) | 14:41 |
| henninge | jtv: it isn't because the paths are provided from another function that gets them from the file system. | 14:42 |
| henninge | (that sentence should probably be using singulars to be clearer) | 14:42 |
| jtv | henninge: perhaps... I don't see how it implies that the string is safe | 14:43 |
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: jpds, - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| henninge | jtv: because it is not entered by a user but derived from a system call. | 14:44 |
| henninge | jtv: the top-level function only works on the current directory, no chance for someone to get in a malicious parameter. | 14:45 |
| jtv | jpds: in your test, you could now verify that the log message is what you expect using a simple self.assertEqual(expected_value, actual_value) | 14:45 |
| jpds | jtv: Ah, I just took a self.failUnlessEqual() and am running the tests now. :) | 14:46 |
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: jpds, - || queue [henninge, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | henninge: the path comes from a parameter... I don't see anything here that tells us what that parameter might be | 14:47 |
| jtv | jpds: that's fine too | 14:47 |
| henninge | jtv: you were talking about our particular use case ... | 14:48 |
| jtv | henninge: yes, but I still don't know why a package couldn't have a directory "\" ; rm -rf / ; echo \"" | 14:49 |
| jpds | jtv: The test passes. :) | 14:53 |
| jtv | jpds: \o/ | 14:53 |
| === salgado is now known as salgado-lunch | ||
| jtv | now, with these tricks you sometimes get subtle little type mismatches the fake you injected isn't quite compatible with what the real call would have given you. So you'll also want a test without the fake, and without testing the output in detail, just to make sure using a real time doesn't break. | 14:54 |
| * jtv left out a word there | 14:54 | |
| jtv | you sometimes get subtle little type mismatches _where_ the fake you injected isn't quite compatible | 14:55 |
| sinzui | Was the word "waffle"? | 14:55 |
| jpds | jtv: OK, and what should I do about the 31/12/9999 thing? | 14:56 |
| jtv | jpds: I don't serious think we need to worry about the Y10K problem just yet. :-) In fact, that second test should be very lightweight—it really just needs to break when the datetime trick you pulled in your current test is wrong. | 14:57 |
| jtv | *seriously | 14:58 |
| * jtv must be very tired | 14:58 | |
| sinzui | jpds: The date should be formatted at 2011-01-31 | 15:03 |
| sinzui | dates are bigendian in reality | 15:04 |
| allenap | sinzui: Are you reviewing https://code.edge.launchpad.net/~jpds/launchpad/fix_489363/+merge/16531, just passing through, or talking about something else? In either of the last two cases, I'll review it. | 15:09 |
| jtv | allenap: I think that's the one I'm looking at | 15:10 |
| jtv | yes, it is | 15:10 |
| allenap | jtv: Okay, I'll leave it alone then. | 15:10 |
| sinzui | allenap: I am passing through. I saw a confusing date and had to remark on it | 15:10 |
| allenap | sinzui: Cool, thanks. | 15:10 |
| jtv | jpds: sinzui was right about the date btw... slashes in dates are pure, unmitigated, 7th-circle-of-hell evil | 15:11 |
| jpds | sinzui, jtv: I'm not doing slashes in dates, I'm just doing: "Wed Oct 20 12:00:00 2004". | 15:21 |
| jtv | jpds: I think he was just commenting on how you wrote it in here. :-) | 15:21 |
| sinzui | I was | 15:22 |
| === salgado-lunch is now known as salgado | ||
| allenap | jtv: Why are slashes so bad in dates? Honest question :) | 15:34 |
| jtv | allenap: what date is 08/09/10 ? | 15:35 |
| allenap | jtv: But what date is 08-09-10? | 15:35 |
| jtv | allenap: at least the convention there is pretty unambiguous: September 8 of next year | 15:35 |
| henninge | jtv: do you like this better? http://paste.ubuntu.com/345393/ | 15:38 |
| henninge | jtv: it avoids the shell completely | 15:38 |
| jtv | henninge: wow! | 15:39 |
| henninge | that can mean anything | 15:39 |
| jtv | henninge: nonsense... it can not mean, for instance, "the 3rd decimal of pi is 1" | 15:40 |
| henninge | ok, but apart from that ... | 15:40 |
| allenap | jtv: I suppose so. I hoped everyone learnt to use the full year about a decade ago. As for month and day ordering, I think there's only the Americans to blame. Alas, 300 million people are difficult to ignore. | 15:40 |
| jtv | henninge: in this case I mean to express admiration at your solution. | 15:40 |
| henninge | "Wow, this is great!" or "Wow, I have never seen such rubbish!" | 15:40 |
| henninge | option 1 it is | 15:41 |
| henninge | jtv: I am glad you llike it ;) | 15:41 |
| jtv | allenap: that's it in a nutshell... ostracizing them would be difficult and could be called discrimination, so the easiest way out is to ostracize the slash instead | 15:41 |
| jtv | henninge: and "call" looks a lot simpler than Popen too | 15:42 |
| henninge | a little googling and RTFM | 15:44 |
| === beuno is now known as beuno-lunch | ||
| === jamalta_ is now known as jamalta | ||
| henninge | jtv: did you have any other points to raise about the branch? | 16:10 |
| jtv | henninge: yes, still working on that reply in the mp | 16:11 |
| henninge | jtv: anything we can short-cut here? | 16:11 |
| jtv | henninge: I'm almost through, hang on | 16:11 |
| jtv | henninge: I've posted a reply | 16:18 |
| jtv | henninge: feel free to point out where I get nonsensical... I seem to be making a lot of mistakes tonight | 16:19 |
| sinzui | allenap: jtv: I have a CP candidate that will appear in reviews in a few minutes | 16:19 |
| allenap | sinzui: I can take it. | 16:19 |
| jtv | allenap: I was about to ask. Thanks. :) | 16:19 |
| sinzui | flacoste will check it after it clears review and we verity it on staging | 16:19 |
| * henninge checks Bangkok time | 16:20 | |
| henninge | jtv! Go to bed! | 16:20 |
| jtv | henninge: soon! | 16:21 |
| henninge | jtv: today! (your today) | 16:21 |
| jtv | henninge: that'd be hard, unless I sleep in the office | 16:21 |
| jtv | jpds: test_logMessage_ensure_message looks good... in test_logMessage_fake_time you can also use a local variable instead of a class member; then everything referring to self.prober can go away and you'll end up with just 3 methods. | 16:24 |
| jtv | _fake_gettime, test_logMessage_fake_time, and test_logMessage_ensure_message | 16:24 |
| jtv | jpds: I wouldn't call the middle one test_logMessage_fake_time though; that's how you test, but you want to say what behaviour you're testing—maybe test_logMessage_output because you're actually verifying the output | 16:25 |
| jtv | jpds: in a similar vein, maybe test_logMessage_integration for test_logMessage_ensure_message since you're testing the integration. (A bit far-fetched perhaps; but it seems weird to say both "test" and "ensure" for the same thing in the same identifier) | 16:26 |
| === salgado is now known as salgado-afk | ||
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: jpds, sinzui || queue [henninge, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: {jpds, henninge}, sinzui || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | allenap: that's a nice corner I've painted you into... | 16:41 |
| allenap | jtv: Hehehe :) | 16:42 |
| allenap | allenap: r=me, it's Christmas after all. | 16:42 |
| jpds | jtv: All done and pushed. | 16:42 |
| al-maisan | jtv: not sure what time it is on your end but the branch we discussed earlier is now ready for review: https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550 | 16:46 |
| jtv | allenap: wait... you can _do_ that!? | 16:47 |
| jtv | al-maisan: far too late for another review! | 16:47 |
| al-maisan | jtv: understood ..or maybe allenap could take a look at it? | 16:47 |
| al-maisan | if it's not too late for him | 16:47 |
| jtv | jpds: and since I'm being difficult... LogginMixin & its methods need to have docstrings. :-) | 16:48 |
| allenap | al-maisan: I very much doubt I'll get to it. It's my last day today and I have some other things to finish off after doing sinzui's branch and before going afk. | 16:48 |
| al-maisan | allenap: I understand .. never mind .. have a nice break! | 16:48 |
| allenap | al-maisan: Thanks :) | 16:48 |
| al-maisan | jtv: are you around tomorrow by any chance? | 16:49 |
| jtv | al-maisan: try bugging me about it tomorrow | 16:49 |
| jtv | heh | 16:49 |
| al-maisan | jtv: he he :) | 16:49 |
| === beuno-lunch is now known as beuno | ||
| jpds | jtv: Sure, done. | 16:51 |
| jtv | thanks | 16:51 |
| jtv | jpds: approved. You can now "ec2 land https://code.edge.launchpad.net/~jpds/launchpad/fix_489363/+merge/16531" | 16:55 |
| jpds | jtv: I don't have access to PQM. | 16:55 |
| jtv | jpds: nm I'll do it then | 16:55 |
| === salgado-afk is now known as salgado | ||
| jpds | jtv: Thanks! | 16:57 |
| === jtv changed the topic of #launchpad-reviews to: on call: allenap || reviewing: sinzui || queue [allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | allenap: that's me bowing out :) | 16:59 |
| jtv | henninge: continue on your mp tomorrow? | 16:59 |
| allenap | jtv: Cheerio, until 2010 :) | 16:59 |
| henninge | jtv: yes, I'll have something ready for you when you start. | 16:59 |
| henninge | jtv: thanks for hanging around this long. | 16:59 |
| jpds | jtv: Have a great holiday! | 16:59 |
| jtv | allenap: oh, you're not here tomorrow? then happy 2553 BE! | 16:59 |
| allenap | jtv: Thanks :) | 17:00 |
| jtv | jpds: thanks, same to you! | 17:00 |
| jtv | henninge: great, I like tag-team development | 17:00 |
| jtv | oh, it's midnight... no more trains. :) | 17:00 |
| allenap | sinzui: All done, r=me. | 17:08 |
| sinzui | thanks. Have a lovely holiday | 17:09 |
| === salgado is now known as salgado-afk | ||
| === allenap changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === salgado-afk is now known as salgado | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!