=== Edwin is now known as Guest66225 [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:01] me [15:01] me [15:01] me [15:01] hi who is here? [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:01] me [15:02] me [15:02] me [15:03] [topic] agenda [15:03] New Topic: agenda [15:03] * Roll call [15:03] * Agenda [15:03] * Outstanding actions [15:03] * New topics [15:03] * Mentat update. [15:03] * Some of the UI reviewers have graduated, but ReviewerSchedule indicates that no one has graduated. Same for UI/Reviews, which still lists people who've left the team. [15:03] * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews? [15:03] * Assertion errors and the webservice (gary) [15:03] * Peanut gallery [15:04] [topic] outstanding actions [15:04] New Topic: outstanding actions [15:04] bac and lifeless to chat about branch size. [15:04] last week in the AsiaPac meeting rockstar brought up the suggestion that we lower the limit on the diff size for reviews. [15:04] that led to a really good discussion. [15:05] U1 is 500 lines [15:05] me [15:05] lifeless made the point that diff size is a bad proxy for amount of work and suggested we think about it differently [15:05] bac, isn't there already a diff size of 800? [15:05] I would rather say 'Have no more than N changes in your thing to be reviewed' [15:05] [01:28] where we might say N is - 4. [15:06] hard to count [15:06] abentley: the current soft limit is 800 lines of diff [15:06] abstract [15:06] difficult to communicate and agree on [15:06] prefer soft 800 with negotiation, myself [15:06] +1 [15:06] let's not go over this again [15:06] gary_poster: his point was if in writing up a good cover letter you would notice your branch is unfocused if you have a bunch of things to mention [15:07] i think too we should keep a "soft 800 lines" limit [15:07] me too [15:07] bac: is the proposal about something where I rename a class (1 action) and do a find and replace creating a 3000 line diff? This is 1 change and is good. But adding a schema, mode, vocab, view, template is 5 changes and bad? [15:07] point being, he wants to discuss it with me to come up with a solid proposal. so let's not dive into it now. [15:08] I'm happy to include that as general advice, bac. ...ok, fair enough [15:08] I think we should use common sense and if the reviewer rejects it, so be it [15:08] just think about the idea of how to measure the *focus* of a branch rather than just the size, although the latter is much easier to determine [15:08] me too with bigjools, but if this is just an alert of a conversation that bac and lifeless will have then...I guess we can move on? [15:08] gary_poster: yes, lets [15:09] cool [15:09] [topic] mentat update [15:09] New Topic: mentat update [15:09] 800 max is okay, but I find that smaller diffs are much, much easier to review. I'd rather read 5 200 line diffs than 1 800 line diff. [15:10] stevenk and thumper say the mentoring is going very slowly. i'm not sure how we can improve that, given the time zone differences. perhaps if you have a non-time critical MP you can assign it to stevenk to help him get more reviews [15:10] [topic] * Some of the UI reviewers have graduated, but ReviewerSchedule indicates that no one has graduated. Same for UI/Reviews, which still lists people who've left the team. [15:10] New Topic: * Some of the UI reviewers have graduated, but ReviewerSchedule indicates that no one has graduated. Same for UI/Reviews, which still lists people who've left the team. [15:11] i'm unsure who put this on the agenda last week (please put your name on items) but rockstar has updated the wiki [15:11] it looks like lifeless updated the page. Are there any other graduated UI reviewers besides noodles775, sinzui, and rockstar? [15:11] no. [15:11] henninge, is in the mentat [15:12] I am [15:12] does anyone else want to become a UI reviewer [15:12] salgado asked about becoming a ui reviewer so i referred him to rockstar [15:12] It would be great to get another ... :) [15:12] noodles775, would you like to be my mentor? ;) [15:12] we need more, since one of them is leaving :( [15:12] who is leaving? [15:12] salgado: yeah, I'll be moving to ISd sometime... [15:12] * noodles775 needs to send email out. [15:13] :-( [15:13] oh, :( [15:13] glad you are still around-ish though :-) [15:13] noodles775: :( [15:13] Yep, still around :) [15:13] noodles775: :( [15:14] who is mentoring henninge? [15:14] perhaps the other can help salgado [15:15] I can mentor salgado [15:15] thanks sinzui [15:15] moving on... [15:15] yay! [15:15] thanks sinzui [15:15] [topic] * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews? [15:15] New Topic: * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews? [15:15] lifeless wants us to read that page and the attached presentation. [15:15] bac: rockstar is mentoring me [15:15] henninge: thanks [15:16] and for reviewers to try to think more about architectural issues when reviewing. [15:17] i'll ask him tonight to put his thoughts into an email wrt reviewing. [15:17] [topic] Assertion errors and the webservice (gary) [15:17] New Topic: Assertion errors and the webservice (gary) [15:17] preparing paste 1 sec [15:18] We are currently using assertions to communicate errors that have more appropriate specific exceptions. [15:18] lib/lp/registry/model/product.py:720-ish [15:18] if license not in License: [15:18] raise AssertionError("%s is not a License" % license) [15:18] LookupError? [15:18] lib/lp/answers/model/faq.py:184, and below [15:18] if search_text is not None: [15:18] assert isinstance(search_text, basestring), ( [15:18] 'search_text should be a string, not %s' % type(search_text)) [15:18] ValueError/TypeError? [15:18] Those are from benji, after a conversation he and I had in regards to bug 413174. He has others. [15:18] If it's not already the case, I'd like to suggest that we only use assertions for communicating exceptions that should never happen, within the current code structure. [15:18] This is particularly important for the webservice. For the bug I mentioned, benji and I decided that it would be better to have a whitelist approach for exposing errors to webservice users. He has a branch that will land with this soon, and we'll describe it later. [15:18] Launchpad bug 413174 in Launchpad Foundations "API AssertionError could be adapted to a http code (affected: 1, heat: 7)" [High,Triaged] https://launchpad.net/bugs/413174 [15:18] But for now...that's my assertion suggestion. [15:18] "within the current code structure": like, method, or function. [15:19] thanks gary_poster [15:19] gary_poster, the case I use assertion errors is when the passed object is in the wrong state [15:19] Is that a valid use? [15:20] I think that's a ValueError myself. valid is in the eye of the beholder, but I'd like for us to agree for the LP code base [15:20] gary_poster, okay [15:20] something to remember about assertions is that when the interpreter is run with -O, they will be no-ops; so don't use an assertion for any check that shouldn't be optimized away [15:21] we should always return a decent error to the webservice caller, surely? an exception without a webservice_error files an OOPS. [15:21] benji: +1, as that's easy to forget. [15:21] benji, that is why we strive to use raise AssertError. I never accept the other form in a review [15:21] (although I don't think "raise AssertionError()" is optimized away, right?) [15:21] sometime the engineer drops the assertion because of that [15:21] right, sinzui [15:21] right, raise AssertionError() is not removed [15:22] gary_poster, In the example of deleting a milestone. If the state is bad, the method could return (false, message) [15:22] bigjools: yes, we want to do that. benji's branch is a way to do that. some exceptions are not OK to pass back to the webservice user. It's a security audit question. I like whitelists for that. [15:22] so instead of a ValueError, a success state could be returned [15:23] sinzui, the ValueError, in benji's branch, has the REST/webservice advantage that it returns a 400, plus whatever content is in the exception instantiation. [15:23] okay [15:24] gary_poster: thanks for bringing it up. i look forward to seeing benji's work [15:24] cool, thanks bac. [15:24] [topic] other stuff? [15:24] New Topic: other stuff? [15:24] just a little thin [15:24] g [15:25] I think [15:25] Who should set an MP to "approved"? [15:25] heh, good workflow question for MP design generally, I think. [15:25] henninge: ideally, the last required reviewer [15:25] I have been asked as a reviewer to do it but I think it is the responsibilty of the branch owner to determine when the branch has had enough reviews. [15:26] henninge: but it is so easy to forget [15:26] tarmac will make it easier, IIUC [15:26] because that will be the cue to try and land (again, IIUC) [15:26] gary_poster, that's not something I've heard. [15:26] henninge: if the dev already knows he needs more reviews he should add them to the MP [15:26] abentley: you know more than I about it, I'm sure [15:26] gary_poster, oh, you mean it will become necessary with tarmac. [15:27] yes [15:27] bac: does not happen very often AFICR [15:27] and it will have a more obvious meaning [15:27] A [15:27] right now it's almost pointless from a practical perspective [15:27] gary_poster, yes, in the current iteration. There is ongoing work to split the review state from the merge state. [15:27] I mean, people add a UI or r-c review after the code review [15:27] ah, ok, cool [15:28] If we want, we can write a script that will approve things when the criteria are met. Then we'd have to decide the criteria. [15:28] henninge: if i think it needs a UI or text review, i suggest that in the code review i do or on irc. [15:28] I'd almost prefer for the MP to do this for me. If I say I need a release-critical review, a code review, and a ui review, then I can make that gesture in the MP [15:28] s/almost// [15:29] (where abentley's script would be one implementation of what I'm talking about, yeah) [15:29] gary_poster: i think MPs purposely didn't include a bunch of specific behavior b/c other projects have different policies [15:30] gary_poster: yeah, that could be overcome by a script [15:30] bac: sure but it is still the responsibilty of the one seeking approval to make sure the branch gets all required reviews. [15:30] gary_poster, variations in approval criteria are a significant obstacle to that. [15:30] so, setting an MP to "approved" just docuemnts that he followed through on that. [15:30] gary_poster, which is why doing it on a per-project basis is recommended. [15:30] bac, abentley, gotcha. [15:30] gary_poster, i.e. with a script. [15:30] right [15:31] henninge, but it can be the approval-seekers responsibility still with automation, yes? [15:31] By requesting more reviews [15:31] gary_poster: I am not opposed to automation at all! ;) [15:31] ok :-) [15:32] gary_poster: oh, misread that. [15:33] gary_poster: you mean, by requesting more reviews an mp would go back to "needs review"? [15:33] I was just about to propose that we had consensus, darn :-) [15:33] yes [15:33] fine, we do have a consensus ... ;) [15:33] heh, ok [15:33] henninge: so i don't think we need a strict policy here. the review is a conversation. please communicate to the dev if you have concerns [15:33] henninge: ok, to move on? [15:34] consensus: [15:34] - approval seeker is of ultimately responsible for the review [15:34] bac: I was not asking for a policy, just a reminder that you should not wait for the reviewer to change that state - just do it yourself. ;-) [15:34] oh [15:34] that's even easier I think [15:34] henninge: ok [15:34] ok, /me is quiet :-) [15:35] any other topic? [15:35] 3 [15:35] 2 [15:35] gary_poster: that does not contradict what you were just saying ... ;) [15:35] 1.5 [15:35] bac: continue ;) [15:35] 1 [15:35] aggred henninge :-) [15:35] ree [15:35] #endmeeting [15:35] Meeting finished at 09:35. [15:36] thanks y'all [15:36] * bac practices for dallas [15:36] bac: you need to practise? [15:36] bigjools: yeah, not really [15:36] :) [15:36] :-) thanks bac [15:36] thanks bac [15:37] thanks bac [15:37] thanks gary_poster ;) [15:37] :-) thank you === Ursinha is now known as Ursinha-lunch === salgado is now known as salgado-lunch === EdwinGrubbs is now known as Edwin-lunch === Ursinha-lunch is now known as Ursinha === benji is now known as benji-lunch === benji-lunch is now known as benji === salgado-lunch is now known as salgado === Edwin-lunch is now known as EdwinGrubbs === salgado is now known as salgado-afk === Edwin is now known as Guest38361 === Ursinha is now known as Ursinha-afk