| wgrant | thumper: Could you be convinced to review https://code.launchpad.net/~wgrant/launchpad/die-lucilleconfig-die/+merge/44305? | 00:40 |
|---|---|---|
| wgrant | Or abentley, if you are still around. | 00:41 |
| * thumper looks | 00:47 | |
| thumper | you need to get stub to do the patch allocatino | 00:51 |
| wgrant | thumper: I already obtained a number from him yesterday. | 00:57 |
| wgrant | Thanks. | 00:57 |
| === 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 | ||
| thumper | wgrant: is stub off now? | 01:09 |
| wgrant | thumper: I don't think so. | 01:20 |
| 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:21 |
| wgrant | StevenK: Yes. | 01:22 |
| StevenK | wgrant: And thanks for tacking on the IDS change | 01:22 |
| wgrant | StevenK: It didn't really fit into any of my previous branches :( | 01:23 |
| wgrant | But regardless, that 6-year-old temporary hack is gone! | 01:24 |
| === lifeless_ is now known as lifeless | ||
| === _thumper_ is now known as thumper | ||
| === jelmer__ is now known as jelmer | ||
| 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:30 |
| 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:32 |
| * deryck goes to warm up the coffee before settling in | 13:33 | |
| 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:49 |
| leonardr | allenap: sure | 13:50 |
| === 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 | ||
| leonardr | give me the mp | 13:50 |
| === 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 | ||
| allenap | leonardr: Thanks. It's https://code.launchpad.net/~allenap/launchpad/bug-activity-grouping-bug-353890/+merge/44247 | 13:50 |
| 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:23 |
| allenap | leonardr: Yeah, you're right, I'll put it into the comprehension. | 14:24 |
| leonardr | k | 14:24 |
| === henninge_ is now known as henninge | ||
| 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:37 |
| 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:40 |
| allenap | leonardr: I could have done something like: | 14:41 |
| allenap | events = [] | 14:41 |
| 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:42 |
| leonardr | allenap, having trouble wrapping my head around the idea of "first newest comment". what's going on there? | 14:58 |
| 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:00 |
| 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:01 |
| leonardr | that sounds good | 15:02 |
| === 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 | ||
| leonardr | allenap: what happens in the section starting around line 527? | 15:14 |
| 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:15 |
| allenap | leonardr: Mmm, I don't remember what I was doing there. I'll get back to you. | 15:17 |
| leonardr | ok | 15:17 |
| * leonardr thinks you could put the time format you use on lines 602 and 616 into a constant | 15:18 | |
| leonardr | line 651 "no activities is" => "no activities are" | 15:21 |
| leonardr | test_common_actor_over_a_prolonged_time would be easier to understand if you mentioned that the grouping window is 5 minutes | 15:26 |
| allenap | leonardr: The time format is only used in the __repr__(), so just for debugging. | 15:27 |
| allenap | leonardr: Oh, yes, good idea. | 15:27 |
| 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:28 |
| allenap | leonardr: test_two_comments_with_activity_by_common_actor shows that I think; activities are grouped towards the earlier comment. | 15:29 |
| leonardr | right, ok | 15:30 |
| leonardr | the tests are great | 15:30 |
| allenap | leonardr: Cool, thanks :) | 15:30 |
| leonardr | allenap: why &\x238594; instead of →? does the # make your editor think the rest is a comment? | 15:39 |
| allenap | leonardr: Yeah, doctest-mode is a little stupid. I'll switch it back for the benefit of others. | 15:40 |
| leonardr | ok | 15:40 |
| allenap | Although doctest-mode is excellent in many ways :) | 15:41 |
| allenap | Ah, the bit around line 527 is exactly as you say: it now prints out everything at once. | 15:52 |
| leonardr | allenap: one more question about your interesting_changes_expression | 16:11 |
| 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:12 |
| 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:13 |
| 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:14 |
| 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:15 |
| allenap | leonardr: After your comments I thought of a way to make interesting_activity a bit more obvious: http://paste.ubuntu.com/546305/ | 16:21 |
| leonardr | allenap, i like it | 16:24 |
| allenap | leonardr: Cool. | 16:24 |
| === 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 | ||
| leonardr | sinzui, what is your merge proposal? | 16:26 |
| * sinzui looks | 16:26 | |
| jcsackett | leonardr: for your queue https://code.launchpad.net/~jcsackett/launchpad/autofocus/+merge/44284 | 16:27 |
| === 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 | ||
| sinzui | leonardr, https://code.launchpad.net/~sinzui/launchpad/buga-buga/+merge/44297 | 16:27 |
| leonardr | ok | 16:27 |
| === 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] | ||
| leonardr | sinzui: in newbug.py you remove a test for 'event_creator is not None'. will that never happen? | 17:32 |
| leonardr | line 48, "bug nomiation" => "bug nomination" | 17:35 |
| === benji is now known as benji-lunch | ||
| leonardr | line 31, "submit label to" => "submit label" | 17:38 |
| sinzui | leonardr, it cannot ever happen | 17:44 |
| leonardr | ok | 17:44 |
| * 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 | 17:45 |
| leonardr | sinzui, r=me with typo fixes | 18:10 |
| === 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 | ||
| sinzui | thanks | 18:10 |
| === deryck[lunch] is now known as deryck | ||
| 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 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 | ||
| leonardr | if not, will the javascript cause a problem if that input isn't around? | 18:32 |
| === benji-lunch is now known as benji | ||
| === leonardr is now known as leonardr-dentist | ||
| leonardr-dentist | i will be back in a few hours and i can take a couple more reviews when i do | 19:04 |
| === 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 | ||
| 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:24 |
| 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:35 |
| EdwinGrubbs | jcsackett: right | 19:36 |
| 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:44 |
| jcsackett | EdwinGrubbs: the tool that does the text show hide is already tested in windmill elsewhere, right? | 19:50 |
| 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:51 |
| 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:52 |
| 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? | 19:53 |
| sinzui | yes | 20:03 |
| jcsackett | anyone available to review a very trivial branch: https://code.launchpad.net/~jcsackett/launchpad/unlink-321675/+merge/44388 | 20:05 |
| bac | jcsackett: sure | 20:08 |
| jcsackett | thanks, bac. | 20:08 |
| === 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 | ||
| leonardr | sinzui, what's the other branch you want me to review? | 23:07 |
| sinzui | leonardr, https://code.launchpad.net/~sinzui/launchpad/peppermint-sticks/+merge/44378 | 23:09 |
| leonardr | sinzui: line 301, "by their names" => "by their display names", and show the display names? | 23:19 |
| leonardr | r=me apart from that | 23:24 |
| sinzui | thanks for pointing that out | 23:40 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!