[00:40] thumper: Could you be convinced to review https://code.launchpad.net/~wgrant/launchpad/die-lucilleconfig-die/+merge/44305? [00:41] Or abentley, if you are still around. [00:47] * thumper looks [00:51] you need to get stub to do the patch allocatino [00:57] thumper: I already obtained a number from him yesterday. [00:57] Thanks. === 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 [01:09] wgrant: is stub off now? [01:20] thumper: I don't think so. [01:21] wgrant: well if you have the nod from stub, feel free to land it [01:21] thumper: Oh, I don't actually have a review, just a number. [01:21] wgrant: Win [01:22] StevenK: Yes. [01:22] wgrant: And thanks for tacking on the IDS change [01:23] StevenK: It didn't really fit into any of my previous branches :( [01:24] But regardless, that 6-year-old temporary hack is gone! === lifeless_ is now known as lifeless === _thumper_ is now known as thumper === jelmer__ is now known as jelmer [13:30] deryck: Are you busy (stupid question)? Would you have time for a review or two? Both are quite large branches (701 lines and 1077 lines). [13:30] allenap: I can work through them, through the day, if that's okay. I tend to be slow on large diffs. [13:32] deryck: Thank you. The smaller one is reasonably straightforward: https://code.launchpad.net/~allenap/launchpad/sub-filters-via-api-bug-672619/+merge/44331 [13:32] I'll badger someone else for the bigger branch. [13:32] heh, ok. [13:33] * deryck goes to warm up the coffee before settling in [13:49] leonardr: Hi there. Are you on-call reviewing this week? If so, would you be able to review an oversize branch of mine? It's not too complicated actually; a lot of the bulk is doctest and import noise. [13:50] allenap: sure === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:50] give me the mp === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:50] leonardr: Thanks. It's https://code.launchpad.net/~allenap/launchpad/bug-activity-grouping-bug-353890/+merge/44247 [14:23] allenap: line 292, i don't see a big readability advantage to defining an expression as a lambda function [14:23] versus just using the expression in the list comprehension [14:23] what do you think? [14:24] leonardr: Yeah, you're right, I'll put it into the comprehension. [14:24] k === henninge_ is now known as henninge [14:37] allenap: how about line 66, replacing chain(comments, activity) with comments + activity? is performance super important? i had to look up what chain() does [14:40] leonardr: Performance in bug comments especially has been quite critical, but I doubt changing it as you suggest would have much or any impact. I used chain() because comments and activity are generators; I didn't nead to materialize them into almost-immediately discarded lists so I didn't. The itertools module has a lot of very nice things in it, and I would argue that it's worth being familiar with. [14:41] leonardr: I could have done something like: [14:41] events = [] [14:42] allenap: ah, i didn't see that they were generators [14:42] chain is fine in that case [14:42] leonardr: Okay, cool :) I won't go on with my "something like" bit then :) [14:58] allenap, having trouble wrapping my head around the idea of "first newest comment". what's going on there? [15:00] leonardr: Yeah, it doesn't read well. When there are many comments, only the first X and last X comments are shown; in the code that's oldest_comments and newest_comments. The first newest comment is the oldest of the most recent comments to be shown. [15:00] why is the first newest comment the oldest? [15:00] ah, it's first chronologically [15:00] leonardr: Yep. [15:01] oldest_new_comment might make more sense [15:01] leonardr: I originally had oldest_newest_comment... perhaps I should rename newest_comments as recent_comments, then have oldest_recent_comment. [15:02] that sounds good === sinzui changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: allenap || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:14] allenap: what happens in the section starting around line 527? [15:15] it looks like you used to print out activity_and_comments, then take a look at the most recent comment, and now you just print everything out at once? [15:17] leonardr: Mmm, I don't remember what I was doing there. I'll get back to you. [15:17] ok [15:18] * leonardr thinks you could put the time format you use on lines 602 and 616 into a constant [15:21] line 651 "no activities is" => "no activities are" [15:26] test_common_actor_over_a_prolonged_time would be easier to understand if you mentioned that the grouping window is 5 minutes [15:27] leonardr: The time format is only used in the __repr__(), so just for debugging. [15:27] leonardr: Oh, yes, good idea. [15:28] allenap: what would happen to test_interleaved_activity_with_comments_by_common_actor if there was another comment after activity2? [15:28] would activity2 be associated with comment or comment2? [15:28] (also, the test name is inaccurate since you only have one comment in that test) [15:29] leonardr: test_two_comments_with_activity_by_common_actor shows that I think; activities are grouped towards the earlier comment. [15:30] right, ok [15:30] the tests are great [15:30] leonardr: Cool, thanks :) [15:39] allenap: why &\x238594; instead of →? does the # make your editor think the rest is a comment? [15:40] leonardr: Yeah, doctest-mode is a little stupid. I'll switch it back for the benefit of others. [15:40] ok [15:41] Although doctest-mode is excellent in many ways :) [15:52] Ah, the bit around line 527 is exactly as you say: it now prints out everything at once. [16:11] allenap: one more question about your interesting_changes_expression [16:12] Go for it. [16:12] bugtask_change_re looks like it matches some optional text, then a colon, then one of assignee, importance, etc. [16:12] is that right? [16:13] anyhow, there's something, then one of a number of field names [16:13] leonardr: Yeah, the optional text is the name (according to valid_name()'s rules) of the bugtask target to which the activity pertains. [16:14] so interesting activity is a change to a bugtask, or a change to a bug [16:14] which is why you don't have that special stuff for the bug fields [16:14] right? [16:14] leonardr: Yeah. [16:15] ok, this looks good [16:15] i'll summarize the changes we tlaked about in the review [16:15] leonardr: Cheers, thank you! [16:21] leonardr: After your comments I thought of a way to make interesting_activity a bit more obvious: http://paste.ubuntu.com/546305/ [16:24] allenap, i like it [16:24] leonardr: Cool. === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:26] sinzui, what is your merge proposal? [16:26] * sinzui looks [16:27] leonardr: for your queue https://code.launchpad.net/~jcsackett/launchpad/autofocus/+merge/44284 === jcsackett changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:27] leonardr, https://code.launchpad.net/~sinzui/launchpad/buga-buga/+merge/44297 [16:27] ok === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: sinzui || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === deryck is now known as deryck[lunch] [17:32] sinzui: in newbug.py you remove a test for 'event_creator is not None'. will that never happen? [17:35] line 48, "bug nomiation" => "bug nomination" === benji is now known as benji-lunch [17:38] line 31, "submit label to" => "submit label" [17:44] leonardr, it cannot ever happen [17:44] ok [17:45] * leonardr doing final runthrough [17:45] leonardr, the code requires use to use a transition method [17:45] leonardr, I talked to deryck[lunch] about that because I could not imagine that case to be valid [18:10] sinzui, r=me with typo fixes === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: jcsackett|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:10] thanks === deryck[lunch] is now known as deryck [18:32] jcsackett: am i right in thinking that the inputs whose focus you set will always be present? [18:32] if so, r=me === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:32] if not, will the javascript cause a problem if that input isn't around? === benji-lunch is now known as benji === leonardr is now known as leonardr-dentist [19:04] i will be back in a few hours and i can take a couple more reviews when i do === sinzui changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: -|| queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === sinzui changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: -|| queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:24] bac, jcsackett: can you review a trivial branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-526608-milestone-ajax-plus-icon/+merge/44382 [19:24] EdwinGrubbs: i'm happy to look at it. [19:35] EdwinGrubbs: the two "style_for_" things are just to hide the irrelevant controls on milestones, right? so if there's a milestone, don't show add. [19:36] jcsackett: right [19:44] sinzui: would you be willing to review my fix removing all the linked js files and not depending on devmode for using launchpad.js? [19:50] EdwinGrubbs: the tool that does the text show hide is already tested in windmill elsewhere, right? [19:51] jcsackett: the widget in lazr_js actually hides and shows the elements with the classes "addicon", "editicon", and "nulltext", so that has yui tests. I'm not sure how detailed the windmill tests are. [19:52] EdwinGrubbs: dig, that was my intuition. just checking for the lack of new test coverage, but since it's using stuff that's already safely in use elsewhere that seems fine to me. [19:53] EdwinGrubbs: r=me. [19:53] sinzui: can you follow up for me on https://code.launchpad.net/~edwin-grubbs/launchpad/bug-526608-milestone-ajax-plus-icon/+merge/44382? [20:03] yes [20:05] anyone available to review a very trivial branch: https://code.launchpad.net/~jcsackett/launchpad/unlink-321675/+merge/44388 [20:08] jcsackett: sure [20:08] thanks, bac. === jelmer__ is now known as jelmer === gary_poster_ is now known as gary_poster === gary_poster_ is now known as gary_poster === leonardr-dentist is now known as leonardr [23:07] sinzui, what's the other branch you want me to review? [23:09] leonardr, https://code.launchpad.net/~sinzui/launchpad/peppermint-sticks/+merge/44378 [23:19] sinzui: line 301, "by their names" => "by their display names", and show the display names? [23:24] r=me apart from that [23:40] thanks for pointing that out