| === salgado-afk is now known as salgado | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === mbarnett` is now known as mbarnett | ||
| bac | hi everyone | 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:00 |
| bac | yay, mootbot is here | 15:00 |
| bac | who else? | 15:00 |
| allenap` | me | 15:00 |
| henninge | me | 15:00 |
| sinzui | ma | 15:00 |
| sinzui | me | 15:00 |
| sinzui | mo | 15:01 |
| sinzui | mu | 15:01 |
| danilo_ | me | 15:01 |
| sinzui | my | 15:01 |
| bigjools | me | 15:01 |
| bac | gmb, intellec`, mars, EdwinGrubbs: ping | 15:01 |
| * bigjools calls troops | 15:01 | |
| intellec` | me | 15:01 |
| EdwinGrubbs | me | 15:01 |
| al-maisan | me | 15:01 |
| mars | me | 15:01 |
| barry | lurk | 15:01 |
| mars | lol | 15:01 |
| bac | gary_poster: release-manager ping | 15:02 |
| adeuring | me | 15:02 |
| * bac understands gary_poster may be too busy | 15:02 | |
| gary_poster | bac, yeah, a bit busy thanks | 15:02 |
| bac | [topic] agenda | 15:02 |
| MootBot | New Topic: agenda | 15:02 |
| bac | * Roll call | 15:03 |
| bac | * Action items | 15:03 |
| bac | * Styles of reviews: should that be up to the reviewer or is there a preferred style? [salgado] | 15:03 |
| bac | * Conditional expressions - are they kosher, now that we're on Python2.5? [intellectronica] | 15:03 |
| bac | * Peanut gallery (anything not on the agenda) | 15:03 |
| bac | [topic] action items | 15:03 |
| allenap` | Apologies from deryck. | 15:03 |
| noodles775 | me | 15:03 |
| MootBot | New Topic: action items | 15:03 |
| bac | thanks allenap` | 15:03 |
| bac | * intellectronica to draft guidelines for drive-by cleanups | 15:03 |
| intellec` | intellec`: i have no idea who this intellectronica dude is | 15:03 |
| bac | intellec`: any progress? | 15:04 |
| intellec` | but i haven't done that. sorry | 15:04 |
| bac | intellec`: would you like to commit to a date to do it or drop it from the list? | 15:04 |
| intellec` | i would even go as far as suggesting that we drop it, just to be realistic. it's been lingering for a while | 15:04 |
| bac | dropped | 15:04 |
| bac | * Gavin to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc | 15:04 |
| allenap` | I'm doing that as we speak, so keep it, but it will definitely be done later today. | 15:05 |
| bac | cool, thanks allenap`. look forward to a lively discussion. | 15:05 |
| bac | * Gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. | 15:05 |
| salgado | me | 15:05 |
| gary_poster | no, sorry, and next week is team lead sprint | 15:05 |
| bac | gary_poster: ok, i'll note that and we can revisit in a few weeks | 15:06 |
| bac | * Henning to update wiki page for pre-imp requirement for community contributions, etc. | 15:06 |
| bac | henninge: good news? | 15:06 |
| henninge | I did that as far as I could. | 15:06 |
| henninge | But I found out that we don't have a real reviewer documentation on dev.lp.net | 15:06 |
| henninge | a lot of pages would need to be migrated from launchapd.canonical.com and updated | 15:07 |
| henninge | I did not do that. | 15:07 |
| henninge | But I mentioned the requirement on https://dev.launchpad.net/PreMergeReviews | 15:07 |
| barry | some of those old pages are *really* out of date :/ | 15:07 |
| bac | henninge: understood. thanks. | 15:07 |
| bac | barry: yes, we need to do some wiki tending... | 15:08 |
| bac | i'll take a look at it this week. | 15:08 |
| bac | [action] bac to look at moving wiki pages to dev.lp.net and cleaning out the cruft | 15:08 |
| MootBot | ACTION received: bac to look at moving wiki pages to dev.lp.net and cleaning out the cruft | 15:08 |
| bac | new topics | 15:09 |
| bac | * Styles of reviews: should that be up to the reviewer or is there a preferred style? [salgado] | 15:09 |
| bac | er | 15:09 |
| bac | [topic] * Styles of reviews: should that be up to the reviewer or is there a preferred style? [salgado] | 15:09 |
| MootBot | New Topic: * Styles of reviews: should that be up to the reviewer or is there a preferred style? [salgado] | 15:09 |
| salgado | so, we have basically two styles of doing reviews: | 15:09 |
| salgado | 1. Comments go around the code they apply to: Reviewer includes the diff (often quoted by the email client) and each comment made by the reviewer goes around the code it applies to. This means there's context for each comment and the developer can quickly see where that code is. | 15:09 |
| salgado | 2. Top poster: Reviewer makes one or more comments at the top of the message, which may or may not contain the diff. Works well when the review points out *very* few things (or none), but as a developer I find it rather painful to go through a review containing just a list of bullet points with comments. | 15:10 |
| salgado | Developers always try hard to make the reviewers' lives easier, so when wearing our reviewer hats we should try and make their lives easier as well by including the diff for context and commenting around the specific parts the comment applies to. | 15:10 |
| gmb | me | 15:10 |
| gmb | Argh, sorry | 15:10 |
| * gmb was otp | 15:10 | |
| intellec` | there's another style: irc discussion (at best pasted into the MP later) | 15:10 |
| salgado | should we have a policy on which style is preferred? | 15:11 |
| barry | and a mix of styles: general comments at the top, specific comments mixed in with code quotes | 15:11 |
| intellec` | i don't think we need to decide on a style. it's just important to remember that readability counts in reviews too | 15:11 |
| bigjools | also as jml reminded me recently, include the file names with the diff chunks when commenting on them | 15:12 |
| bac | i'll also note you cannot refer to line numbers in the diff as it is a moving target and gets updated when the branch is pushed | 15:12 |
| bigjools | intellec`: +1 | 15:12 |
| bac | bigjools: +1 | 15:12 |
| barry | the general rule is: make the reviewers lives easier. this applies to the reviewer too <wink>. iow, if you make the review hard for the dev to deal with, you're just making your own reviewing life harder (more questions, more back and forth, more context switching, etc.). do what you need to do to give the dev a high fidelity review they can act fully on the first time through | 15:12 |
| bigjools | I don't think we need a policy, just some hints on making it easy to understand a review | 15:12 |
| bac | reviewees, at least those of us here, shouldn't be shy about pointing out unclear style to reviewers. | 15:13 |
| salgado | I brought this up because I find it really disturbing to go through a review which makes a bunch of points about the code but doesn't include the diff or anything for context | 15:13 |
| barry | i always keep the diff header for the file i'm reviewing, so it's easy to see. i'll cut out code usually only on hunk granularity so it's easier to follow | 15:13 |
| barry | bac, salgado agreed | 15:14 |
| bac | thanks for bringing it up salgado. i agree with bigjools that i don't think we need a new policy just awareness. | 15:14 |
| salgado | should we put this in a guidelines wiki page, to increase awareness, then? | 15:15 |
| bac | personally i've adopted the style of downloading the diff, doing the review in the editor of choice (cough, emacs) and pasting it to the page. and i try to delete only on chunk boundaries as barry mentioned. | 15:15 |
| intellec` | i think that if you feel that we need to increase awareness an email to the list is in order | 15:15 |
| bac | salgado: sure. would you do that? | 15:15 |
| salgado | sure, I can do it | 15:15 |
| bac | additional thoughts? | 15:16 |
| barry | as a dev, please have a conversation with your reviewer if you think the review did not provide the necessary level of context to help you resolve the issues | 15:16 |
| bac | [action] salgado to update the wiki page to encourage reviews with sufficient context | 15:16 |
| MootBot | ACTION received: salgado to update the wiki page to encourage reviews with sufficient context | 15:16 |
| salgado | barry, definitely | 15:16 |
| bigjools | reviewing is a two-way process | 15:17 |
| henninge | bac: I thouht ML discussion, not wiki page? | 15:17 |
| barry | big +1 | 15:17 |
| barry | we're all here to learn, right?! :) | 15:17 |
| henninge | and: which wiki page? ;_) | 15:17 |
| bigjools | barry: I likez to lern | 15:17 |
| salgado | henninge, I'll create one and send an email about it to the list | 15:17 |
| bac | salgado: thanks! | 15:17 |
| henninge | salgado: great, thanks | 15:17 |
| bac | moving on... | 15:18 |
| bac | * Conditional expressions - are they kosher, now that we're on Python2.5? [intellectronica] | 15:18 |
| barry | bigjools: i gots lernt me good sometimes | 15:18 |
| intellec` | use(conditional_expressions) if conditional_expressions is kosher else not use(conditional_expressions) | 15:18 |
| bigjools | barry: git 'er dun | 15:18 |
| intellec` | i'm +1 on using them, as long as they're not nested or otherwise too complex. i'd like to get other reviewers' opinion and decide on an official policy | 15:18 |
| bac | bigjools: you've been in austin too long | 15:18 |
| bigjools | lol | 15:18 |
| barry | +1, tastefully, which will have to evolve. my personal preference in general is that it makes code like this easier to read: | 15:19 |
| barry | if foo: | 15:19 |
| barry | bar = 7 | 15:19 |
| barry | else: | 15:19 |
| barry | bar = 9 | 15:19 |
| barry | becomes | 15:19 |
| barry | bar = (7 if foo else 9) | 15:19 |
| * gmb agrees with barry | 15:19 | |
| barry | and always use parentheses, even if the syntax does not strictly require it, because it's more readable | 15:19 |
| al-maisan | this reminds me a bit of perl :P | 15:19 |
| intellec` | i also like the parentheses around the expression | 15:20 |
| barry | al-maisan: guido deliberately picked odd syntax so people wouldn't abuse it :) | 15:20 |
| mars | +1, trust the developers to use them well | 15:20 |
| al-maisan | well, he *did* succeed in that | 15:20 |
| barry | also, multiline form if the conditional is long: | 15:20 |
| mars | intellec`, "tasteful" is the right word :) | 15:20 |
| adeuring | +1 | 15:20 |
| bigjools | I think it's fine, any readability issues will be picked up in review | 15:20 |
| barry | bar = (7 if foo.getSomeDataOutOfThere('baz') == 'frobnicate' | 15:21 |
| barry | else 9) | 15:21 |
| mars | eewww | 15:21 |
| barry | but multiline is a 'hint' that it might be too complex <wink> | 15:21 |
| bac | barry: does muti-line make sense? i think i'd rather see that using if-then | 15:21 |
| intellec` | so, basically, it's a yes, but like everything else we discuss it in reviews to make sure the result is readable? i like that | 15:21 |
| bigjools | and I expect we'll establish formatting conventions with a bit of use | 15:21 |
| mars | barry, I think IRC didn't treat that last example so well :) | 15:21 |
| intellec` | barry: i agree, multiline also doesn't improve much on the imperative form | 15:21 |
| henninge | intellec`: +1 | 15:21 |
| barry | agreed. i think it will be a evolution of team taste over time | 15:22 |
| barry | s/a/an/ | 15:22 |
| bac | any thoughts on limiting conditional expressions to single line for readability? | 15:22 |
| bigjools | I say see how it goes | 15:22 |
| adeuring | bac: no "chaining" of conditional expressions | 15:22 |
| mars | -1 on limiting them. Trust the devs. | 15:22 |
| barry | bigjools: +1 | 15:22 |
| intellec` | bac: yes, i think that's a good rule | 15:22 |
| gmb | bac: +1 | 15:23 |
| barry | adeuring: agreed on chaining. that really reduces readability | 15:23 |
| bac | adeuring: +1 | 15:23 |
| barry | bac: i have another 2.5ism for ya, when you're ready :) | 15:23 |
| bac | if we are in agreement (mostly) i'll add that to the wiki this week | 15:24 |
| bac | [action] bac to update coding guideline wrt conditional expressions | 15:24 |
| MootBot | ACTION received: bac to update coding guideline wrt conditional expressions | 15:24 |
| bac | barry: yes? | 15:24 |
| barry | <cough>from __future__ import with_statement</cough> should be legal too now | 15:25 |
| intellec` | barry: do transactions work with it? that's the classis use, no? | 15:25 |
| * salgado has done that already; legal or not | 15:25 | |
| barry | context managers rock | 15:25 |
| mars | +1 | 15:25 |
| barry | intellec`: i've had success with them, jtv doesn't like them for that purpose. but that have lots of other uses | 15:25 |
| barry | just about any place you've got ugly try/finally's, you can write a simple context manager and use a with statement | 15:26 |
| bigjools | how about you take that to the ML with some examples? | 15:27 |
| barry | i've used them for stashing and restoring umasks, cwd, sys.stdout, etc. etc. they work great with files which already support the context manager protocol | 15:27 |
| barry | bigjools: +1 | 15:27 |
| bac | barry: would you be willing to email the list with examples and update the coding guidelines? | 15:27 |
| barry | bac: yep | 15:27 |
| bac | [action] barry to school us on the proper use of 'with' | 15:27 |
| MootBot | ACTION received: barry to school us on the proper use of 'with' | 15:27 |
| bac | that's it for listed items | 15:28 |
| bac | [action] * Peanut gallery (anything not on the agenda) | 15:28 |
| MootBot | ACTION received: * Peanut gallery (anything not on the agenda) | 15:28 |
| bac | anyone have another topic? | 15:28 |
| bac | of course that should | 15:28 |
| bac | 've been a topic not an actoin | 15:28 |
| bac | going once | 15:29 |
| bac | twice | 15:29 |
| EdwinGrubbs | I have a question | 15:29 |
| bac | yes EdwinGrubbs | 15:29 |
| EdwinGrubbs | Should translations/translations.js put items in the Y.translations.translations namespace, or should we have a convention similar to python, where translations/__init__.js adds items to the Y.translations namespace? | 15:29 |
| === salgado is now known as salgado-lunch | ||
| * bigjools wonders how useful it would be to have a bzr plugin that creates new files with the correct copyright/licence at the top | 15:29 | |
| mars | bigjools, I just use templates in my editor for that | 15:30 |
| bigjools | you still need to bzr add though? | 15:30 |
| mars | bigjools, but you could have a lint step: legal-lint! | 15:30 |
| bigjools | uff :) | 15:31 |
| bac | sinzui, mars, intellec`: any thoughs on EdwinGrubbs' question? | 15:31 |
| mars | bac, thinking | 15:31 |
| intellec` | i prefer not importing into __init__ | 15:32 |
| intellec` | i find it hard to follow later on | 15:32 |
| mars | EdwinGrubbs, anything under the translations/ directory can add to the namespace and objects. What we do now (and by YUI convention) is that the same named file includes everything rolled up. | 15:32 |
| EdwinGrubbs | mars: that should be added to the wiki then, if it is an existing yui convention | 15:33 |
| mars | intellec`, agreed. I find the correct building of package interfaces using __init__ to be sticky enough already, without having to mold the Py conventions to JS | 15:34 |
| bac | EdwinGrubbs, mars: would one of you like to sort it out and document it? | 15:34 |
| EdwinGrubbs | bac: I can do it | 15:35 |
| bac | [action] edwin to document JS namespace issue | 15:35 |
| MootBot | ACTION received: edwin to document JS namespace issue | 15:35 |
| bac | thanks edwin | 15:35 |
| bac | final thoughts? | 15:35 |
| bac | 3 | 15:35 |
| bac | 2 | 15:35 |
| bac | 1 | 15:36 |
| bac | #endmeeting | 15:36 |
| MootBot | Meeting finished at 09:36. | 15:36 |
| bac | thanks for coming everyone | 15:36 |
| al-maisan | Thanks! | 15:36 |
| mars | thanks bac | 15:36 |
| barry | bac: thanks! | 15:36 |
| henninge | thank you bac | 15:36 |
| bac | thanks for "lurking" barry | 15:37 |
| === salgado-lunch is now known as salgado | ||
| === EdwinGrubbs is now known as Edwin-lunch | ||
| === mrevell is now known as mrevell-dinner | ||
| === Edwin-lunch is now known as EdwinGrubbs | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!