=== Ursinha is now known as bananinha === bananinha is now known as Ursinha === Ursinha is now known as Ursinha-afk === mrevell is now known as mrevell-lunch === Ursinha-afk is now known as Ursinha === mrevell-lunch is now known as mrevell [15:00] #startmeeting [15:00] Meeting started at 09:00. The chair is barry. [15:00] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:00] hello everyone and welcome to this week's ameu reviewer's meeting. who's here today? [15:00] me [15:01] ... and apparently no-one else... [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:02] me [15:02] [TOPIC] agenda [15:02] New Topic: agenda [15:02] * Roll call [15:03] * Reviewers to enforce bug<->branch linking for all non-trivial branches (intellectronica) [15:03] * Determine official preference, if any, of Storm usage: And(expr, expr) vs. expr & expr, Gary [15:03] * Determine why _get_store uses protected spelling, and if we think it should, Gary [15:03] * Merge proposals and cover letters (barry) [15:03] * Using OCR for JavaScript reviews (mars) [15:03] * Peanut gallery (anything not on the agenda) [15:03] * Mentoring update [15:03] * Action items [15:03] me [15:03] me [15:03] intellectronica: ping [15:03] i guess we'll skip his topic for now [15:04] [TOPIC] * Determine official preference, if any, of Storm usage: And(expr, expr) vs. expr & expr, Gary [15:04] New Topic: * Determine official preference, if any, of Storm usage: And(expr, expr) vs. expr & expr, Gary [15:04] gary_poster: you're up [15:04] Storm supports both imported And/Or (e.g., ``And(expr, expr[, expr]*)``) and overloaded binary operators (e.g., ``(expr) & (expr) [& (expr)]*``). [15:04] See https://storm.canonical.com/Manual#AND . [15:04] In our codebase I see And/Or, not binary operators. [15:04] I have a branch going through review that uses the binary operators. [15:04] My opinions... [15:04] Positives for binary operators: [15:04] - no imports [15:04] - readable [15:04] Negatives for binary operators: [15:04] - a hack (they should be able to overload boolean operators but Python does not support) [15:04] - because of hack, precedence is wrong (e.g., ``Class.foo==42 & Class.bar=='kumquat'`` does not work; you need ``(Class.foo==42) & (Class.bar=='kumquat')``). [15:04] Maybe some will not find it as readable and stick "less readable" in the negatives. [15:04] So, what is status of using overloaded binary operators for combining Storm expressions? Preferred, rejected, allowed, or allowed-while-we-evaluate? [15:05] (done, in case it wasn't clear :-) ) [15:05] gary_poster: sorry i think they're less readable :) [15:06] agreed [15:06] Ditto. [15:06] gary_poster: and the precedence problem worries me [15:06] -1 on using binary operators [15:06] gary_poster: cool. [15:06] precendece is my main issue [15:06] -1 -- the precende problem can cause nightmares... [15:07] (nightmares--well, not really, at least IME--you just get a clear failure) [15:07] ok, so done? [15:07] gary_poster: i think so, sorry ;) [15:07] np [15:07] me (sorry for lateness - new wireless router) [15:07] me [15:07] intellectronica: we'll come back to your topic [15:08] [TOPIC] * Determine why _get_store uses protected spelling, and if we think it should, Gary [15:08] New Topic: * Determine why _get_store uses protected spelling, and if we think it should, Gary [15:08] intellectronica: it has some serious latency, you might want to consider going back to the old one [15:08] Last week we stated that we should not have public methods spelled as protected methods (_*). [15:08] I noticed that our SQLBase bits seem to rely on ``_get_store``. [15:08] This method can be convenient even when using Storm spelling. It is shorter to use than the full utility call, and it specifies which store and flavor to use. [15:08] I'm guessing that changing this means changing our SQLBase code? [15:08] I'm inclined to put changing this as a bug report in LP. [15:08] Agree/appropriate/possible? [15:08] (Or maybe I misunderstand about this being needed by our SQLBase stuff?) [15:08] (done) [15:10] gary_poster: have you seen abentley's branch? [15:10] is this not about storm code? [15:10] barry: no [15:10] so i asked jamesh [15:11] and he said that the only reason for having an underscore there [15:11] is that it's not part of the SQLObject API [15:11] but I don't think we care now [15:11] abentley has a new storm base class for convenience. i think jml was reviewing that but i don't know the status of it [15:11] it provides a convenient way to get the store [15:12] flacoste: i think you're right. plus we want to get rid of the sqlobject api anyway don't we? :) [15:12] well [15:12] sometime in the future [15:13] but it's a lot of work [15:13] and not that practical either [15:13] and the bare storm API is a pain [15:13] heh [15:13] so SugarStorm is nice [15:13] we can do it piecemeal too right? [15:13] more or less [15:13] right, that's what i'm thinking [15:13] because of the incompatibility between the resultset interface [15:13] flacoste, bare Storm needs improvements, but if we avoid those improvements, that's a tragedy. [15:13] and we use these for unions [15:14] i have great hope for abentley's SugarStorm [15:14] or watherver it's called [15:14] agree with rockstar. If this is a Storm thing, then let's change this in Storm. This seems like it should go lower than SugarStorm [15:14] we should probably make it easy to combine SQLObject and Storm ResultSet [15:14] so that we aren't block by the incompatibility [15:15] because when you start converting one object [15:15] flacoste: +1 if possible [15:15] which uses UNION [15:15] it becomes an never-ending tasks [15:15] I once tried converting Person to use only the Store API [15:15] and gave up because the branch was inflating faster than Argentina's inflation [15:15] heh [15:15] :) [15:16] flacoste: 70 hits for 'union|UNION' in lib/c/l/database [15:16] I got caught by the union thing [15:16] would be great to fix that [15:16] flacoste: well, person might not be the best first choice :) [15:16] well, i was there [15:16] and it started fine :-) [15:16] :-D [15:16] and needed to use storm for some thing [15:16] anyway [15:17] ( well, the Person anecdote was intended as an example of Storm API not being as convenient, right?) [15:17] so, back to gary's specific question: what to do about _get_store()? [15:17] gary_poster, flacoste, will we still want obj.get_store() after stub's landed his sso branch? it includes some IStore adapters for our classes [15:18] salgado: if we *have* to have these helper static methods anyway, and they are so easy to use, shouldn't we just use them? [15:18] herb: Okay, can you apply https://pastebin.canonical.com/14232/ and then apply https://pastebin.canonical.com/14233/ please? [15:19] Heh. [15:19] gmb: hee hee [15:19] Wrong channel. [15:19] salgado: I certainly prefer class methods over zope stuff where either solution works. [15:19] agree [15:19] fair enough. I prefer that too [15:20] so to change this is this in our code base or in Storm? flacoste ? [15:20] 'cause if it is in Storm, may or may not happen ... [15:21] gary_poster: niemeyer was not receptive to my suggestions. [15:21] abentley: yes, definitely observed === Ursinha is now known as aham === aham is now known as Ursinha [15:22] abentley: did jml finish the review of your branch? [15:22] barry: No. He asked for changes, I submitted a revised version, then nothing. [15:22] abentley: i'll ping him tonight about it [15:22] abentley: but you should too :) [15:23] barry: I've got plenty of other stuff to work on, so I'm not fussed. [15:23] gary_poster: have we resolved your topic? ;) [15:24] barry: not really, unless we say that, because there wasn't much clamoring of agreement with me that this ought to be changed, we should just ignore it. [15:24] but then, can I use _get_store? [15:25] gary_poster: you can always add an alias for get_store() and use that [15:25] My branch does. flacoste thouht it was reasonable at the time. [15:25] e.g.: get_store = _get_store [15:25] sounds good to me [15:25] should it not be getStore(), though? [15:25] and we tack that on to all our classes? [15:25] salgado: dang, yeah :/ [15:26] gary_poster: you had another topic? [15:26] heh, yeah. one sec, lemme get the paste [15:26] I miss the old bzr review plugin a bit because it enforced our policies. [15:26] In particular, the lint is not being used regularly, and the information about pre-impl call is often missing in reviews I've done. [15:26] I have a quick example about the ``make lint`` issue. [15:26] I noticed when I had to manually switch my most recent branch over from devel to db-devel, and merge, that ``make lint`` had apparently not been run for many of the branches. [15:26] I had to do a lot of cleaning up on the merged code to make ``make lint`` happy. The majority of the complaints were real. [15:26] We're building generic bzr tools AIUI (bzr send), but our policies are not generic, and enforcing them is valuable. [15:26] I'm inclined to want a lp-specific bzr plugin that defers the heavy lifting to the bzr send plugin code, but is more specific to what we want. [15:26] I also wonder if ec2test could help with the linting, but that's not as clear to me. [15:27] The general idea would be that, if you use -s to submit to PQM, it would make sure that ``make lint`` was happy. [15:27] However, with the old review plugin, there was a way to say "yeah, yeah, make lint is on crack for that complaint." We would need that here too. [15:27] We could put it in command line options but would prefer to have it in a file. Not sure where the file would go. But that's an implementation detail. [15:27] So: IRT my general observation of having regressed a bit in our process because of missing automation for linting and message regexes: agree/disagree? [15:27] And then furthermore, if you agree that we have regressed, what should we do to improve? [15:27] (done) [15:28] can we fix lpreview to just use lpsend? [15:28] There's no reason we can't hack the existing review plugin to do lint + cover letter and then hand off to bzr send. [15:28] Well, I don't *think* there's any reason, anyway. [15:28] gary_poster: that dovetails with my topic. if we used the standard cover letter template for mps at least people would have to willfully ignore linting and pre-imps [15:28] gmb: that's the kind of thing I'm talking about, yeah [15:28] gmb: great minds think alike [15:28] abentley would know better than I. [15:28] gmb: Yes there is. There's no way to specify a message body [15:29] abentley: That's what I was afraid you were going to say. [15:29] gmb: It's not that 'bzr send' couldn't, it's that it's not implemented yet. [15:29] abentley: i wonder if there was some $EDITOR hack we could use for that [15:30] barry: send just passes over to the MUA, so I'm not sure that would work. [15:30] barry: If we want to force everyone to use mail_client=editor, perhaps. [15:30] abentley: yeah, that's what i meant. and it would just be for lp branches, so location.conf could override [15:30] abentley: i use gmail, and so i benefit from jamesh's plugin. having to use EDITOR will make like hard for me [15:30] abentley: not that i have any concrete ideas. just wonderin' [15:31] I would disable such a feature because I start my cover letter when I create my branch [15:31] barry: yeah, some kind of automated/enforced-ish standard template would be fine with me too. It's not as tight as the lpreview approach, but something is better than nothing, and might be sufficient [15:31] it'd be nice for lpsend to actually work with my name first, I have to manually paste the cover letter anyway [15:31] sinzui: so do i [15:31] danilos: Work with your name? [15:31] danilos: +1 :) [15:31] so the temolate thing seems like an insufficient idea [15:32] danilos, +1 as well :) [15:32] abentley: some of us get a unicode error when using lpsend [15:32] BjornT: Not from send, from Launchpad, right? [15:32] abentley: right [15:32] abentley: right [15:33] So that's not a send bug, and AIUI, work has been done to fix it this cycle. [15:33] I've said before, but I guess I have to say again, I think bzr send should support a hook that we can use to plug in the process stuff from lpreview. [15:34] abentley: what are you waiting for? :) [15:35] abentley: (I don't remember hearing that before) +1 [15:35] barry: a free moment. [15:35] abentley: those are so overrated [15:35] ok, so maybe this is another one of those "...and we descend into silence" moments, this time because we agree that something needs to be done but don't have the time. [15:35] abentley: Is there a bug for updating send to add that hook? [15:36] gmb: I don't know. [15:36] gary_poster: yep. seems like time is better spend on send rather than lpreview though, so that's something [15:36] gary_poster, a very astute observation :) [15:36] :-) [15:36] gary_poster: will you check to see if there's a bug open on adding a hook for bzr send? [15:36] barry: ok [15:37] [ACTION] gary_poster will check to see if there's a bug open for adding a hook to 'bzr send' and submit one if there isn't [15:37] ACTION received: gary_poster will check to see if there's a bug open for adding a hook to 'bzr send' and submit one if there isn't [15:37] barry: IRT my previous topic: sorry, but I was not clear. so can I just use _get_store for now, and move on? [15:37] gary_poster: add a getStore() alias if possible and use that [15:38] ...ok [15:38] thanks [15:38] sure, thanks everybody [15:38] [TOPIC] * Merge proposals and cover letters (barry) [15:38] New Topic: * Merge proposals and cover letters (barry) [15:38] so along the lines of gary_poster 's last topic... [15:38] i notice that not everyone is using the standard cover letter template. some people don't use any template [15:39] should we enforce that? (updating the template if necessary) [15:39] * mars hides [15:39] * gary_poster hides [15:39] not blaming anyway, just wondering! [15:39] * danilos hides behind gary [15:39] :-) [15:39] lol [15:39] * BjornT doesn't know where the standard template is located [15:39] if the template isn't useful or overkill, let's say so [15:39] I don't know where it is either, though [15:39] I don't think we should enforce a template. There are lots of times it doesn't make sense. [15:40] https://dev.launchpad.net/QuickCoverLetterTemplate [15:40] i think we should get bzr send to use the template. then people will automatically use it, unless they explicitly choose not to [15:40] well, the diff output could be removed now [15:40] BjornT, +1 [15:40] abentley: i think the template usually makes sense though [15:40] a new plugin for bzr send that adds a --template option? [15:41] I must admit, I kind of like using lp TTW to request reviews [15:41] but you know, if "we don't do that" that's ok [15:41] gary_poster: you could still use the template :) [15:41] but seems a bit of a shame, since we have a TTW interface [15:41] gary_poster, that's a different problem - communicating a project's review preferences through the web API [15:41] barry: I don't. I find I'm usually writing the same thing under several headings. [15:42] oops, nix the 'API' from that last statement [15:42] barry: sure...which is where? and how is policy for *that* plugged in? [15:42] (I mean, in the TTW interface) [15:42] oh sorry [15:42] abentley: really? maybe there's some confusion about what's intended for each section? [15:43] i preferred the old template [15:43] and i still use it [15:43] gary_poster: well, i just mean that you could paste the cover letter into the text area [15:43] i run bzr write-cover-letter [15:43] Well, a bit, but it generally just gets in the way of a natural description of the change. [15:43] and then :r cover.txt [15:43] flacoste: Me too. [15:43] when bzr send fires my editor [15:43] Well, for non-trivial branches, anyway. [15:43] :r ? [15:43] mars: i see you aren't enlightened [15:43] vim powers! [15:43] mars: it's likely just some vi crap, ignore it :) [15:44] lol [15:44] ah [15:44] dang, we're almost out of time. let's drop this for now because i'd like to give mars's topic a shot [15:44] [TOPIC] * Using OCR for JavaScript reviews (mars) [15:44] New Topic: * Using OCR for JavaScript reviews (mars) [15:44] barry, I'll be brief [15:44] mars: thx [15:45] Hi everyone [15:45] Last Monday the AJAX team decided to expand the scope of JavaScript reviews in Launchpad. === Ursinha is now known as ra === ra is now known as Ursinha [15:45] Any OCR should be able to pick up a JavaScript review, if they feel comfortable doing so. We just ask that the review be unofficially mentored by one of the sprint attendees. [15:45] These people were at the AJAX sprint, or on the AJAX team, and can mentor a JavaScript review: flacoste, mars, sinzui, Edwin, cprov, noodles, intellectronica, rockstar, danilo, and jtv [15:45] Also, feel free to ask me if any questions come up during the review. [15:45] that's all [15:45] * allenap discovers that TTW means Table-Top Wargaming [15:46] lol [15:46] Through-the-web was how I've heard it used, and my intent :-) [15:46] I thought it was ruder [15:46] mars: thanks [15:47] any comments on mars's request or should we break? [15:47] I don't qualify to be a YUI/Windmill reviewer. I just abandoned using windmill to test YUI + GMap. I'm back to despising YUI for reinventing everything to make mashups nigh impossible. [15:47] write it down someplace [15:47] Sounds like a good request. [15:47] barry: mention my item, since i don't think it requires much discussion :) [15:47] sinzui, fair enough [15:47] intellectronica: ah, sorry [15:47] PS. I hate GMap too [15:47] [TOPIC] * Reviewers to enforce bug<->branch linking for all non-trivial branches (intellectronica) [15:47] New Topic: * Reviewers to enforce bug<->branch linking for all non-trivial branches (intellectronica) [15:47] sinzui, I would love it if you could write a ranting email about that statement though [15:48] intellectronica: +1 :) [15:48] intellectronica: (1) I assume up mean --fixes=lp:XXX? [15:48] (2) how do we check that? [15:48] gary_poster: or you can do it TTW :) [15:48] gary_poster: either that, or you can do it ttw [15:48] ah ok [15:48] heh [15:48] heh [15:48] :-) [15:49] mars: I was going to bring it up in the meeting, but my thoughts are not collected. [15:49] and you can check it ttw, or do it after the fact [15:49] gary_poster, require a hook in pqm-submit, --no-fixes has to be explicitly set for this branch? [15:49] kinda like explicitly setting --no-lint [15:49] okay, so apologies for going over. thanks everybody and have a good day [15:49] mars: the thing is, --fixes is on commit, not on pqm submit [15:49] thanks barry [15:49] #ENDMEETING [15:49] Meeting finished at 09:49. [15:50] intellectronica: Not every branch related to a bug fixes that bug. Some are incremental branches. [15:51] abentley: i agree. but they can still be linked to the bug [15:51] intellectronica: They can't be --fixes. [15:51] sinzui, ok, but do let me know when you have something put together. I'd love to read it. [15:51] abentley: right, but they can be linked ttw with a comment explaining what is their relation to the bug [15:52] mars I certain will === ursula is now known as Ursinha === Ursinha is now known as uia === uia is now known as Ursinha === ursula is now known as Ursinha