[15:00] <bac> #startmeeting
[15:00] <MootBot> Meeting started at 09:00. The chair is bac.
[15:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[15:01] <jelmer> me
[15:01] <EdwinGrubbs> me
[15:01] <gary_poster> me
[15:01] <bac> hi who is here?
[15:01] <sinzui> me
[15:01] <adeuring> me
[15:01] <leonardr> me
[15:01] <henninge> me
[15:01] <noodles775> me
[15:01] <bigjools> me
[15:02] <abentley> me
[15:02] <deryck> me
[15:03] <bac> [topic] agenda
[15:03] <MootBot> New Topic:  agenda
[15:03] <bac> * Roll call
[15:03] <bac>  * Agenda
[15:03] <bac>  * Outstanding actions
[15:03] <bac>  * New topics
[15:03] <bac>   * Mentat update.
[15:03] <bac>   * 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] <bac>   * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews?
[15:03] <bac>   * Assertion errors and the webservice (gary)
[15:03] <bac>  * Peanut gallery
[15:04] <bac> [topic] outstanding actions
[15:04] <MootBot> New Topic:  outstanding actions
[15:04] <bac> bac and lifeless to chat about branch size.
[15:04] <bac> last week in the AsiaPac meeting rockstar brought up the suggestion that we lower the limit on the diff size for reviews.
[15:04] <bac> that led to a really good discussion.
[15:05] <sinzui> U1 is 500 lines
[15:05] <gmb> me
[15:05] <bac> lifeless made the point that diff size is a bad proxy for amount of work and suggested we think about it differently
[15:05] <abentley> bac, isn't there already a diff size of 800?
 I would rather say 'Have no more than N changes in your thing to be reviewed'
[15:05] <bac> [01:28] <lifeless> where we might say N is - 4.
[15:06] <gary_poster> hard to count
[15:06] <bac> abentley: the current soft limit is 800 lines of diff
[15:06] <gary_poster> abstract
[15:06] <gary_poster> difficult to communicate and agree on
[15:06] <gary_poster> prefer soft 800 with negotiation, myself
[15:06] <bigjools> +1
[15:06] <bigjools> let's not go over this again
[15:06] <bac> gary_poster: his point was if in writing up a good cover letter <cough> you would notice your branch is unfocused if you have a bunch of things to mention
[15:07] <adeuring> i think too we should keep a "soft 800 lines" limit
[15:07] <henninge> me too
[15:07] <sinzui> 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] <bac> 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] <gary_poster> I'm happy to include that as general advice, bac.  ...ok, fair enough
[15:08] <bigjools> I think we should use common sense and if the reviewer rejects it, so be it
[15:08] <bac> 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] <gary_poster> 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] <bac> gary_poster: yes, lets
[15:09] <gary_poster> cool
[15:09] <bac> [topic] mentat update
[15:09] <MootBot> New Topic:  mentat update
[15:09] <abentley> 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] <bac> 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] <bac> [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] <MootBot> 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] <bac> 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] <EdwinGrubbs> it looks like lifeless updated the page. Are there any other graduated UI reviewers besides noodles775, sinzui, and rockstar?
[15:11] <sinzui> no.
[15:11] <sinzui> henninge, is in the mentat
[15:12] <henninge> I am
[15:12] <sinzui> does anyone else want to become a UI reviewer
[15:12] <bac> salgado asked about becoming a ui reviewer so i referred him to rockstar
[15:12] <noodles775> It would be great to get another ... :)
[15:12] <salgado> noodles775, would you like to be my mentor? ;)
[15:12] <bigjools> we need more, since one of them is leaving :(
[15:12] <henninge> who is leaving?
[15:12] <noodles775> salgado: yeah, I'll be moving to ISd sometime...
[15:12]  * noodles775 needs to send email out.
[15:13] <gary_poster> :-(
[15:13] <salgado> oh, :(
[15:13] <gary_poster> glad you are still around-ish though :-)
[15:13] <bac> noodles775: :(
[15:13] <noodles775> Yep, still around :)
[15:13] <henninge> noodles775: :(
[15:14] <bac> who is mentoring henninge?
[15:14] <bac> perhaps the other can help salgado
[15:15] <sinzui> I can mentor salgado
[15:15] <bac> thanks sinzui
[15:15] <bac> moving on...
[15:15] <salgado> yay!
[15:15] <salgado> thanks sinzui
[15:15] <bac> [topic] * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews?
[15:15] <MootBot> New Topic:  * https://dev.launchpad.net/ArchitectureGuide as per the epic - will reviewers please start discussing the values and metrics during reviews?
[15:15] <bac> lifeless wants us to read that page and the attached presentation.
[15:15] <henninge> bac: rockstar is mentoring me
[15:15] <bac> henninge:  thanks
[15:16] <bac> and for reviewers to try to think more about architectural issues when reviewing.
[15:17] <bac> i'll ask him tonight to put his thoughts into an email wrt reviewing.
[15:17] <bac> [topic] Assertion errors and the webservice (gary)
[15:17] <MootBot> New Topic:  Assertion errors and the webservice (gary)
[15:17] <gary_poster> preparing paste 1 sec
[15:18] <gary_poster> We are currently using assertions to communicate errors that have more appropriate specific exceptions.
[15:18] <gary_poster> lib/lp/registry/model/product.py:720-ish
[15:18] <gary_poster>             if license not in License:
[15:18] <gary_poster>                 raise AssertionError("%s is not a License" % license)
[15:18] <gary_poster> LookupError?
[15:18] <gary_poster> lib/lp/answers/model/faq.py:184, and below
[15:18] <gary_poster>         if search_text is not None:
[15:18] <gary_poster>             assert isinstance(search_text, basestring), (
[15:18] <gary_poster>                 'search_text should be a string, not %s' % type(search_text))
[15:18] <gary_poster> ValueError/TypeError?
[15:18] <gary_poster> Those are from benji, after a conversation he and I had in regards to bug 413174. He has others.
[15:18] <gary_poster> 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] <gary_poster> 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] <gary_poster> But for now...that's my assertion suggestion.
[15:18] <gary_poster> "within the current code structure": like, method, or function.
[15:19] <bac> thanks gary_poster
[15:19] <sinzui> gary_poster, the case I use assertion errors is when the passed object is in the wrong state
[15:19] <sinzui> Is that a valid use?
[15:20] <gary_poster> 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] <sinzui> gary_poster, okay
[15:20] <benji> 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] <bigjools> we should always return a decent error to the webservice caller, surely?  an exception without a webservice_error  files an OOPS.
[15:21] <bac> benji: +1, as that's easy to forget.
[15:21] <sinzui> benji, that is why we strive to use raise AssertError. I never accept the other form in a review
[15:21] <gary_poster> (although I don't think "raise AssertionError()" is optimized away, right?)
[15:21] <sinzui> sometime the engineer drops the assertion because of that
[15:21] <gary_poster> right, sinzui
[15:21] <benji> right, raise AssertionError() is not removed
[15:22] <sinzui> gary_poster, In the example of deleting a milestone. If the state is bad, the method could return (false, message)
[15:22] <gary_poster> 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] <sinzui> so instead of a ValueError, a success state could be returned
[15:23] <gary_poster> 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] <sinzui> okay
[15:24] <bac> gary_poster: thanks for bringing it up.  i look forward  to seeing benji's work
[15:24] <gary_poster> cool, thanks bac.
[15:24] <bac> [topic] other stuff?
[15:24] <MootBot> New Topic:  other stuff?
[15:24] <henninge> just a little thin
[15:24] <henninge> g
[15:25] <henninge> I think
[15:25] <henninge> Who should set an MP to "approved"?
[15:25] <gary_poster> heh, good workflow question for MP design generally, I think.
[15:25] <bac> henninge: ideally, the last required reviewer
[15:25] <henninge> 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] <bac> henninge: but it is so easy to forget
[15:26] <gary_poster> tarmac will make it easier, IIUC
[15:26] <gary_poster> because that will be the cue to try and land (again, IIUC)
[15:26] <abentley> gary_poster, that's not something I've heard.
[15:26] <bac> henninge: if the dev already knows he needs more reviews he should add them to the MP
[15:26] <gary_poster> abentley: you know more than I about it, I'm sure
[15:26] <abentley> gary_poster, oh, you mean it will become necessary with tarmac.
[15:27] <gary_poster> yes
[15:27] <henninge> bac: does not happen very often AFICR
[15:27] <gary_poster> and it will have a more obvious meaning
[15:27] <henninge> A
[15:27] <gary_poster> right now it's almost pointless from a practical perspective
[15:27] <abentley> gary_poster, yes, in the current iteration.  There is ongoing work to split the review state from the merge state.
[15:27] <henninge> I mean, people add  a UI or r-c review after the code review
[15:27] <gary_poster> ah, ok, cool
[15:28] <abentley> 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] <bac> 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] <gary_poster> 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] <gary_poster> s/almost//
[15:29] <gary_poster> (where abentley's script would be one implementation of what I'm talking about, yeah)
[15:29] <bac> gary_poster: i think MPs purposely didn't include a bunch of specific behavior b/c other projects have different policies
[15:30] <bac> gary_poster: yeah, that could be overcome by a script
[15:30] <henninge> bac: sure but it is still the responsibilty of the one seeking approval to make sure the branch gets all required reviews.
[15:30] <abentley> gary_poster, variations in approval criteria are a significant obstacle to that.
[15:30] <henninge> so, setting an MP to "approved" just docuemnts that he followed through on that.
[15:30] <abentley> gary_poster, which is why doing it on a per-project basis is recommended.
[15:30] <gary_poster> bac, abentley, gotcha.
[15:30] <abentley> gary_poster, i.e. with a script.
[15:30] <gary_poster> right
[15:31] <gary_poster> henninge, but it can be the approval-seekers responsibility still with automation, yes?
[15:31] <gary_poster> By requesting more reviews
[15:31] <henninge> gary_poster: I am not opposed to automation at all! ;)
[15:31] <gary_poster> ok :-)
[15:32] <henninge> gary_poster: oh, misread that.
[15:33] <henninge> gary_poster: you mean, by requesting more reviews an mp would go back to "needs review"?
[15:33] <gary_poster> I was just about to propose that we had consensus, darn :-)
[15:33] <gary_poster> yes
[15:33] <henninge> fine, we do have a consensus ... ;)
[15:33] <gary_poster> heh, ok
[15:33] <bac> 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] <bac> henninge: ok, to move on?
[15:34] <gary_poster> consensus:
[15:34] <gary_poster> - approval seeker is of ultimately responsible for the review
[15:34] <henninge> 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] <gary_poster> oh
[15:34] <gary_poster> that's even easier I think
[15:34] <bac> henninge: ok
[15:34] <gary_poster> ok, /me is quiet :-)
[15:35] <bac> any other topic?
[15:35] <bac> 3
[15:35] <bac> 2
[15:35] <henninge> gary_poster: that does not contradict what you were just saying ... ;)
[15:35] <bac> 1.5
[15:35] <henninge> bac: continue ;)
[15:35] <bac> 1
[15:35] <gary_poster> aggred henninge :-)
[15:35] <gary_poster> ree
[15:35] <bac> #endmeeting
[15:35] <MootBot> Meeting finished at 09:35.
[15:36] <bac> thanks y'all
[15:36]  * bac practices for dallas
[15:36] <bigjools> bac: you need to practise?
[15:36] <bac> bigjools: yeah, not really
[15:36] <bigjools> :)
[15:36] <gary_poster> :-) thanks bac
[15:36] <bigjools> thanks bac
[15:37] <henninge> thanks bac
[15:37] <henninge> thanks gary_poster ;)
[15:37] <gary_poster> :-) thank you