=== mrevell_ 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 reviewers meeting. who's here today? [15:00] me [15:00] me [15:00] me [15:01] me [15:02] me [15:02] me [15:02] adeuring, bac ping [15:02] me [15:02] me [15:02] bigjools, BjornT ping [15:02] danilo_: ping [15:02] me, sorry for the delay [15:02] me [15:02] gmb: ping [15:03] rockstar, salgado, sinzui ping [15:03] me [15:03] [TOPIC] agenda [15:03] New Topic: agenda [15:03] * Roll call [15:03] * Action items [15:03] * Mentoring update [15:03] * Peanut gallery (anything not on the agenda) [15:03] mo [15:03] me [15:03] pretty light day today (planned at least) [15:04] [TOPIC] action items [15:04] New Topic: action items [15:04] me [15:04] * danilo to look into storm/sqlobject result set compatibility [15:04] is danilo_ here today? flacoste do you have any status on this? [15:04] me [15:04] danilo? [15:04] that was allenap [15:04] backporting the patch he landed on storm trunk [15:05] I forgot :( [15:05] flacoste: oops! [15:05] me [15:05] Please assign to me and I'll try and do it this time :) [15:05] allenap: done, thanks [15:05] * 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:06] no sorry [15:06] will put back on my list [15:06] gary_poster: cool, thanks [15:06] * flacoste to work on API reviewer cheat sheet [15:06] gary_poster, barry, abentley has a branch that he's been working on the forever. [15:06] rockstar: ok cool, I'll ask him about it [15:06] barry: no progress since last week [15:06] np [15:06] barry: keep it up, i like this weekly "i suck" reminder :-) [15:06] :-D [15:06] :-) [15:07] good for keeping the ego in leash [15:07] gary_poster: I've done the support for specifying a body when sending, but not adding the hook. [15:07] abentley: cool. I'm guessing it's not on your immediate plate to finish it up? [15:07] gary_poster: Right. [15:08] understood [15:08] thanks [15:08] [TOPIC] mentoring update [15:08] New Topic: mentoring update [15:08] barry: Did you get anywhere with adding body support to claws? [15:08] abentley: do you happen to know if there's a bug for this, and what it might be, if so? [15:08] gary_poster: I'm not aware of a bug for this. [15:08] abentley: i didn't :( [15:09] abentley: ok cool. [15:09] barry: one to-do item scratched off ;-) ...but maybe needs to be replaced with another. [15:09] gary_poster: i'll cross it off and you can let me know if you want another [15:09] barry: cool thanks [15:10] am i right that only stub is still being mentored? [15:11] i should ping a few folks to see if they're ready to be mentats [15:11] i don't think there's many folks left who aren't revewiers [15:11] anyway... [15:12] [TOPIC] peanut gallery [15:12] New Topic: peanut gallery [15:12] do you guys have anything not on the agenda? [15:12] I am wondering if we have a policy on redundant assertions? [15:12] e.g. if foo; else: assert not foo [15:13] abentley: i thought the policy is that we always do this [15:13] but maybe it's not documented anywhere? [15:13] intellectronica: we always have an else clause if there's an elif [15:13] but if there's no elif we don't need an else+assert [15:13] oh, right [15:13] Why would we *always* do that? [15:13] abentley's example seems to be different than what barry showed intellectronica [15:14] abentley: doesn't make sense to me [15:14] Yea, the else+assert seems a little silly, as though assert would catch something the if would not. [15:14] abentley: do you have an example in our code? [15:14] BjornT: It was something I reviewed on Monday. [15:14] yes, sorry, i got confused. if there's only one if there's probably no need to do this, unless it's imprtant that the code fails at this point [15:15] https://code.edge.launchpad.net/~salgado/launchpad/bug-358498/+merge/5496 [15:16] abentley: the one on line 16 can just as well be raise AssertionError(...) [15:16] in this specific case I wanted the assertion because the 'elif' block should be removed soon, at which point the assertion will become non-redundant [15:16] and that does fall within our guidelines [15:16] barry: It's unreacable code. [15:16] abentley: proven by the assertion? [15:16] barry: It can equally well be raise IAmSuperman [15:17] abentley: the one on line 13 seems okay, but should have a message [15:17] barry: It just seems like the wrong use of asserts-- proving that Python isn't buggy. [15:17] abentley: ok. i would replace that assert with a raise AssertionError [15:17] salgado: you're saying the whole 8-14 block will be removed eventually? [15:18] abentley: sorry, i was talk about the one in the else clause (first) [15:18] i.e. line 16 is fine [15:18] line 13, yeah, i suppose. doesn't bother me too much either way [15:19] barry: I thought asserts were for proving your own state wasn't buggy. [15:19] abentley: yes, or that the state you're assuming to be the case actually is the case [15:19] Anyhow, I'll continue to say "I think this is redundant, but keep it if you like." [15:19] abentley: +1, for the line 13 assert [15:20] oh, yeah, line 16 isn't a change [15:20] but still. or whatever. [15:20] barry: line 13 can't be reached because of line 6. [15:20] abentley: thanks [15:20] abentley: yep === salgado_ is now known as salgado [15:21] * barry agrees with abentley when he says: Anyhow, I'll continue to say "I think this is redundant, but keep it if you like." [15:21] oh, we're talking about line 13. yes, that one should be removed [15:22] well, yes, but salgado said that he will soon reorganize the code, so leaving this as a"reminder" is OK, IMHO [15:22] agreed. maybe the assertion message (which salgado will add ) should state something to that effect? [15:23] maybe. :) [15:23] abentley: thanks for bringing this up [15:23] anything more on this? [15:23] barry: np [15:24] does anybody else have anything not on the agenda? [15:25] 5 [15:25] 4 [15:25] Happy Tax Day! [15:25] rockstar: "happy"?! [15:25] 3 [15:25] 2 [15:26] 1 [15:26] #endmeeting [15:26] Meeting finished at 09:26. [15:26] bye [15:26] thanks everyone! [15:26] thanks barry [15:26] thanks barry [15:26] see ya back at the ranch === danilo_ is now known as danilos === salgado is now known as salgado-lunch === salgado-lunch is now known as salgado === ursula_ is now known as Ursinha [23:31] here [23:31] hi [23:32] #startmeeting [23:32] Meeting started at 17:32. The chair is barry. [23:32] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [23:32] hi jml, thumper. are you here? [23:32] here [23:32] barry: like you wouldn't believe. [23:32] not that i have a big agenda [23:33] [TOPIC] agenda [23:33] New Topic: agenda [23:33] * Roll call [23:33] * Action items [23:33] * Mentoring update [23:33] * Peanut gallery (anything not on the agenda) [23:33] [TOPIC] action items [23:33] New Topic: action items [23:33] thumper: just one for you, an old one i think [23:33] * thumper to open bug on `webservice` pagetests globs problem [23:33] hmm [23:33] I don't recall doing it [23:34] do you still want to? :) [23:34] I should do it [23:34] i can keep it on the agenda [23:35] [TOPIC] * Peanut gallery (anything not on the agenda) [23:35] New Topic: * Peanut gallery (anything not on the agenda) [23:35] you guys have anything? [23:36] bug 362032 [23:36] Launchpad bug 362032 in launchpad "webservice glob is an admin user" [Undecided,New] https://launchpad.net/bugs/362032 [23:36] I don't really have anything [23:36] thumper: thanks [23:36] jml? [23:38] jml: i guess not [23:38] i have one issue that came up today in a review: [23:38] * don't return unwrapped objects from view methods, barry and gary ("the fightin' arries!") [23:39] back [23:39] computer crapped out [23:39] i agree with the principle. gary wants to go further and prevent view modules from importing removeSecurityProxy [23:39] jml: no worries [23:39] barry: what do you mean? [23:39] what did I miss? [23:39] jml: not much. do you have any agenda items? [23:39] I disagree with not allowing removeSecurityProxy at all [23:40] but it's uses should be well defined [23:40] just one re mentoring. [23:40] thumper: can you state why? [23:40] jml: we'll come back to that [23:40] not off the top of my head [23:40] I'd have to look at the situations where we use it [23:40] and why we did it that way [23:41] thumper: think about it, you don't have to answer now [23:41] barry: so, I'm not against adding a rule wrt removeSecurityProxy but... [23:41] it's not going to help that much :) [23:41] why not? [23:41] two reasons [23:42] for a start, it's actually really easy to return non-wrapped objects, particularly if they aren't db objects [23:42] even without rSP [23:42] the issue goes more deeply into the model code [23:43] e.g. up until recently, all SourcePackage objects had no security proxy at all. [23:43] second, the purpose of the importfascist is unclear [23:44] and so a prohibition there will last exactly as long as it takes for people to need removeSecurityProxy in view code and to forget exactly why it is that we don't allow it. [23:45] btw, fwiw, i think rSP in view code is fine under the right circumstances [23:46] so, what are the reasons for not returning unwrapped objects from views? [23:47] I'm actually happy for "not returning unwrapped objects from views" [23:47] keep the unwrapped objects in a defined scope [23:47] jml: it's a vague (to me) feeling that we could leak private data [23:47] however I'm against "don't allow the unwrapping of objects in views" [23:48] barry: I think the real problem there is that it's difficult to audit any given page. [23:49] jml: agreed. thumper agreed [23:49] barry: in some ways, rSP makes it easier to audit :) [23:49] i'm definitely against unwrapping in model code. so where else would you do it? i don't like having to add an artificial layer in there [23:49] jml: true [23:50] so, I guess I'd say "whatever". We've already got quite a few well-intentioned for-your-own [23:50] -good rules. One more won't break _this_ camel. [23:51] jml: cool, this is an interesting discussion anyway. we can move on and we'll bring it up at the next ameu [23:51] [TOPIC] mentoring update [23:51] New Topic: mentoring update [23:51] o hai [23:52] stub's doing a great job, and I see no reason not to promote him. [23:52] beauty. i'll make it official, thanks [23:52] np [23:52] i think he's the last mentat atm [23:53] well, that's all i've got. anything else from you two? [23:53] nope [23:53] I'm cool. [23:53] #endmeeting [23:53] Meeting finished at 17:53. [23:53] thanks guys [23:53] barry: thank you