ReviewerMeeting20081210

Not logged in - Log In / Register

ReviewerMeeting20081210

summary

log

Dec 10 10:00:07 <barry> #startmeeting
Dec 10 10:00:07 <MootBot>       Meeting started at 09:00. The chair is barry.
Dec 10 10:00:07 <MootBot>       Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
Dec 10 10:00:20 <barry> hello and welcome to this week's ameu reviewers meeting.  who's here today?
Dec 10 10:00:24 <rockstar>      me
Dec 10 10:00:40 <EdwinGrubbs>   me
Dec 10 10:00:44 <adeuring>      me
Dec 10 10:01:28 <flacoste>      me
Dec 10 10:02:13 <rockstar>      That's it?
Dec 10 10:02:16 <barry> wow, light attendance today <wink>
Dec 10 10:02:25 <barry> allenap: ping
Dec 10 10:02:32 <allenap>       me
Dec 10 10:02:35 <barry> bac_uds: hopeful ping
Dec 10 10:02:39 <barry> bigjools: ping
Dec 10 10:02:44 <barry> BjornT: ping
Dec 10 10:02:47 <barry> cprov: ping
Dec 10 10:02:49 <bigjools>      me
Dec 10 10:02:58 <barry> gmb: ping?
Dec 10 10:03:03 <barry> intellectronica: ping
Dec 10 10:03:07 <barry> mars: ping
Dec 10 10:03:16 <barry> salgado: ping
Dec 10 10:03:18 <bigjools>      it's 7 in the morning for cprov at UDS
Dec 10 10:03:18 <intellectronica>       me
Dec 10 10:03:28 <BjornT>        me
Dec 10 10:03:35 <barry> bigjools: what's his cell #?  let's wake him up :)
Dec 10 10:03:45 *       barry is not sure who's at uds
Dec 10 10:03:46 <rockstar>      Ooh, that's bad...
Dec 10 10:03:49 <bigjools>      1-800-EAT-SHIT
Dec 10 10:03:53 <rockstar>      !
Dec 10 10:03:53 *       barry dials
Dec 10 10:03:57 <mars>  me
Dec 10 10:04:03 <barry> bigjools: hey, waiiittt a minute!
Dec 10 10:04:09 <bigjools>      :)
Dec 10 10:04:21 <barry> [TOPIC] agenda
Dec 10 10:04:22 <MootBot>       New Topic:  agenda
Dec 10 10:04:30 <barry>  * Roll call
Dec 10 10:04:30 <barry>  * Printing strings in doctests (barry)
Dec 10 10:04:30 <barry>  * Gavin's `pretty()` function (allenap)
Dec 10 10:04:30 <barry>  * Do we need a standard cover letter template for merge proposals? (barry)
Dec 10 10:04:30 <barry>    * [[attachment:cover-quick.txt]]
Dec 10 10:04:30 <barry>    * [[attachment:cover.txt]]
Dec 10 10:04:30 <barry>  * Peanut gallery (anything not on the agenda)
Dec 10 10:04:30 <barry>  * If there's time, the old boring script
Dec 10 10:04:30 <barry>    * Next meeting
Dec 10 10:04:30 <barry>    * Action items
Dec 10 10:04:30 <barry>    * Mentoring update
Dec 10 10:04:30 <barry>    * Queue status
Dec 10 10:04:38 <barry> [TOPIC]  * Printing strings in doctests (barry)
Dec 10 10:04:47 <MootBot>       New Topic:   * Printing strings in doctests (barry)
Dec 10 10:05:06 <barry> so, i mentioned this in a couple of reviews as well as in the asiapac meeting, with some positive feedback
Dec 10 10:05:22 <barry> the idea is that in docstrings, you should (usually) be printing the value of strings instead of returning them
Dec 10 10:05:42 <sinzui>        me
Dec 10 10:05:44 <barry> the advantage of that is that you usually don't care if its unicode or str, and you're not saddled with extraneous noise like u' and '
Dec 10 10:05:48 <barry> e.g.
Dec 10 10:05:50 <barry> instead of
Dec 10 10:05:57 <barry> >>> foo['key']
Dec 10 10:05:59 <barry> u'baz'
Dec 10 10:06:01 <barry> you'd do
Dec 10 10:06:11 <barry> >>> print foo['key']
Dec 10 10:06:12 <barry> baz
Dec 10 10:06:16 <barry> thoughts?
Dec 10 10:06:28 <salgado>       me!
Dec 10 10:06:31 <salgado>       sorry, I'm late
Dec 10 10:06:33 <intellectronica>       yes, i also prefer printing
Dec 10 10:06:59 <bigjools>      barry: +1
Dec 10 10:07:07 <sinzui>        Unless we fix/hack the the testrunner, translations and answers tests will fail
Dec 10 10:07:51 <barry> sinzui: right.  hence the "usually" :)
Dec 10 10:07:58 <rockstar>      barry, +1
Dec 10 10:08:30 <barry> so, as reviewers just be aware of that in code you review and suggest conversion to printing strings
Dec 10 10:08:34 <barry> any objections?
Dec 10 10:08:45 <adeuring>      none, just a questin:
Dec 10 10:09:00 <barry> adeuring: go4it
Dec 10 10:09:21 <EdwinGrubbs>   I think that the bug really needs to be fixed in the testrunner, since it doesn't tell you which line the error is on when you try printing out a unicode character, the whole doctest just fails, so you end up commenting out half your test  to find the offending line.
Dec 10 10:09:37 <adeuring>      what, if we want to test, if we want to check that we have for eaxmpale a string object, but not a unicode object?
Dec 10 10:09:45 <adeuring>      print repr(foo)
Dec 10 10:09:46 <rockstar>      EdwinGrubbs, +1
Dec 10 10:09:54 <barry> EdwinGrubbs: totally agree.  it's not like we don't actually have proposed fixes either :)  iirc it's a bug in zope upstream
Dec 10 10:10:15 <adeuring>      I mean, whould we use explicity the print statment,
Dec 10 10:10:28 <barry> adeuring: in that case you can either return the value and show the u''
Dec 10 10:10:28 <adeuring>      or should we allow the simple >>> foo
Dec 10 10:10:40 <barry> adeuring: or, use isinstance(foo, unicode)
Dec 10 10:10:58 <barry> adeuring: or even print foo.encode('utf-8')
Dec 10 10:10:59 <rockstar>      I'd prefer the second choice.
Dec 10 10:11:14 <barry> rockstar: for a pure type test, i agree
Dec 10 10:11:37 <rockstar>      barry, yea' and if you want the value, you'd use print.
Dec 10 10:11:45 <adeuring>      barry: OK, though explicit encoding can make things more difficult to read
Dec 10 10:11:47 <barry> rockstar: right
Dec 10 10:12:08 <barry> adeuring: yep.  if we had a fixed testrunner we could just print it
Dec 10 10:12:19 <barry> flacoste: can we ask gary to look into fixing this upstream for us?
Dec 10 10:12:29 <adeuring>      barry: ...or use repr(u'äüö')
Dec 10 10:12:37 <flacoste>      barry: gary doesn't have on the testrunner
Dec 10 10:12:40 <adeuring>      slightly better to read than the encoded string
Dec 10 10:12:44 <flacoste>      barry: but he can send emails
Dec 10 10:12:52 <flacoste>      barry: but anyway, we are way behind upstream here
Dec 10 10:13:00 <flacoste>      barry: so we should just fix our stuff for now
Dec 10 10:13:12 <rockstar>      flacoste, +1
Dec 10 10:13:16 <barry> flacoste: maybe when we move to 2.6 :)
Dec 10 10:13:22 <barry> and zc.buildout :)
Dec 10 10:13:32 <flacoste>      2.6 is crack!
Dec 10 10:13:38 <barry> :-D
Dec 10 10:13:50 <barry> anyway for now, just be aware of this in reviews and make friendly suggestions
Dec 10 10:14:00 <barry> [TOPIC]  * Gavin's `pretty()` function (allenap)
Dec 10 10:14:01 <MootBot>       New Topic:   * Gavin's `pretty()` function (allenap)
Dec 10 10:14:17 <barry> allenap landed a branch with a nice pretty() function in doctest globals
Dec 10 10:14:29 <flacoste>      don't use it for API tests btw!
Dec 10 10:14:29 <barry> although i've since heard that lazr has something similar, right flacoste ?
Dec 10 10:14:37 <sinzui>        zope.testing.doctest:1486
Dec 10 10:14:37 <sinzui>            sys.stdout = self._fakeout
Dec 10 10:14:37 <sinzui>        Becomes
Dec 10 10:14:37 <sinzui>            from codecs import EncodedFile
Dec 10 10:14:37 <sinzui>            sys.stdout = EncodedFile(self._fakeout, "ascii", "utf-8")
Dec 10 10:14:39 <allenap>       flacoste: too late.
Dec 10 10:14:48 <barry> flacoste: why?
Dec 10 10:14:54 <barry> sinzui: yes, excactly
Dec 10 10:14:57 <flacoste>      because there is pprint_entry and pprint_collection
Dec 10 10:15:03 <flacoste>      that does that + other stuff
Dec 10 10:15:08 <barry> flacoste: ah
Dec 10 10:15:11 <flacoste>      for example, it omits the etag key
Dec 10 10:15:24 <flacoste>      so pretty() is fine for non-API stuff
Dec 10 10:15:36 <barry> flacoste: do we need a dev/reviewer style guide for apis?
Dec 10 10:15:47 <flacoste>      yeah, we probably do
Dec 10 10:15:52 <allenap>       flacoste: Yeah, agreed, the pprint_* functions are better for API. I'll clean up my API stuff that uses pretty().
Dec 10 10:15:54 <flacoste>      salgado started something if i recall
Dec 10 10:16:11 <flacoste>      or maybe it was for the coder perspective
Dec 10 10:16:26 <flacoste>      but yes, i'll do a API reviewer cheatsheet
Dec 10 10:16:38 <barry> [ACTION] flacoste to do an API reviewer cheatsheet
Dec 10 10:16:39 <MootBot>       ACTION received:  flacoste to do an API reviewer cheatsheet
Dec 10 10:16:41 <barry> flacoste: thanks!
Dec 10 10:16:43 <flacoste>      or add a section to the reviewer cheet sheet
Dec 10 10:17:12 <barry> btw, rs=barry for any cleanup that consolidates pretty() and lazr's pretty printer
Dec 10 10:17:38 <salgado>       flacoste, barry, https://launchpad.canonical.com/API/StyleGuide is what I wrote
Dec 10 10:18:05 <barry> salgado: nice, thanks
Dec 10 10:18:28 <barry> [TOPIC]  * Do we need a standard cover letter template for merge proposals? (barry)
Dec 10 10:18:29 <MootBot>       New Topic:   * Do we need a standard cover letter template for merge proposals? (barry)
Dec 10 10:18:58 <barry> i mentioned this last week.  this week i have two examples.  one a quick outline and the other more detailed, with examples
Dec 10 10:19:05 <barry> https://dev.launchpad.net/ReviewerMeetingAgenda?action=AttachFile&do=view&target=cover-quick.txt
Dec 10 10:19:17 <barry> https://dev.launchpad.net/ReviewerMeetingAgenda?action=AttachFile&do=view&target=cover.txt
Dec 10 10:19:43 <barry> putting aside the issue of tool support for this, what do you think about encouraging devs to use this in mps?
Dec 10 10:19:54 <barry> the thing i like about it is that everything's in one comment
Dec 10 10:20:07 <barry> so when you get the mp email, you've already got the description, the lint, and the diff
Dec 10 10:20:15 <barry> and it makes it very easy to respond in email
Dec 10 10:20:46 <barry> i really hate it when i have to review a branch, but it's spread out over several emails
Dec 10 10:21:02 <rockstar>      barry, I don't like that either.
Dec 10 10:21:16 <rockstar>      I usually try and consolidate it into one reply.
Dec 10 10:21:37 <barry> rockstar: right, which isn't always easy depending on your mua :)
Dec 10 10:22:01 <rockstar>      Yea.
Dec 10 10:22:09 <rockstar>      (vim ftw)
Dec 10 10:22:27 <barry> rockstar: claws + emacs + emacsclient :)
Dec 10 10:22:33 <bigjools>      jeez, copy and paste ffs :)
Dec 10 10:22:47 <barry> anyway.  +1, -1  for the cover standard?
Dec 10 10:22:59 <bigjools>      +0
Dec 10 10:23:04 <rockstar>      barry, which one is would be the standard?
Dec 10 10:23:22 <barry> rockstar: they're the same except for the explanatory text and examples in the long one
Dec 10 10:23:32 <barry> rockstar: so cover-quick.txt is the template most people would use
Dec 10 10:24:22 <bigjools>      how much of the stuff in the template should we / can we put in the MP form?
Dec 10 10:24:43 *       sinzui has been pasting it all
Dec 10 10:25:26 <barry> sinzui: right.  the idea is that you begin filling out the form right when you start working on the branch.  lots will be empty, but you fill it in as you think about the fix, get a pre-imp, hack the code, etc
Dec 10 10:25:35 <barry> when you're ready for the mp, you paste the whole thing
Dec 10 10:25:37 <rockstar>      barry, why do we need bug XXXXXX section?
Dec 10 10:25:58 <barry> rockstar: as the longer example says, it's to translate the bug description into human
Dec 10 10:26:15 <sinzui>        barry: correct. I do,
Dec 10 10:26:26 <barry> from the native "user" :)
Dec 10 10:26:41 <rockstar>      Hm.  I think if I read the bug itself, and then read the solution, I could deduce the issues myself.
Dec 10 10:27:03 <barry> rockstar: remember, we're trying to make things fast and easy for reviewers
Dec 10 10:27:10 <bigjools>      doesn't the branch link to the bug if you say --fixes?
Dec 10 10:27:11 <barry> rockstar: so the dev should do more work upfront (imo)
Dec 10 10:27:18 <rockstar>      bigjools, yes
Dec 10 10:27:24 *       salgado is now known as salgado-lunch
Dec 10 10:27:39 <barry> rockstar: the idea is.  i'm going to review your branch in 5 minutes.  i should have everything i need right there in that one email
Dec 10 10:27:43 <rockstar>      barry, I'm all for people making the reviewer's life easier.
Dec 10 10:27:50 <barry> rockstar: don't make me go looking around the world unless i want to
Dec 10 10:27:54 <bigjools>      I think that doing that and having the reviewer follow a link is better than the dev re-writing the bug description
Dec 10 10:28:15 <barry> bigjools: the problem is that many bug have poor descriptions, or long conversational threads
Dec 10 10:28:37 <barry> bigjools: i think it's the dev's resposibility to boil it down to: "Bug XYZ describes this very specific problem"
Dec 10 10:28:40 <rockstar>      barry, so why can't that translation to human be in the bug.
Dec 10 10:28:45 <bigjools>      barry: so I would rather the root cause was fixed then - lets update the bug
Dec 10 10:29:18 <barry> i guess the point is that i don't want to have to hunt around in the bug.  i want to read my email, do my review and be done with it
Dec 10 10:29:24 <rockstar>      barry, maybe that would make sense as a sentence in the pre-implementation of implementation section.
Dec 10 10:30:02 <bigjools>      I hear we've got this funky tool called Launchpad that stores all this stuff for us ;)
Dec 10 10:30:19 <barry> i should try to find a real-world example to show you what i mean.  it's really just a 2 sentence summary
Dec 10 10:30:39 <barry> and you can just cut-n-paste the bug description if it's in human readable form
Dec 10 10:30:57 <rockstar>      If there's good information about the bug itself, I feel it really belongs in the bug, not the merge proposal.
Dec 10 10:31:12 <barry> but if it's not, or the bug has a long conversational thread that eventually gets around to the real bug, then 2 sentences, boom you're done, and you've helped reduce reviewer friction
Dec 10 10:31:35 <rockstar>      It may be just two lines, but it's still segmentation of information.
Dec 10 10:31:38 <bigjools>      let me propose this:  if the bug's description was included in the branch page (is it already?) and the email, and the description was updated if it's inaccurate, would that suffice?
Dec 10 10:31:44 <barry> rockstar: this isn't a big detailed thing, it's a summary.  the cover can always say there are important details in the tracker issue
Dec 10 10:32:03 <rockstar>      QA might not know to go look at the merge proposal for the actual explanation.
Dec 10 10:32:07 <sinzui>        rockstar: I summarize the bug because I don't want to make you spend 20 minutes reading all the comments that were not relevant to diagnosing the root cause
Dec 10 10:32:42 <barry> bigjools: sure.  if it were included in the email that would help
Dec 10 10:32:55 <barry> sinzui: exactly
Dec 10 10:33:04 <rockstar>      sinzui, I understand that, and I agree it's helpful.  I just think that we have all these buckets where certain types of information go.
Dec 10 10:33:05 <bigjools>      barry: great - I just hate the idea of re-writing the bug description and not including it in the bug report!
Dec 10 10:33:06 <barry> this is all about reducing reviewer friction
Dec 10 10:33:22 <rockstar>      barry, and I agree it needs to happen.
Dec 10 10:33:41 <rockstar>      I also like to make sure that the branch is linked to the appropriate bugs it fixes.
Dec 10 10:33:44 <sinzui>        rockstar: secondly, I may land five branches to fix a bug, each branch focuses on an aspect of the bug that is strictly scoped so that I have small branches
Dec 10 10:33:49 <barry> bigjools: i have no problem if the workflow is: dev updates the bug description to accurately reflect the root cause of the problem being reported, then cut-and-pastes that into his cover letter :)
Dec 10 10:33:54 <BjornT>        i think we should rename 'Bug XXX' to 'Summary', and concentrate on describing what the branch contains, rather than what the bug was.
Dec 10 10:34:12 <barry> sinzui: excellent point!
Dec 10 10:34:14 <rockstar>      BjornT, +1
Dec 10 10:34:50 <intellectronica>       +1, and we already have that in the submission form, so the only thing is to make sure that it's displayed conveniently both on the web and in email
Dec 10 10:34:54 <bigjools>      BjornT: again, I think that should be recorded in LP, with the branch.  It can be sent with the email.
Dec 10 10:34:59 <barry> BjornT: Summary of Bug XXXXX and i'm on board with that
Dec 10 10:35:16 <bigjools>      barry: not all branches are bug fixes though
Dec 10 10:35:24 <barry> bigjools: hmm. okay
Dec 10 10:35:30 <BjornT>        barry: well, as sinzui said, he might land five branches to fx
Dec 10 10:35:33 <barry> but they should be, right? :)
Dec 10 10:35:34 <BjornT>        fix a single bug
Dec 10 10:35:43 <bigjools>      barry: blueprints? :)
Dec 10 10:35:46 <rockstar>      barry, two lines in of the faur line Summary can be devoted to it.
Dec 10 10:35:53 <BjornT>        barry: should he include the same bug description in all five merge proposals?
Dec 10 10:35:55 <rockstar>      s/faur/four
Dec 10 10:35:58 <barry> bigjools: i've heard it's a great way to increase your karma
Dec 10 10:36:14 <bigjools>      barry: you get more for blueprints! :)
Dec 10 10:36:36 <barry> BjornT: when i've done that, the first sentence is the same, but then the rest of that summary section goes on to explain the sub-issue this branch is addressing
Dec 10 10:36:44 <barry> but i'm okay with Summary
Dec 10 10:37:10 <BjornT>        barry: right. in that case you're not summarizing the bug report; you're summarizing your branch
Dec 10 10:37:19 <barry> as long as i don't have to guess which bug it's addressing
Dec 10 10:37:32 *       EdwinGrubb (n=egrubbs@216-188-227-97.dyn.grandenetworks.net) has joined #launchpad-meeting
Dec 10 10:37:36 <barry> BjornT: cool, i'm convinced.  Summary sounds good
Dec 10 10:38:05 <barry> i'll make that change and take it to the ml
Dec 10 10:38:17 <BjornT>        i do agree that the summary should explain in words what issue was fixed, rather than just saying 'fixes bug xxx'.
Dec 10 10:38:35 <barry> btw, i really appreciate all the good feedback.  ultimately this is about making things easier for us reviewers!
Dec 10 10:38:50 <barry> you should work harder when your dev hat is on :)
Dec 10 10:39:00 <barry> BjornT: +1
Dec 10 10:39:29 <bigjools>      as someone once said to me, we're being worked like camels already :)
Dec 10 10:39:56 <barry> bigjools: :)  reducing dev friction is a whole 'nuther topic
Dec 10 10:40:16 <barry> but not for today :)
Dec 10 10:40:29 <bigjools>      5 mins left!
Dec 10 10:40:32 <barry> [TOPIC] # Peanut gallery (anything not on the agenda) 
Dec 10 10:40:33 <MootBot>       New Topic:  # Peanut gallery (anything not on the agenda)
Dec 10 10:40:44 <barry> bigjools: what a great straight man you are!
Dec 10 10:40:56 <barry> so, the floor is open.  do you have anything not on the agenda?
Dec 10 10:41:16 <rockstar>      How are merge proposals going?
Dec 10 10:41:37 <rockstar>      We're fixing bugs to get things easier to use.
Dec 10 10:41:39 <bigjools>      I think they're great, but I think they could prompt us for the info that barry wants :)
Dec 10 10:41:44 <rockstar>      Is anyone noticing?
Dec 10 10:41:56 <flacoste>      bigjools: honestly, i don't think so
Dec 10 10:42:13 <flacoste>      form filling is boring
Dec 10 10:42:33 <barry> rockstar: you had me at "PendingReviews is dead".  i think they're great
Dec 10 10:42:39 <bigjools>      flacoste: you'd rather copy & paste a template?
Dec 10 10:42:39 <sinzui>        flacoste: but note taking is safe and satisfying.
Dec 10 10:42:44 <rockstar>      barry, Hurray!
Dec 10 10:42:54 <flacoste>      yeah, free form text FTW!
Dec 10 10:42:56 <barry> tool support would definitely make it easier
Dec 10 10:43:09 <barry> rockstar: plus i want a button to re-send me the mp email :)
Dec 10 10:43:13 <bigjools>      I like being reminded of things, I'm not getting any younger!
Dec 10 10:43:34 <BjornT>        flacoste: +1
Dec 10 10:44:07 <barry> flacoste, rockstar how much of the mp stuff is exposed in the api?
Dec 10 10:44:09 <BjornT>        bigjools: you should be able to use 'bzr send' and have the template come up in your text editor.
Dec 10 10:44:28 <bigjools>      BjornT: bzr send creates MPs?
Dec 10 10:44:31 <rockstar>      barry, the basic read only stuff landed last week.
Dec 10 10:44:36 <barry> BjornT: though of course, you've already started the template when you began to think about the bug, had your pre-impl, etc. <wink>
Dec 10 10:44:50 <BjornT>        bigjools: well, somehow you should be able to submit MPs from bzr
Dec 10 10:44:53 <rockstar>      bigjools, it will, as soon as abentley gets back from UDS.
Dec 10 10:45:06 <barry> BjornT: big +1 there
Dec 10 10:45:07 <rockstar>      BjornT, yea, that's the idea.
Dec 10 10:45:11 <bigjools>      BjornT: agree
Dec 10 10:45:18 <bigjools>      rockstar: that would be very cool!
Dec 10 10:45:20 <intellectronica>       personally, i would rather have web forms (and perhaps the ability to bypass them using the API)
Dec 10 10:45:31 <intellectronica>       and i think they would be very good for are users too
Dec 10 10:45:38 <bigjools>      well both - choice is good
Dec 10 10:45:41 <rockstar>      So diffs and bzr send are the big ones for mp right now then?
Dec 10 10:46:05 <barry> rockstar: i think so
Dec 10 10:46:31 <barry> oops, look at the clock.  we're the time go?
Dec 10 10:46:35 <rockstar>      Cool.  I'll bring that up in the meeting.  I know the code has been landing, just don't know the specifics.
Dec 10 10:46:42 <barry> anything else before we break?
Dec 10 10:46:50 <barry> 5
Dec 10 10:46:52 <BjornT>        rockstar: especially the possiblity to attach diffs (rather than having them generated automatically, which is also nice of course)
Dec 10 10:47:00 <barry> 4
Dec 10 10:47:07 <barry> 3
Dec 10 10:47:09 <rockstar>      BjornT, agreed
Dec 10 10:47:13 <barry> 2
Dec 10 10:47:18 <barry> 1
Dec 10 10:47:19 *       EdwinGrubbs has quit (Connection timed out)
Dec 10 10:47:20 <rockstar>      Break!
Dec 10 10:47:21 <barry> #endmeeting

ReviewerMeeting20081210 (last edited 2008-12-17 12:43:48 by barry)