=== matsubara-afk is now known as matsubara === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [15:00] #startmeeting [15:00] Meeting started at 09:00. The chair is bac. [15:00] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:00] Hello and welcome to the LP Reviewers Meeting. Who is here today? [15:00] me [15:00] hi bac [15:00] me [15:00] me [15:00] me [15:00] hi jtv [15:01] lurk [15:01] me [15:02] BjornT, EdwinGrubbs, mars, gmb, allenap: ping [15:02] me [15:02] adeuring, gary_poster: ping [15:02] Thanks bac. [15:02] me [15:02] me [15:02] salgado: ping [15:02] who else are we missing? === matsubara is now known as matsubara-lunch [15:03] me [15:03] me-ish [15:03] me [15:04] ok, let's move on and perhaps others will show up [15:04] me [15:04] [topic] Agenda [15:04] New Topic: Agenda [15:04] * Roll call [15:04] * Action items [15:04] * Landing branches for community contributors. [bac for jml] [15:04] * Ease the "no lambdas" coding style to allow using them for Twisted callbacks. [heninge, allenap, abentley] [15:04] * Peanut gallery (anything not on the agenda) [15:04] we had a lot of outstanding actions from two weeks ago but not much on the agenda for today. [15:05] [topic] action items [15:05] New Topic: action items [15:05] [topic] * allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc [15:05] New Topic: * allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc [15:05] Dang, still haven't done that. [15:05] Please keep it on the list. [15:06] allenap: will do. you think you'll have time this week? [15:06] we need a new topic, "who sucks this week?" [15:06] [topic] * gary_poster to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again 10-Feb. [15:06] New Topic: * gary_poster to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again 10-Feb. [15:06] me [15:06] * gary_poster cringes [15:06] bac: Yes, I will. I do suck :( [15:06] 17-Feb [15:06] did i say the 10th? surely i meant the 17th, gary_poster [15:06] :-D [15:06] :-) thanks [15:06] me [15:07] [topic] * brad to find and clean up relevant reviewer pages on lp.c.c and move to dev.launchpad.net. [15:07] New Topic: * brad to find and clean up relevant reviewer pages on lp.c.c and move to dev.launchpad.net. [15:07] * bac rocks [15:07] cool bac! [15:07] i think i got all of this done. let me know if there are other old pages that need carting over. [15:07] * jtv cheers bac [15:07] [topic] * salgado to update the wiki page to encourage reviews with sufficient context. [15:07] New Topic: * salgado to update the wiki page to encourage reviews with sufficient context. [15:08] salgado doesn't appear to be here so we'll hold that one [15:08] [topic] * bac to update coding guideline wrt conditional expressions. [15:08] New Topic: * bac to update coding guideline wrt conditional expressions. [15:08] * bac two for two [15:09] * noodles775 claps [15:09] [topic] * barry to school us on the proper use of 'with'. [15:09] New Topic: * barry to school us on the proper use of 'with'. [15:09] * bigjools hi-5s bac [15:09] barry thanks for the great email on that topic [15:09] np! [15:09] it was really informative [15:09] and finally [15:09] [topic] * edwingrubbs to document JS namespace issue. [15:09] New Topic: * edwingrubbs to document JS namespace issue. [15:10] bac: yes, that has been added to the wiki, and I opened bugs for each team. [15:10] excellent EdwinGrubbs [15:10] so, we've closed about half of those outstanding issue [15:10] so on to new, hopefully more interesting stuff [15:11] [topic] * Landing branches for community contributors. [bac for jml] [15:11] New Topic: * Landing branches for community contributors. [bac for jml] [15:11] ok, so this isn't too interesting [15:11] jml wanted us to discuss our approach for landing community contributions [15:12] i think unofficially we've assumed the reviewer would do it but that isn't always the case. [15:13] hello, I am unexpectedly here [15:13] i'm pretty sure we discussed in the past the idea that the contributor needs to find someone to do the landing and make arrangements. but i think reviewers need to plan on taking care of the landing unless otherwise noted [15:13] hi jml. welcome. [15:14] bac: +1 [15:14] jml's other point was the OCR should be the next contact if the original reviewer isn't around when the branch is actually ready [15:14] like i said, not controversial but we need to agree this is how we'll do it [15:14] thoughts? [15:15] ok, i'll formalize that idea and slap it on the wiki page [15:15] I agree. Also, as mentioned before, all it takes is "ec2 land http://url_to_mp" in a current devel. [15:15] doesn't that suffer from the last-minute-change hole? [15:16] [action] bac to update wiki page to make clear community contributor landing responsibilities [15:16] ACTION received: bac to update wiki page to make clear community contributor landing responsibilities [15:16] jtv: it does [15:16] oh, it does? [15:16] jtv, you mean, the script has a bug? [15:16] jtv: there is an easy fix to ec2 i'd like to do when i get a chance but haven't yet [15:16] jml: not necessarily, but I'm not sure what behaviour is specified [15:17] bac: also, I made it my practice to add "Landed by henninge" to the commit message, as we discussed earlier. Should that be formalized, too? [15:17] the problem is the contributor can push new changes to the branch during the time ec2 is running. those new unreviewed, untested changes are then picked up and landed by pqm [15:17] * henninge remembers [15:17] bac, and also the contributor can push new changes while the branch is in PQM. [15:17] I always copy the branch before submitting it [15:18] abentley: sure, but that's generally a smaller windown [15:18] I think bigjools' approach is absolutely the right one. [15:18] I think it's almost the right one. [15:18] I also think we should be emailed when a branch gets new revisions after a review has started [15:18] But with that, resorting to the OCR when the reviewer is out of reach may lead to confusion. [15:18] jml: given the current technical constraints. [15:18] bigjools: that's what i do now. i wonder if we could get some tool support to make that less of a PITA [15:18] jtv, the tools are easily fixed. [15:19] I'd rather a way of freezing the branch [15:19] I implemented merge directives so that they would specify the particular revision to land. Unfortunately, PQM still doesn't support them. [15:19] jml: all for that. [15:19] PQM must die [15:19] jtv, do it. if you have time to land someone's branch in a painstaking way you have time to make it easier for everyone. [15:19] bigjools, +1 [15:19] abentley: I'd prefer "refuse branches with unreviewed revisions" over "land an older version" [15:19] die in the most horrible manner possible [15:20] bigjools, swift death ftw. [15:20] jtv: +1 [15:20] jml: the tools can not eliminate the PQM window abentley pointed out [15:20] bac, copying the branch addresses that issue too. [15:20] jml: yes, if that is the tool fix you're referring to [15:21] jml: it narrows the window for unreviewed revisions and in return for that you get more time for revisions to be lost [15:21] Couldn't the current tool just not land the branch if there are extra untested revisions after it finishes the test? [15:21] I think the branch should get frozen once approved [15:21] Like a failure? [15:21] noodles775: that's my preferred outcome [15:21] That way we could still use ec2 land safely? [15:22] bigjools: I do sometimes have last-minute fixes, e.g. because I forgot to check for lint. They need to be reviewed, but apart from that... [15:22] why is the 'reviewed revision' not set on our MPs? [15:22] bigjools, our partly-implemented landing queues are reasonably sophisticated, but let's talk about that offline. [15:22] jtv: well, you *could* always rs them, couldn't you? [15:23] henninge: that'd be another extra feature I guess [15:23] * henninge has never tried to approve his own MP [15:23] bac, there was no immediate win. [15:23] maybe it's not so bad if we can review our own cosmetic changes, as long as all revisions have been approved by _someone_ who is a reviewer [15:24] * abentley has approved many of his MPs, because the reviewers neglected to set the status. [15:24] i think we've opened up a can of worms here that we need to discuss more on the ML, again. [15:24] abentley: oh, I have done that, too. [15:24] let's table it for now [15:25] [topic] * Ease the "no lambdas" coding style to allow using them for Twisted callbacks. [heninge, allenap, abentley] [15:25] New Topic: * Ease the "no lambdas" coding style to allow using them for Twisted callbacks. [heninge, allenap, abentley] [15:25] This came up during a review on Monday. [15:25] heh, tabling means something different in British English :) [15:25] bigjools: like everything else [15:25] allenap submitted code for review that uses Twisted and had lines like this [15:25] d.addCallback(lambda ignored: reactor.stop()) [15:26] hmm. are use of lambdas "standard operating procedure" for twisted dev? [15:26] barry, yes. [15:26] barry, as is using 'd' for a narrowly-scoped Deferred [15:26] jml: then: when in rome... [15:26] barry, which is also verboten in our coding standard. [15:27] stupid question perhaps but is there no "bind" in python? [15:27] bac, bigjools: http://www.economist.com/research/styleGuide/index.cfm?page=673901 (go to the end of page) [15:27] d.addCallback(bind(reactor, stop)), something like that? [15:27] abentley had a good point yesterday, which is that this is not about code reuse, but about adapting functions to be callbacks. [15:27] jtv: There is functools.partial [15:28] jml: same reason we don't pep8 our method names [15:28] jtv: But that doesn't help when you want to ignore an argument. [15:28] jtv, what definition of bind would make that equivalent to 'lambda ignore: reactor.stop()'? [15:28] oic [15:30] http://paste.ubuntu.com/373295/ [15:30] LINK received: http://paste.ubuntu.com/373295/ [15:30] Ok, so do we agree that the use of lambda is acceptable in Twisted context? [15:30] * jml agrees [15:30] +! [15:30] +1 [15:30] and +1 [15:30] +1 [15:30] +1 [15:30] +1 [15:30] +1 [15:30] +1 [15:30] +1 [15:30] * barry would much rather see lambda than kerchop! [15:31] +1 [15:31] I hate for the rule to be so "arbitrary" though. [15:31] common sense applies [15:31] cool, I'll update the guideline to say so. [15:31] bigjools: now *that* is a rule I like [15:31] kerchop was certainly enough of a deterrent to sway the votes ;) [15:31] "Break any of these rules sooner than say anything outright barbarous." [15:32] [action] henninge to update guidelines to allow lambda in twisted context [15:32] ACTION received: henninge to update guidelines to allow lambda in twisted context [15:32] "Adapting functions to be callbacks" seems to make sense too though, as a more general context? - maybe that point should be said with the guideline :) [15:32] [action] bac to not use 'table' as a verb in mixed company [15:32] ACTION received: bac to not use 'table' as a verb in mixed company [15:32] [topic] * Peanut gallery (anything not on the agenda) [15:32] New Topic: * Peanut gallery (anything not on the agenda) [15:32] bac: you split an infinitive. [15:33] lol @ bac [15:33] it's allowed in southern US english [15:33] any new topic? [15:33] noodles775: well. in general, lambdas shouldn't e used for callbacks. it's just that twisted style embraces and promotes lambdas, so it's more a "fit in with twisted coding styles when necessary" rule [15:33] barry: oic [15:34] * jml disagrees w/ barry, but whatever. [15:34] jml: :) [15:34] nothing else then? [15:34] barry, it's also a matter of "writing twisted without lambdas is unnecessarily painful". [15:34] 3 [15:34] 2 [15:34] * bigjools has a quickie [15:34] abentley: true that [15:35] topic, that is [15:35] bigjools: enjoy it [15:35] bigjools: thanks for sharing [15:35] welcome [15:35] do I have the floor? [15:35] darn, did it again [15:35] In a review of mine, bac noticed that I had some long line lint in a file [15:35] that I'd changed, but not anything I'd changed myself. The file was [15:35] _schema_circular_imports.py and someone had added very long lines to hack [15:35] the return types for API exports. I want to remind people to use the [15:35] helpers in that file (e.g. patch_return_type()) because a) they're much [15:35] stay away from the table though ;) [15:35] shorter, b) it factors the hacky code. This might be worth having as [15:35] a review point if everyone agrees? [15:36] +1 [15:36] bigjools: thanks for bringing that up. i didn't know about the helper [15:36] +1 [15:36] also if you need to patch something and there isn't a helper, write one [15:36] a hearty +1 to that last point [15:37] and make an effort to convert some of the existing crack in the file if you edit it [15:37] that's it from me [15:37] [topic] ensure developers have tested API changes with launchpadlib [15:37] New Topic: ensure developers have tested API changes with launchpadlib [15:38] heh [15:38] in that same review i noted bigjools was fixing an API change that hadn't correctly updated the WADL. [15:38] had the original code been exercised locally using launchpadlib the error would've been discovered [15:39] so, encourage the use of lplib. it's easy and fun! [15:39] * bigjools wants lplib-based tests ... [15:39] is it at all possible to automate? [15:39] bac, do you mean that our tests should use the python launchpadlib library? [15:39] jml: we discussed that a long time ago and it wasn't do to the credentials problem [15:39] do we not include launchpadlib in our dependencies and generate the wadl every single build precisely because of this? [15:39] bigjools: that's actually possible iirc [15:40] flacoste: IIRC it wasn;t done because LP would have a dependency on lplib [15:40] leonardr has added new functionality to bypass the OAUTH browser dance which may make putting lplib into our test suite feasible now [15:40] bigjools, we already depend on lplib [15:40] oh?> [15:41] wait, that conflicts with your previous statement [15:42] gary_poster: perhaps foundations should revisit this issue to see if it is something we can do now [15:42] bigjools, poor phrasing on my part [15:43] * gary_poster reads back [15:43] bigjools, get rid of the "do", "not" and the question mark, and add "don't we?" to the end. [15:43] I think we can have lplib tests now. I have some httplib hacks to make it possible in one of our layers. [15:44] In any case, I was just talking to Leonard about him producing tests like this [15:44] so then people can use that as an example [15:44] gary_poster: can one of your team do a prototype? [15:44] :-) ^^^ [15:44] [action] leonardr to create an example for automated lplib tests in the launchpad tree [15:44] ACTION received: leonardr to create an example for automated lplib tests in the launchpad tree [15:44] That should be within this cycle. It will hopefully be produced as part of the webservice versioning work Leoanrd is doing [15:44] great! [15:45] \o/ [15:45] that's it, we're out of time. thanks for coming. [15:45] #endmeeting [15:45] Meeting finished at 09:45. [15:45] thanks bac! [15:45] thanks all === matsubara-lunch is now known as matsubara === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk [21:02] hi thumper, mwhudson -- reviewers meeting? [21:02] bac: hi, just getting on a call with mrjazzcat [21:03] thumper: will you be available this hour? [21:03] yes, in 30 min [21:03] ok [21:04] mwhudson: reviewers meeting at :30 [21:04] bac: ack === mrjazzcat is now known as mrjazzcat-meetin [21:31] thumper: let me know when you're available [21:31] bac: ready [21:31] #startmeeting [21:31] Meeting started at 15:31. The chair is bac. [21:31] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [21:32] hi [21:32] hi, welcome to our, uh, quarterly ASIAPAC reviewers meeting [21:32] thumper is rockstar around today? [21:32] bac: rockstar is not well today [21:33] sorry to hear that [21:33] so we had a good meeting today with the AMEU people [21:33] before we get to that, though, i'd like to chat with you two about these meetings and how to make them useful and informative. [21:34] i want you to be plugged in to what the rest of the reviewers are doing but don't want to show up here and regurgitate info about what happened at the earlier meeting. [21:34] thoughts on what you'd like to see? [21:34] well, that part is useful tbh [21:35] general agreements on change in style [21:35] or issues that other reviewers are having [21:35] it's nice to have a semi-interactive forum to disagree with the others :) [21:35] ok. i try to keep https://dev.launchpad.net/ReviewerMeetingAgenda updated with at least the highlghts [21:36] and the log files are available [21:36] cool mwhudson [21:36] well do jump in so it doesn't feel like i'm just cut-n-pasting you a wiki page. :) [21:37] I just read the outstanding actions section [21:37] one non-controversial topic jml brought up (well i brought it up on his behalf) was having reviewers land community contribution branches [21:37] seems reasonable [21:37] it seems there are a few people (jml being one) who get pinged to land stuff [21:37] well that sounds sensible yes [21:38] so we formalized the informal idea that the reviewer is to assume he'll be landing the branch [21:38] ok [21:38] and if the reviewer cannot then whoever is OCR at the time should consider it part of their duties [21:38] * thumper nods [21:38] i'll be adding something to summarize that to the wiki [21:39] other than wgrant do you have other contributors in your time zone? [21:39] not that I know of [21:39] bac: but we may end up working with some americans [21:39] we sometimes get us folks in their evenings [21:39] bac: as it is only 3h to pacific time [21:39] nothing regular though [21:40] ah, right [21:40] that topic led to the issue of 'ec2 land' having a 3-4 hour window where unreviewed and untested code can be pushed to LP and pqm will happily land it [21:41] we talked about solutions to that but nothing bubbled up to the top as a winner [21:41] the solution is to finish off merge queues [21:41] the only risk-free way to do it is to get a copy of the branch for landing [21:41] we record revision ids [21:41] thumper: and will that include integration with PQM? [21:41] bac: getting a copy is safe for now [21:42] oh right yeah [21:42] bac: we'll move to tarmac I believe [21:42] that really sucks :( [21:42] doing it manually is a bit of PITA, though [21:42] bac: although integration with pqm isn't too hard [21:42] yes [21:42] perhaps we'll get the time to finish off merge queues [21:43] that'd be sweet. in the interim we may need to look at some light tool support to make grabbing and landing a branch less painful [21:43] we've all become soft since 'ec2 land' is so easy now! [21:44] agreed [21:44] bac: or... [21:44] i guess you could do ec2 land --community that would construct the arguments from the mp, copy the branch and then ec2 test -s that [21:44] bac: we extend ec2 land to accept a revid [21:44] thumper: it needn't even do that. it could just take the current one, right [21:45] thumper: but even then, there would still be the time it languishes in PQM as a window where new stuff could push [21:45] bac: well, you'd still want the ability to specify it if an unreviewed revision has already been added [21:45] but i think just having ec2 not submit if the branch changed after it started it's work would be sufficient [21:45] bac: agreed [21:46] i've meant to do that but just haven't gotten around to it [21:46] another topic [21:46] henning, abentley, and gavin brought up the fact that twisted uses lambdas a lot for defining callback functions [21:47] after some discussion we agreed to a "when in rome" approach and decided to relax our coding guidelines to allow lambdas for that purpose when dealing with twisted [21:47] well yeah, the total lambda ban is a bit silly for this [21:47] in fact i'm fairly sure there are some lambdas in the code already for callbacks... [21:47] I agree with mwhudson [21:48] bigjools repeated his mantra of "let common sense prevail" and we moved on [21:49] +1 [21:49] bigjools also reminded everyone of the circular import patching helper. i knew nothing about it so i was glad to learn about them [21:49] patch_return_type() [21:50] makes _schema_circular_blah more readable [21:50] interesting [21:50] hadn't heard of that, so also happy to learn [21:50] ah worth knowin [21:51] i then got on a soap box to encourage reviewers to insist that API changes be tested via launchpadlib, even if only in an ad hoc fashion [21:51] which led us to talk again about getting launchpadlib tests in our test suite. gary indicated it may be possible now and leonard is supposed to create some examples [21:51] there was much celebration [21:52] I remember seeing a launchpad client in some test code [21:52] I remember thinking that that seemed like a much more sensible approach than the webservice doctests [21:53] yeah, leondard had to write an approved way to get credentials without using the browser since people were making hacks to do it anyway. while he didn't like the idea for real code it does allow us to use it for tests [21:53] i'm anxious to see this work. [21:54] me too [21:54] that pretty much sums up our meeting. [21:54] do either of you have anything new to bring up? [21:54] well, i don't think i have anything to disagree with this week then [21:54] darn [21:55] thumper you're generally good for some disagreement... [21:55] i'm glad weekly chr is going away i think [21:55] yeah, me too [21:55] I'm pretty happy right now, just tired [21:56] ok, well that's it then. i've got to race off to another call [21:56] ok, thanks bac [21:56] #endmeeting [21:56] Meeting finished at 15:56. [21:56] thanks bac [21:56] talk to you soon [21:57] see you next week...