[00:40] <wgrant> thumper: Could you be convinced to review https://code.launchpad.net/~wgrant/launchpad/die-lucilleconfig-die/+merge/44305?
[00:41] <wgrant> Or abentley, if you are still around.
[00:47]  * thumper looks
[00:51] <thumper> you need to get stub to do the patch allocatino
[00:57] <wgrant> thumper: I already obtained a number from him yesterday.
[00:57] <wgrant> Thanks.
[01:09] <thumper> wgrant: is stub off now?
[01:20] <wgrant> thumper: I don't think so.
[01:21] <thumper> wgrant: well if you have the nod from stub, feel free to land it
[01:21] <wgrant> thumper: Oh, I don't actually have a review, just a number.
[01:21] <StevenK> wgrant: Win
[01:22] <wgrant> StevenK: Yes.
[01:22] <StevenK> wgrant: And thanks for tacking on the IDS change
[01:23] <wgrant> StevenK: It didn't really fit into any of my previous branches :(
[01:24] <wgrant> But regardless, that 6-year-old temporary hack is gone!
[13:30] <allenap> 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] <deryck> allenap: I can work through them, through the day, if that's okay.  I tend to be slow on large diffs.
[13:32] <allenap> 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] <allenap> I'll badger someone else for the bigger branch.
[13:32] <deryck> heh, ok.
[13:33]  * deryck goes to warm up the coffee before settling in
[13:49] <allenap> 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] <leonardr> allenap: sure
[13:50] <leonardr> give me the mp
[13:50] <allenap> leonardr: Thanks. It's https://code.launchpad.net/~allenap/launchpad/bug-activity-grouping-bug-353890/+merge/44247
[14:23] <leonardr> allenap: line 292, i don't see a big readability advantage to defining an expression as a lambda function
[14:23] <leonardr> versus just using the expression in the list comprehension
[14:23] <leonardr> what do you think?
[14:24] <allenap> leonardr: Yeah, you're right, I'll put it into the comprehension.
[14:24] <leonardr> k
[14:37] <leonardr> 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] <allenap> 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] <allenap> leonardr: I could have done something like:
[14:41] <allenap> events = []
[14:42] <leonardr> allenap: ah, i didn't see that they were generators
[14:42] <leonardr> chain is fine in that case
[14:42] <allenap> leonardr: Okay, cool :) I won't go on with my "something like" bit then :)
[14:58] <leonardr> allenap, having trouble wrapping my head around the idea of "first newest comment". what's going on there?
[15:00] <allenap> 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] <leonardr> why is the first newest comment the oldest?
[15:00] <leonardr> ah, it's first chronologically
[15:00] <allenap> leonardr: Yep.
[15:01] <leonardr> oldest_new_comment might make more sense
[15:01] <allenap> leonardr: I originally had oldest_newest_comment... perhaps I should rename newest_comments as recent_comments, then have oldest_recent_comment.
[15:02] <leonardr> that sounds good
[15:14] <leonardr> allenap: what happens in the section starting around line 527?
[15:15] <leonardr> 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] <allenap> leonardr: Mmm, I don't remember what I was doing there. I'll get back to you.
[15:17] <leonardr> ok
[15:18]  * leonardr thinks you could put the time format you use on lines 602 and 616 into a constant
[15:21] <leonardr> line 651 "no activities is" => "no activities are"
[15:26] <leonardr> test_common_actor_over_a_prolonged_time would be easier to understand if you mentioned that the grouping window is 5 minutes
[15:27] <allenap> leonardr: The time format is only used in the __repr__(), so just for debugging.
[15:27] <allenap> leonardr: Oh, yes, good idea.
[15:28] <leonardr> allenap: what would happen to test_interleaved_activity_with_comments_by_common_actor if there was another comment after activity2?
[15:28] <leonardr> would activity2 be associated with comment or comment2?
[15:28] <leonardr> (also, the test name is inaccurate since you only have one comment in that test)
[15:29] <allenap> leonardr: test_two_comments_with_activity_by_common_actor shows that I think; activities are grouped towards the earlier comment.
[15:30] <leonardr> right, ok
[15:30] <leonardr> the tests are great
[15:30] <allenap> leonardr: Cool, thanks :)
[15:39] <leonardr> allenap: why &\x238594; instead of &#8594;? does the # make your editor think the rest is a comment?
[15:40] <allenap> leonardr: Yeah, doctest-mode is a little stupid. I'll switch it back for the benefit of others.
[15:40] <leonardr> ok
[15:41] <allenap> Although doctest-mode is excellent in many ways :)
[15:52] <allenap> Ah, the bit around line 527 is exactly as you say: it now prints out everything at once.
[16:11] <leonardr> allenap: one more question about your interesting_changes_expression
[16:12] <allenap> Go for it.
[16:12] <leonardr> bugtask_change_re looks like it matches some optional text, then a colon, then one of assignee, importance, etc.
[16:12] <leonardr> is that right?
[16:13] <leonardr> anyhow, there's something, then one of a number of field names
[16:13] <allenap> 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] <leonardr> so interesting activity is a change to a bugtask, or a change to a bug
[16:14] <leonardr> which is why you don't have that special stuff for the bug fields
[16:14] <leonardr> right?
[16:14] <allenap> leonardr: Yeah.
[16:15] <leonardr> ok, this looks good
[16:15] <leonardr> i'll summarize the changes we tlaked about in the review
[16:15] <allenap> leonardr: Cheers, thank you!
[16:21] <allenap> leonardr: After your comments I thought of a way to make interesting_activity a bit more obvious: http://paste.ubuntu.com/546305/
[16:24] <leonardr> allenap, i like it
[16:24] <allenap> leonardr: Cool.
[16:26] <leonardr> sinzui, what is your merge proposal?
[16:26]  * sinzui looks
[16:27] <jcsackett> leonardr: for your queue https://code.launchpad.net/~jcsackett/launchpad/autofocus/+merge/44284
[16:27] <sinzui> leonardr, https://code.launchpad.net/~sinzui/launchpad/buga-buga/+merge/44297
[16:27] <leonardr> ok
[17:32] <leonardr> sinzui: in newbug.py you remove a test for 'event_creator is not None'. will that never happen?
[17:35] <leonardr> line 48, "bug nomiation" => "bug nomination"
[17:38] <leonardr> line 31, "submit label to" => "submit label"
[17:44] <sinzui> leonardr, it cannot ever happen
[17:44] <leonardr> ok
[17:45]  * leonardr doing final runthrough
[17:45] <sinzui> leonardr, the code requires use to use a transition method
[17:45] <sinzui> leonardr, I talked to deryck[lunch] about that because I could not imagine that case to be valid
[18:10] <leonardr> sinzui, r=me with typo fixes
[18:10] <sinzui> thanks
[18:32] <leonardr> jcsackett: am i right in thinking that the inputs whose focus you set will always be present?
[18:32] <leonardr> if so, r=me
[18:32] <leonardr> if not, will the javascript cause a problem if that input isn't around?
[19:04] <leonardr-dentist> i will be back in a few hours and i can take a couple more reviews when i do
[19:24] <EdwinGrubbs> 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] <jcsackett> EdwinGrubbs: i'm happy to look at it.
[19:35] <jcsackett> 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] <EdwinGrubbs> jcsackett: right
[19:44] <deryck> 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] <jcsackett> EdwinGrubbs: the tool that does the text show hide is already tested in windmill elsewhere, right?
[19:51] <EdwinGrubbs> 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] <jcsackett> 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] <jcsackett> EdwinGrubbs: r=me.
[19:53] <jcsackett> 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] <sinzui> yes
[20:05] <jcsackett> anyone available to review a very trivial branch: https://code.launchpad.net/~jcsackett/launchpad/unlink-321675/+merge/44388
[20:08] <bac> jcsackett: sure
[20:08] <jcsackett> thanks, bac.
[23:07] <leonardr> sinzui, what's the other branch you want me to review?
[23:09] <sinzui> leonardr, https://code.launchpad.net/~sinzui/launchpad/peppermint-sticks/+merge/44378
[23:19] <leonardr> sinzui: line 301, "by their names" => "by their display names", and show the display names?
[23:24] <leonardr> r=me apart from that
[23:40] <sinzui> thanks for pointing that out