| === barry` is now known as barry | ||
| mwhudson | anyone around? | 03:36 |
|---|---|---|
| henninge | jtv: Hi! Can you please review this patch of mine, that adds a test to yesterday's branch? | 08:17 |
| henninge | ;-) | 08:17 |
| henninge | jtv: http://paste.ubuntu.com/281070/ | 08:17 |
| henninge | jtv: I was in fact able to test it by going a bit deeper into the inner structure of the job. | 08:19 |
| jtv | henninge: that's what I figured yesterday, but the question was whether it's worth the trouble. | 08:19 |
| jtv | Doesn't look like too much work though | 08:20 |
| henninge | jtv: once I thought of it, it was not too hard to write, no. | 08:20 |
| henninge | jtv: the test case already has a lot of useful helper methods. | 08:20 |
| jtv | henninge: but do you know that this test exercises the problem case? I don't see a guarantee in there that there will be a directory in the set of changes. | 08:21 |
| henninge | jtv: the method _makeBranch... creates the directory in the branch, so it does come up as changed. | 08:22 |
| jtv | ah | 08:22 |
| henninge | jtv: I also tried it out when my fix is removed and the test does fail, with the first assert finding three elements in the list. | 08:23 |
| jtv | Is there an easy way you can check that in the test? | 08:23 |
| henninge | jtv: I am passing NULL_REVISON to the job, so it asks bzrlib for all changes since the creation of the branch, so the directories *will* always be included. | 08:24 |
| jtv | henninge: very well. Another point: why all the trouble asserting the contents of those two lists? You know what's in them, just assertEquals. | 08:26 |
| jtv | ah, you extract the paths | 08:26 |
| henninge | jtv: well, it could be that just a directory got extracted, instead of the file ... | 08:26 |
| henninge | jtv: but what I could do, is check that all in one assert | 08:27 |
| henninge | jtv: ah, no, not easily. | 08:27 |
| jtv | you could self.assertEqual([pot_path], [x[0] for x in job.template_files_changed]) | 08:28 |
| henninge | jtv: yes. Do you find that easier to read? | 08:28 |
| henninge | I was thinking along the lines of | 08:29 |
| jtv | henninge: I find it easier to read with a little helper for the repetitive parts | 08:29 |
| jtv | self.assertEqual([pot_path], get_paths(job.template_files_changed)) | 08:29 |
| henninge | self.assertEqual([(pot_path, pot_content], job.template_files_changed) | 08:29 |
| henninge | but I'd have to procide pot_content, with is a UniqueString atm. | 08:30 |
| henninge | provide | 08:30 |
| henninge | oh, could do that ... | 08:30 |
| henninge | jtv: incremental diff: http://paste.ubuntu.com/281080/ | 08:33 |
| jtv | henninge: why bother with unique strings for the content? Just pass "# empty" or something | 08:34 |
| jtv | The code does get a lot shorter though, which shows you're on to something. | 08:35 |
| henninge | jtv: I have been told that if a string is arbitrary, just say so be using UniqueString. | 08:35 |
| henninge | which makes sense to me | 08:35 |
| henninge | jtv: full difff: http://paste.ubuntu.com/281081/ | 08:35 |
| jtv | henninge: in this case, it isn't _entirely_ arbitrary: it's supposed to be in gettext. And you're not trying to catch bzr passing the wrong file contents, and there's no risk of accidentally picking up data from elsewhere. | 08:36 |
| henninge | jtv: right, it is not about the content at all | 08:37 |
| henninge | jtv: but this is needed to make the assert easy to write because the tuple contains the content. | 08:38 |
| henninge | tuples | 08:38 |
| jtv | Sure. r=jtv. | 08:38 |
| henninge | jtv: thank you! | 08:39 |
| henninge | jtv: btw, this is how a failure looks, which I think comes out nicely now: | 08:40 |
| henninge | AssertionError: not equal: | 08:40 |
| henninge | a = [(u'subdir/foo.pot', 'generic-string1')] | 08:40 |
| henninge | b = [(u'subdir/foo.pot', 'generic-string1'), (u'subdir', ''), ('', '')] | 08:40 |
| jtv | henninge: not surprising. :) Yes, looks good. | 08:41 |
| === henninge_ is now known as henninge | ||
| allenap | Hello gmb, fancy a very very small branch to kick the day off? | 10:11 |
| gmb | allenap: Sure. | 10:11 |
| allenap | https://code.edge.launchpad.net/~allenap/launchpad/itch/+merge/12576 | 10:11 |
| allenap | gmb: Thank you :) | 10:12 |
| === gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| gmb | allenap: Land it :) | 10:14 |
| allenap | gmb: Thanks :) | 10:14 |
| adeuring | gmb: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-438169-parse-sysfs-attr/+merge/12577 ? | 10:45 |
| gmb | adeuring: Certainly. | 10:45 |
| === gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| adeuring | gmb: thanks! | 10:45 |
| gmb | adeuring: Looks like you've committed at least part of a conflict in lib/canonical/launchpad/scripts/hwdbsubmissions.py (the <<<<<<< TREE line is there in the diff) | 10:53 |
| adeuring | gmb: Ah, right; there are conflicts with a brnach I submitted yesterday. | 10:54 |
| gmb | adeuring: In fact it looks like the entire conflict has been committed. | 10:54 |
| gmb | adeuring: Okay. Please fix that and paste a diff; I'll review everything that's not conflicted. | 10:55 |
| adeuring | gmb: let me see... I did not notice any conflict yersterday... | 10:55 |
| gmb | adeuring: Argh, according to the MP test_hwdb_submission_parser.py has the same problem - unresolved conflicts have been committed. | 10:57 |
| gmb | adeuring: I'll reject this mp and you can resubmit (otherwise the diff will never get updated) | 10:57 |
| adeuring | gmb: ok | 10:57 |
| adeuring | gmb: conflict resolved. | 11:14 |
| gmb | adeuring: Cool. Please submit a fresh mp so that I get a clean diff. | 11:14 |
| adeuring | gmb: I already resubmiitted the the MP | 11:15 |
| gmb | adeuring: Oh, cool. | 11:15 |
| gmb | Looking now, then :) | 11:15 |
| adeuring | gmb: scrap that -- i did mot wait long enough after pushing the latest version of the brnach... | 11:17 |
| gmb | Ha. | 11:18 |
| gmb | adeuring: So, what's the situation... should I just merge your branch into a clean branch of devel and do this the old fashioned way or do you want to delete the old MP and create a new one? | 11:42 |
| adeuring | gmb: I_m waiting thatthe latest version of the brnach appears on LP... | 11:42 |
| gmb | adeuring: Ok. | 11:43 |
| adeuring | I should perhaps simply delete the brnach and and push it again.. | 11:43 |
| gmb | adeuring: That might work, yes. | 11:49 |
| gmb | adeuring: Or push --overwrite, but I'd favour the first option myself. | 11:49 |
| adeuring | gmb: https://code.edge.launchpad.net/~adeuring/launchpad/bug-438169-parse-sysfs-attr/+merge/12581 | 11:50 |
| * gmb looks | 11:50 | |
| gmb | adeuring: Ah, that looks much saner. | 11:50 |
| gmb | adeuring: To make that test nicer in test_hwdb_submission_parser.py you could use dedent() rather than removing all the indentation from the sysfs-attributes string. | 11:53 |
| gmb | At the moment it looks quite ugly. | 11:53 |
| adeuring | gmb: right; will do | 11:53 |
| gmb | adeuring: Also in that test, the opening braces of dicts shouldn't be on a line of their own. Better to put them on the end of the line above. | 11:54 |
| gmb | (I'm talking about the assertEqual() call here) | 11:54 |
| gmb | adeuring: Other than that, r=me. | 11:57 |
| adeuring | gmb: thanks! | 11:57 |
| === gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === matsubara-afk is now known as matsubara | ||
| cprov | gmb: accepting reviews ? | 13:14 |
| gmb | cprov: Just about to grab some lunch, but I'll put it on the queue for when I return. | 13:14 |
| === gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [cprov] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| cprov | gmb: https://code.edge.launchpad.net/~cprov/launchpad/bug-408528-ensurePerson-again/+merge/12583 | 13:14 |
| * gmb lunches | 13:15 | |
| cprov | gmb: thank you. | 13:15 |
| === noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [cprov, noodles] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === danilos is now known as Guest95679 | ||
| noodles775 | Hi gmb or reviewer - can I pls get a js review for this one when you've time! https://code.edge.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035 | 13:17 |
| === danilo__ is now known as danilos | ||
| === bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: - || queue: [cprov, noodles] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === jtv is now known as jtv-afk | ||
| === bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, noodles || queue: [cprov] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === mup_ is now known as mup | ||
| bac | hi noodles775 | 13:40 |
| === jtv-afk is now known as jtv | ||
| noodles775 | hi bac! | 14:23 |
| === gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: cprov, noodles || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| bac | hi noodles775. i was confused by the MP you submitted. it looks like it has already been reviewed? which are the new parts? | 14:30 |
| noodles775 | bac: it has, but salgado requested a javascript review (as he didn't feel confident enough). In the mean-time, I added js unittests and a windmill test (in addition to small changes suggested by salgado) | 14:31 |
| gmb | cprov: Do you want me to leave that review to sinzui? | 14:37 |
| gmb | Or am I looking at the wrong branch? | 14:37 |
| cprov | gmb: no, please review it yourself, I will just pass it through curtis if he has time | 14:38 |
| gmb | cprov: Okay, looking now - we are talking about bug-408528-ensurePerson-again, right? | 14:42 |
| cprov | gmb: yup | 14:42 |
| gmb | Cool. | 14:42 |
| cprov | gmb: prepare your brain to be melted ... | 14:43 |
| gmb | Oh, fun... | 14:43 |
| bac | noodles775: so is this review a JS review specifically? | 14:45 |
| noodles775_ | bac: yep. The rest is already reviewed. | 14:46 |
| noodles775_ | bac: but of course, is you see something you think should be improved... | 14:46 |
| noodles775_ | s/is/if | 14:46 |
| bac | noodles775_: i think i'm going to follow salgado's lead and defer to someone with more JS experience | 14:46 |
| noodles775_ | bac: ok, pop it back in the queue... perhaps gmb can have a look (or you guys could swap?) | 14:47 |
| gmb | noodles775_ I'll try when I've finished with cprov's branch. | 14:47 |
| gmb | If I have a brain left. | 14:47 |
| noodles775_ | heh, ok! Thanks. | 14:48 |
| gmb | cprov: r=me, pending sinzui's approval, too. | 14:59 |
| cprov | gmb: super! I will address Curtis comment. Thanks | 15:00 |
| noodles775_ | BjornT: I just wrote our first windmill test using the new layer - you've made writing windmill tests about 100x more fun :) | 15:01 |
| === noodles775_ changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: cprov, - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com | ||
| === gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: noodles, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| gmb | noodles775_: Argh, I'm only just getting to your branch, sorry. Taking a look now. | 15:37 |
| gmb | noodles775_: |Er... where's the MP; I don't see it on activereviews. | 15:38 |
| noodles775_ | gmb: https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035 | 15:40 |
| noodles775_ | thanks. | 15:40 |
| * gmb looks | 15:41 | |
| gmb | noodles775_: Wouldn't it be better to use "if (! Y.Lang.isValue())" than checking for isNull? | 16:00 |
| gmb | Or are you expecting undefineds? | 16:00 |
| noodles775_ | gmb: just on a call, I'll be there in a tick. | 16:00 |
| gmb | noodles775_: Okay, no rush. I'll grab a drink whilst I wait. | 16:01 |
| noodles775_ | gmb: hmm, so Y.get returns null if the thing you're trying to get doesn't exist, so Y.Lang.isNull seems more straight forward to me? | 16:07 |
| noodles775_ | gmb: in fact, I'd only use !Y.Lang.isValue() if I was expecting undefineds (which I'm not here) right? | 16:08 |
| gmb | noodles775_: Well, you'd generally use it as a belt-and-braces. If you're not worried about it then fair enough. | 16:10 |
| gmb | Just checking. | 16:10 |
| adeuring | gmb, bac: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/bug-438734-parse-kernel-release-nnode/+merge/12589 ? | 16:14 |
| bac | adeuring, gmb: i will | 16:14 |
| adeuring | bac: thanks! | 16:15 |
| === bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: noodles, adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| gmb | noodles775_: The comments at the top of the tests should describe the behaviour that the test expects. At the moment they seem to be descriptive of the line the immediately precede. | 16:16 |
| gmb | Also, some of the tests lack comments. | 16:16 |
| noodles775_ | gmb: Yes, I initially had comments there, but then realised that I was duplicating the same string used in the assert? | 16:17 |
| noodles775_ | Is there a reason for having that initial comment describing the behaviour the test expects when it's also in the assert output? I wasn't sure. | 16:18 |
| gmb | noodles775_: Well, I'd change the Assert errors to read more like "X should happen (when Y)" and have the leading comments as "X happens when Y". The comment is for the reader, whereas the Assert message is for the developer who sees a failure (e.g. "X should occur when Y, but it didn't.") | 16:19 |
| gmb | noodles775_: The advantage of the comments is that you don't have to read the code. | 16:19 |
| gmb | (Unless you want to) | 16:19 |
| noodles775_ | gmb: ok - it seems like duplicating (virtually) the same info, but I can do that :) | 16:20 |
| gmb | noodles775_: Two different sets of stakeholders. The person reading the test failures isn't neccessarily the same person who's looking at the code. And the person who's looking at the code doesn't want to parse it when he doesn't have to. | 16:21 |
| gmb | Especially if that person is me. | 16:21 |
| noodles775_ | gmb: yep, np. I personally just don't see it as any easier to parse the comment than the assertion string in such small tests, but consider it done :) | 16:21 |
| gmb | noodles775_: Cool. | 16:22 |
| gmb | noodles775_: Other than that, r=me. | 16:22 |
| noodles775_ | Great, thanks! | 16:22 |
| * gmb goes off review so that he can catch up on other work. | 16:22 | |
| === gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| bac | adeuring: done | 16:26 |
| bac | bye gmb | 16:26 |
| adeuring | bac: thanks! | 16:26 |
| === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| === matsubara is now known as matsubara-lunch | ||
| allenap | bac: Got time to review a tiny change? https://code.edge.launchpad.net/~allenap/launchpad/ec2-niceties/+merge/12593 | 16:55 |
| bac | allenap: yep | 16:56 |
| allenap | bac: Thanks. | 16:56 |
| bac | allenap: no, thank you! i look forward to a quieter ec2 experience | 16:56 |
| bac | allenap: hey, while i have you held hostage, did you see my bug about buildbot and UTC? | 16:57 |
| bac | allenap: r=bac, btw | 16:57 |
| allenap | bac: Thanks :) | 16:57 |
| bac | allenap: gah, does this mean you're ignoring me? | 16:58 |
| allenap | bac: I think I did, but I can't remember it now. | 16:58 |
| allenap | bac: Of course I'm not ignoring you :) I'm just taking a long time. | 16:58 |
| bac | ok, well when you have a chance to look into it i'd be curious to know if it is doable/easy. | 16:58 |
| bac | my head almost exploded last week in a call trying to juggle UTC, BST, and EST | 16:59 |
| allenap | bac: BST is easy for me ;) | 17:01 |
| bac | allenap: cool, i'll nominate you for release manager | 17:02 |
| allenap | bac: Actually, I'll start work on this bug soon. It seems like a good place for me to get into buildbot. | 17:02 |
| allenap | bac: On second thoughts.... | 17:02 |
| BjornT | bac: can you do a quick review of this? https://code.edge.launchpad.net/~bjornt/launchpad/windmill-1.3beta/+merge/12601 | 18:21 |
| bac | sure BjornT | 18:21 |
| === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: bjornt || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| BjornT | bac: i did this change just now to clarify things further: http://pastebin.ubuntu.com/281491/ | 18:24 |
| bac | BjornT: i'm a little confused. where will it find that version of windmill? | 18:25 |
| BjornT | bac: in download-cache. i've already added it there, so you can test it if you want | 18:26 |
| bac | BjornT: ah, right. (lightbulb goes off) | 18:26 |
| bac | BjornT: this looks good to me. | 18:27 |
| BjornT | thanks | 18:27 |
| === EdwinGrubbs is now known as Edwin-lunch | ||
| al-maisan | hello bac, how are things? | 18:46 |
| bac | hi al-maisan | 18:46 |
| === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| al-maisan | Could you please review a branch of mine? | 18:47 |
| bac | al-maisan: sure. | 18:47 |
| al-maisan | It's here: https://code.edge.launchpad.net/~al-maisan/launchpad/unembargo-email-316488/+merge/12603 | 18:47 |
| al-maisan | bac: thanks | 18:47 |
| bac | al-maisan: i'll get on it shortly when the diff arrives | 18:48 |
| al-maisan | bac: thanks, that's fine :) | 18:48 |
| al-maisan | hmm .. the diff calculation for merge proposals seems not to be working correctly .. | 18:53 |
| al-maisan | bac: the actual diff is here: https://pastebin.canonical.com/22758/ | 18:53 |
| bac | thanks muharem | 19:09 |
| bac | al-maisan: i wonder why the diff is different. did you target the wrong branch? | 19:10 |
| al-maisan | bac: maybe | 19:13 |
| al-maisan | I branched from devel | 19:13 |
| al-maisan | should I be branching off db-devel ? | 19:13 |
| bac | al-maisan: no, devel is right, AFAIK | 19:14 |
| bac | al-maisan: please use paste.ubuntu.com so everyone has access. | 19:18 |
| bac | al-maisan the code changes look great. thanks for the nice branch. | 19:19 |
| al-maisan | bac: thanks :) | 19:20 |
| al-maisan | will use paste.ubuntu.com in future | 19:20 |
| === Edwin-lunch is now known as EdwinGrubbs | ||
| leonardr | bac, can you review this lazr.restfulclient branch? | 20:36 |
| leonardr | https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/basic-auth/+merge/12612 | 20:36 |
| bac | leonardr: sure | 20:36 |
| bac | leonardr: why is that branch private? | 20:37 |
| leonardr | not intentionally | 20:37 |
| bac | i figured not. odd. | 20:37 |
| leonardr | you should be able to see it now | 20:37 |
| bac | leonardr: i can see it but was curious about the hash marks on the left, the privacy indicator | 20:38 |
| bac | leonardr: on the MP i mean | 20:38 |
| leonardr | maybe that project's branches are private by default | 20:39 |
| bac | leonardr: the default policy is public but branches by ~launchpad are private. i think that should be changed unless there's a good reason | 20:40 |
| bac | nice job leonardr. r=bac | 20:48 |
| leonardr | great | 20:48 |
| bac | hi barry, are you really here? | 20:48 |
| barry | bac: kinda sorta | 20:51 |
| barry | bac: what's up? | 20:52 |
| bac | barry: not much. just seeing if you are alive | 20:52 |
| barry | bac: ibuprofen is a wonderdrug | 20:52 |
| === bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| === matsubara is now known as matsubara-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!