| === Edwin is now known as Guest66225 | ||
| 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:00 |
| 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:01 |
| abentley | me | 15:02 |
| deryck | me | 15:02 |
| 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:03 |
| 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:04 |
| 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? | 15:05 |
| bac | <lifeless> 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:05 |
| 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:06 |
| 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:07 |
| 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:08 |
| 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:09 |
| 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:10 |
| 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:11 |
| 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:12 | |
| 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:13 |
| bac | who is mentoring henninge? | 15:14 |
| bac | perhaps the other can help salgado | 15:14 |
| 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:15 |
| bac | and for reviewers to try to think more about architectural issues when reviewing. | 15:16 |
| 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:17 |
| 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 |
| ubot5 | 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 |
| gary_poster | But for now...that's my assertion suggestion. | 15:18 |
| gary_poster | "within the current code structure": like, method, or function. | 15:18 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| 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:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:28 |
| 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:29 |
| 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:30 |
| 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:31 |
| henninge | gary_poster: oh, misread that. | 15:32 |
| 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:33 |
| 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:34 |
| 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 | aggred henninge :-) | 15:35 |
| gary_poster | ree | 15:35 |
| bac | #endmeeting | 15:35 |
| MootBot | Meeting finished at 09:35. | 15:35 |
| 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:36 |
| henninge | thanks bac | 15:37 |
| henninge | thanks gary_poster ;) | 15:37 |
| gary_poster | :-) thank you | 15:37 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!