#launchpad-reviews 2009-09-14
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue: []
* danilos changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue: [danilo]
<danilos> henninge, a lovely branch for you in the queue
 * henninge already loves it ...
<danilos> henninge, still waiting for MP to hit LP though
<danilos> henninge, https://code.edge.launchpad.net/~danilo/launchpad/bug-408718/+merge/11682
<danilos> henninge, btw, I couldn't see what you said with /me, it seems it's a bug in xchat
<henninge> not important
<henninge> danilos: can you never see it? like here:
 * henninge thinks maybe its danilo and not xchat thats broken ...
<danilos> henninge, exactly right there
<adeuring> henninge: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-submissions-with-udev-data/+merge/11683 ?
<henninge> danilos: so is it a bug in my xchat or yours.
<henninge> adeuring: sure, put it in the queue, please.
<danilos> henninge, mine, it's probably a translations bug
<adeuring> henninge: thanks!
<henninge> I see
* adeuring changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue: [danilo, adeuring]
<danilos> henninge, I've seen it before, a bad translation on messages with no c-format flag (or someone just cuts off %s)
<henninge> mp page has a double title ...
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: danilo || queue: [adeuring]
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: danilo || queue: [adeuring]
<danilos> henninge, heh, no wonder it's a translations bug, just look at msgids: https://pastebin.canonical.com/22063/
<henninge> what in the world ...
 * danilos tests the non working version
<danilo_> ok, it doesn't
<henninge_> danilo_: doesn't what?
<danilo_> henninge: don't worry, I am testing the bug in xchat-gnome; I'll move that to some other place, sorry :)
<allenap> intellectronica: Fancy a pretty trivial ui conversion review? https://code.edge.launchpad.net/~allenap/launchpad/ui-convert-bugtarget-x-3.0-bug-427755/+merge/11685
<intellectronica> allenap: sure
<intellectronica> allenap: which pages should i look at?
<allenap> intellectronica: Doh, I forgot to put that in. Sorry. I'll add a comment and ping you back.
<henninge_> danilo_: I  do not see where the query is executed a second time after being aborted. Why return an empty result set?
<danilos> henninge: I don't want to execute it a second time
<danilos> henninge: as the cover letter explains, this is an optimization we can do without
<danilos> henninge: if it fails, just do not do super-fast-imports but process that one pofile slowly
<danilos> henninge: which brings up a good point: I should reset the statement_timeout to default after the query :)
<henninge> danilos: I must be missing some understanding of how superfast imports work ...
<danilos> henninge, you might, but it's quite simple
<danilos> henninge, basically, what they do is: fetch the entire pofile into ExistingPOFileInDatabase... if an identical message is already imported, don't run updateTranslation on it, and just skip over it
<danilos> henninge, by disabling them in this way, it will be much slower when it happens, because updateTranslation will be called on each of the messages; however, updateTranslation is able to cope with identical messages anyway, so no harm done
<danilos> henninge, and, if this keeps happening once a month on every file, no big deal either
<henninge> danilos: so the rows fetched by _fetchDBRows are used for the check of what's already there
<danilos> henninge, if we have time, we can log the query plan later this week (so stub has something to look at when it happens again)
<danilos> henninge, exactly
<allenap> intellectronica: I've added a comment. In most cases there's not a lot to see, except to see that it's sane.
<henninge> danilos: ok, got it.
<danilos> henninge, in essence, this is avoiding what is most likely a PG bug (the query that caused DB disk space to be eated returned ~1500 rows)
<danilos> s/eated/eaten
<intellectronica> allenap: sure, i just want to load the page, really, since we've seen in previous conversions that stuff got broken which wasn't covered by tests
<intellectronica> allenap: thanks for adding the info
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: danilo, allenap || queue: [adeuring]
* danilos changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: danilo, allenap || queue: [adeuring, danilo]
<henninge> danilos: review sent.
<henninge> danilos: forgot to sign it ...
<danilos> henninge, about EmptyResultSet, note that this is SelectResults (from SQLObject emulation layer), so I went with the simplest thing possible (I don't know if there's EmptySelectResults emulation in Storm)
<danilos> henninge, also, since this will set the statement_timeout to 5 minutes only for this run of the script (which should not be longer than 10 minutes), I won't touch that in order to keep the patch as simple as possible
<henninge> danilos: I have used it lately, I think ...
<danilos> henninge, with SQLObject code?
<henninge> not sure ...
<danilos> henninge, i.e. are you certain it has fetchall()?
<henninge> danilos: no, I am not.
<danilos> henninge, so, http://paste.ubuntu.com/270800/
<henninge> danilos: I see. Too bad.
<henninge> danilos: leave it as it is, then.
<danilos> henninge, well, the alternative is to port the query to Storm, but I ain't doing that to re-enable poimport :)
<henninge> no!
<henninge> just get that done quickly ... ;)
<beuno> intellectronica, care to take an easy-peasy review from me?
<intellectronica> beuno: sure, let me just finish allenap's first
<beuno> sure
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: danilo, allenap || queue: [adeuring, danilo, beuno]
<beuno> intellectronica, https://code.edge.launchpad.net/~beuno/launchpad/margin-tweaks/+merge/11689
<jtv> beuno, are you back already?  Hope you enjoyed it!  Just ran into your colon-lleno-de-galletitas translation while showing LP to a friend.
<jtv> henninge, intellectronica: can I chuck a tiny little branch onto your queue?
<beuno> jtv, I am, although I will be off again wed-fri  :)
<jtv> beuno: good for you!
 * jtv prepares to see "ui designer did not run from wed to sun" emails on the list
<intellectronica> jtv: sure, add yourself to the queue. i'm just finishing allenap's and then there's a patchli from beuno and by then i'm sure one of us can pick up yours
* jtv changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: danilo, allenap || queue: [adeuring, danilo, beuno, jtv]
<jtv> intellectronica: thanks... patchli?  I like that...  been to Switzerland recently?
<intellectronica> jtv: no, it's been too long. i miss the cheese
<jtv> intellectronica: of the country's 3 main exportsâclocks, cheese & bankingâthis is the only one with holes.
<intellectronica> huh :)
<intellectronica> allenap: r=me ui=me. let's take care of those two XXXs and please, give me back that sane human-readable formatting
<allenap> intellectronica: Thanks :)
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: danilo, beuno || queue: [adeuring, danilo, jtv]
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adeuring, beuno || queue: [danilo, jtv]
<intellectronica> beuno: r=me
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adeuring, jtv || queue: [danilo]
<intellectronica> jtv: 429272 ?
<beuno> intellectronica, thanks. Do you know how sending through EC2 works now that it's part of the tree?
<intellectronica> beuno: i didn't even know it's part of the tree. i use the same command i've always been using
<jtv> intellectronica: yup
<beuno> will do the same then
<jtv> wooowww... a cloud just started falling down here
<jtv> diagonally
<intellectronica> jtv: can you give me some sample URLs to look at?
<jtv> intellectronica: whoops yes, of course.  Try 1) https://translations.launchpad.{dev,net}/+imports
<jtv> 2) /~intellectronica/+imports
<jtv> 3) uh...
<jtv> https://translations.edge.launchpad.net/colon-full-of-cookie/trunk/+pots/colon-full-of-cookie/de/+filter?person=barry
<jtv> vs local example: 
<jtv> https://translations.launchpad.dev/alsa-utils/trunk/+pots/alsa-utils/es/+filter?person=carlos
<danilos> intellectronica, thanks for skipping on my review request :(
<intellectronica> omg, i never saw this project before, bizarre
<intellectronica> danilos: you requested a review from me?
<danilos> intellectronica, not from you, I've put it in the queue
<danilos> intellectronica, for either of the two OCRs to pick it up (if it was only one person, I'd probably ping you, but with two?)
<intellectronica> danilos: sorry, allenap, beuno and jtv personally asked me for a review and i took on their reviews in the order i promised them
<intellectronica> danilos: in the future i recommend pinging as many people as you can. try to sound desperate and blonde. it helps
<danilos> intellectronica, ok, I'll make a note to self to be much more "aggressive" in the future
<intellectronica> but i'll also follow the queue more closely
<danilos> intellectronica, thank you
<henninge> adeuring: do you care to quickly explain to me where the files in your branch are used? I have no clue about what hwdb about.
<henninge> I may have a guess, but ...
<adeuring> henninge: the schema is used in l/c/l/scripts/hwdbsubmissions.py
<henninge> adeuring: what does it do?
<adeuring> henninge: it is used to ensure that data submitted by the HWDB client has "sane" data
<adeuring> henninge: so that the processing script can make some aussptions that some data exists
<henninge> adeuring: got your headset ready?
<henninge> ;-)
<adeuring> henninge: yes
<intellectronica> jtv: r=me ui=me good stuff
<jtv> intellectronica: thx
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adeuring, danilo || queue: []
<intellectronica> jtv: i wonder how we can discover more pages like this. the test suite is unlikely to help, because it tests for the presence of the first title, not the absence of the second
<jtv> intellectronica: we could have an automatic pagetest for double/omitted h1s I guess
<intellectronica> danilos: 428440 ?
<intellectronica> yeah, maybe that's something we should. there will be more pages like this
<danilos> intellectronica, yep
<intellectronica> let's talk to barry when he gets in
<intellectronica> danilos: please add a comment between the condition and the then-clause of the if statement to distinguish them
<danilos> intellectronica, sure
<intellectronica> danilos: the indentation on line 27/28 is a bit off
<stub> https://code.edge.launchpad.net/~stub/launchpad/kill-harder/+merge/11517
<intellectronica> danilos: other than that it's all nice and good and r=me
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adeuring, - || queue: []
<danilos> intellectronica, I don't see the problem with the indentation, what do you mean? (this is the default emacs python-mode does, and I never had complaints about it)
<danilos> intellectronica, thanks for the review!
<intellectronica> danilos: can't you see the the selves don't align?
<danilos> intellectronica, they do for me (both on MP page and in my editor)
<intellectronica> danilos: best is if you break immediately after the parenthesis
<intellectronica> danilos: really?!
<danilos> intellectronica, yeah
<intellectronica> danilos: wow, looking with a text editor it indeed looks fine, but not in the mp. strange
<intellectronica> maybe a chromium-related bug or something
<danilos> intellectronica, perhaps some font issue then?
<intellectronica> stub: so, is that a branch you want reviewed, or you were just testing your keyboard?
<stub> That is a branch I'd like reviewed
<intellectronica> danilos: yeah, but still, it's fixed-width, what could possibly go wrong
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adeuring, stub || queue: []
<danilos> intellectronica, I've seen "fixed-width" fonts which aren't really (a slight mistake in metrics, and...)
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, stub || queue: []
<henninge> intellectronica: wow, you are quick ... ;-)
<intellectronica> yeah, looks just fine in FF too
<intellectronica> henninge: yeah, the trick is to only accept very small patches
<intellectronica> stub: in line 117 it will be easier to read if you don't rely on coercion of the empty list to false and instead check for length
<intellectronica> stub: what's the deal with the unspecified slice? is that your way of saying you want to clone the list so that you can modify it while iterating over the existing copy?
<stub> yes - copy to be explicit I'm not modifying the source list.
 * stub fixes line 117
<intellectronica> stub: cool. r=me
<stub> ta :-)
 * stub adds a comment that pids is getting mutated.
 * henninge lunches
<adeuring> henninge: could you do a follow-up review of my previous MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-submissions-with-udev-data-2/+merge/11691 ?
<henninge> yes, put it in the queue, I have something else to finish first.
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: []
<henninge> intellectronica: do you know of any random behavior of extract_text when used in a doc test (as opposed to pagetest)?
<henninge> intellectronica: Ursinha observed that 
<intellectronica> no, what's the random behaviour?
<intellectronica> and what's the input?
<henninge> Ursinha: ^
* adeuring changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, stub || queue: [adeuring]
<henninge> intellectronica: oh no, sorry. that is a pagetest
<henninge> intellectronica: http://paste.ubuntu.com/270891/
<henninge> Ursinha: how does it fail?
* sinzui changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, stub || queue: [adeuring, sinzui, sinzui]
<henninge> intellectronica: She said she needed lines 11 to 19 to get it to work.
<henninge> intellectronica: but i just tried it without those lines and it works just fine.
<intellectronica> what's the point of that complicated setup? can you not simply import extract_text?
<intellectronica> heh, i should have asked about a million dollar
<intellectronica> so what is it that doesn't work?
<henninge> Ursinha: how did you come to know about this setup anyway? I didn't even know that was needed.
<henninge> intellectronica: She seems to be no responding atm.
<henninge> not
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue: [adeuring, sinzui, sinzui]
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, sinzui || queue: [adeuring, sinzui]
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: adeuring, sinzui || queue: [sinzui]
<Ursinha> henninge, bad connection, sorry
<Ursinha> I said, I saw that in page-helpers.txt
 * Ursinha checks the name of the file
<Ursinha> henninge, actually that file didn't tell me to do that, but I tried to use it without importing stuff and it didn't work, so I looked in that file to see where did it come from
<henninge> Ursinha: but extract_text is used all over Launchpad page tests so it should work.
<henninge> Ursinha: how did it fail exactly?
<bac> hey barry would you have time to do a 2nd UI review on some mailling list pages?  i've got pretty screenshots!
<Ursinha> henninge, it said it couldn't find it
<henninge> Ursinha: have you tried it again, just now?
<Ursinha> henninge, running this moment
<henninge> Ursinha: without the setup?
<Ursinha> henninge, yes
<Ursinha> henninge, and about the random behaviour, wasn
<barry> bac: i would!  my stack is a bit deep right now, but i'm starting to pop items.  sign me up
<Ursinha> wasn't'just with extract_text, but in simple output text, as I mentioned that happened with whitespaces it wanted me to put
<Ursinha> henninge, now it ran without errors
<bac> barry: i'll look around.  maybe post-vacation, wonderfully refreshed beuno has a spare moment for a UI review.  if not i'll hound you later.
<henninge> Ursinha: ok, I'd say that was some confusion.
<henninge> Ursinha: are you aware why it is better to use extract_text?
<Ursinha> henninge, I don't think so
<beuno> bac, I can take it!
<henninge> henninge: First: If some-one decides in the future to put a link on one word in the test, you'd have to add extra ... for the test to pass.
<henninge> with extract text, extra html is just filtered out.
<henninge> Ursinha: ^
<bac> beuno: intellectronica has already done a first UI review, so you're just mentoring:  https://code.edge.launchpad.net/~bac/launchpad/bug-421986-team-list-pages/+merge/11672
<Ursinha> henninge, right
<Ursinha> indeed
<henninge> Ursinha: Second: If the test fails it outputs *all* text that is saw instead.
<Ursinha> hmm
<henninge> Ursinha: with html this can be *a lot* of text.
<henninge> that makes reading the failure output of the text very, very hard.
<henninge> I had that once ...
<henninge> Ursinha: but with extract_text that html will get filtered out, too.
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui || queue: [sinzui]
<intellectronica> sinzui: i find the quoted "codename" in a column header a bit funny
<sinzui> intellectronica: it is consistent
<intellectronica> sinzui: really? where else does it appear?
<intellectronica> beuno: what do you think of it?
<Ursinha> henninge, http://paste.ubuntu.com/270911/
<sinzui> We show the "codename" in quotes on several pages, series +index, +series, 
<henninge> Ursinha: wonderful!
<Ursinha> henninge, :)
<henninge> Ursinha: saves me some typing I was just doing ... ;-)
<henninge> Ursinha: r=me
<beuno> intellectronica, I'm not crazy about it, although it is consistent, and I can't think of a much better way of doing it off the bat
<Ursinha> henninge, many thanks
<intellectronica> sinzui: i'd say we can remove the quotes, or just call the thing 'Version name'. i'm curious to hear what beuno and other people think about this
<intellectronica> when i saw it, i thought the version is called 'codename', and it made it hard to understand what's going on. if it wasn't quoted it would have been better
<beuno> intellectronica, is there a screenshot to look at?
<intellectronica> these quotes are a literary device indicating irony, or other cases where the obvious meaning is not the correct parsing. i don't think it should be used in user interface much
<intellectronica> beuno: let me prepare one for you
<abentley> sinzui: It would be nice not to show the quotes when there's no codename.
<intellectronica> beuno: https://devpad.canonical.com/~tom/version-codename-screenshot.png
<adeuring> henninge: I'll leave quite soon -- if you have questions about my branch, can you ask them via mail?
<sinzui> beuno: https://edge.launchpad.net/launchpad-registry/3.1
<beuno> I don't feel the quotes are needed in the column header
<sinzui> ^ abentley I do not see quotes being used
<intellectronica> sinzui: in https://launchpad.dev/firefox/trunk/+milestones the table is screwed. there's one th missing, it seems
<henninge> adeuring: I will, np.
 * sinzui looks
<abentley> sinzui: Neither do I.  Perhaps that's been fixed.
<sinzui> abentley: the phantom quotes that appeared a few months ago was an ajax issue
<intellectronica> sinzui: other than that ui=me
<intellectronica> sinzui: and r=me as well
<sinzui> I see the series column is missing.
<beuno> bac, ball is in your court
<bac> beuno: thanks!
 * bac looks
<Ursinha> henninge, the test suite run caused my notebook to freeze again
<henninge> Ursinha: wow
<henninge> Ursinha: Really the full suite or just this one page test?
<Ursinha> henninge, it happens about 50% times I run it
<Ursinha> henninge, I did bin/test -vvt lp.translations.*
<henninge> Ursinha: interesting ...
<henninge> Ursinha: what about running it in EC2?
<Ursinha> henninge, that's ok, I just do that before submitting
<henninge> Ursinha: ?
<Ursinha> I mean, I run the translations tests
<henninge> Ursinha: That must be a more general problem, though ...
* danilos changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui || queue: [sinzui, danilo]
<Ursinha> henninge, I think so, will try to do more research on it later
<henninge> Ursinha: yeah, run it through ec2test for now ...
<danilos> henninge, intellectronica, abentley: another branch in the queue, when you feel like it, I'd be more than happy to have it reviewed :)
* bigjools changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui || queue: [sinzui, danilo, bigjools (1 line change!)]
<abentley> intellectronica: Which sinzui are you reviewing?
<Ursinha> henninge, yeah... the freezing tests aren't stopping me to land my branch, I just wanted to report
<intellectronica> abentley: the one i claimed :)
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui, sinzui || queue: [danilo, bigjools (1 line change!)]
<abentley> sinzui: In lib/lp/registry/templates/product-distros.pt, you have "distrribution-series" instead of "distribution-series".
<sinzui> yuck
<barry> henninge, intellectronica, abentley do any of you have time for a 178 line review?
<abentley> barry: Sure.
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui, sinzui || queue: [danilo, bigjools (1 line change!), barry]
<barry> abentley: awesome, thanks.  will mp it now
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui, bigjools || queue: [danilo, barry]
* abentley changed the topic of #launchpad-reviews to: on call: henninge, intellectronica, abentley || reviewing: adeuring, sinzui, danilo || queue: [barry]
<henninge> bigjools: now you even got two reviews for one line ;-)
* henninge changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, danilo || queue: [barry]
<bigjools> henninge: I feel lucky
<intellectronica> sinzui, beuno: so, any decision on the "codename"?
<sinzui> I believe we remove quotes, we never show it is quotes
<sinzui> s/is/in/
<intellectronica> sinzui: i'm not sure i understand. you're going to remove the quotes from the column header?
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, - || queue: [barry]
<sinzui> I am removing it form the column header
<sinzui> beuno: I don't feel the quotes are needed in the column header
<sinzui> ^ beuno has spoken
<sinzui> sorry, that quote does not look like a quote, but it is a quote
<barry> abentley: mp sent
<abentley> barry: Did you specify me as a reviewer?
<barry> abentley: i did
<abentley> barry: Okay, I'll review it when I get the email.
<barry> abentley: thanks. it should be easy
<bac> beuno: re: your question about breadcrumbs...  the url https://launchpad.dev/~testing-spanish-team/+invitation/hwdb-team only has as single breadcrumb for the testing-spanish-team.  the rest of the traversal does not generate any.  if the breadcrumb list only has one element they are not displayed.
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, barry || queue: []
<bac> beuno: so that's the explanation of the current behavior.  what do you think the breadcrumb for that URL should be?
<beuno> bac, good question
<beuno> I'm in a call, waiting to go into another call
<beuno> so I'd say land it for now  :)
<bac> ok, i'll answer in the MP.  we can revisit as necessary.
<abentley> barry: You know that you can run lint anytime, right?
<barry> abentley: i always remember that after bzr send tells me there are lint problems ;)
<abentley> barry: Ah, okay.
<barry> abentley: if we didn't have so many fp's i'd say bzr send should barf if there are lint problems
<abentley> barry: I know the feeling.  Does your comment somewhat mean "I wish lint showed up earlier in the email"?
<bac> beuno:  when you get a chance, please update your vote on https://code.edge.launchpad.net/~bac/launchpad/bug-421986-team-list-pages/+merge/11672
<sinzui> barry: when you landed you title branch, did you write tests that verified titles like `+series : Mozilla Firefox`?
<beuno> bac, sure. As soon as I get DNS back
<barry> abentley: that would partly help because i wouldn't spend so much time writing the cover letter if i know i had lint to pick.  maybe also it could print the lint problems and then ask to continue or abort?
<barry> sinzui: there are a billion tests for that.  every story prints browser.title.
<abentley> barry: We could do that on the commandline, sure.  I think we'd have to display the lint, so folks would know if it was fp's.
<sinzui> barry: okay, I will not let this block me form landing branches
<barry> sinzui: fixing all those is what took so damn long for that branch
<barry> abentley: +1
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, - || queue: []
* bigjools changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, - || queue: [bigjools]
<bigjools> got a complicated-ish one when LP sees my MP mail, I'll be around to guide you through it whoever can look at it
<barry> abentley: thanks!
<abentley> barry: np
* sinzui changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, - || queue: [bigjools, sinzui]
<intellectronica> abentley: if it's ok with you i'm going off call. it's getting late and i still want to do some hacking this evening
<intellectronica> i'll take that to be the good old silent affirmative
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui, - || queue: [bigjools, sinzui]
<cprov> abentley: would you have time for another review ?
<abentley> cprov: I expect so.
<cprov> abentley: okay, thanks -> https://code.launchpad.net/~cprov/launchpad/bug-417159-build-ui-tweaks/+merge/11703
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [bigjools, sinzui]
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [bigjools, sinzui, cprov]
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: bigjools || queue: [sinzui, cprov]
<abentley> bigjools: Could you please document the parameters of newBinaryPublication and newSourcePublication?
<bigjools> abentley: yep
<bigjools> I have to run out for a bit, I'll be back in an hour
<abentley> bigjools: Why does newBinaryPublication ignore the status parameter?
<abentley> bigjools: (newSourcePublication too)
<cprov> abentley: ignoring the status seems to be the right thing to do, publishing records should always be created as PENDING. 'status' shouldn't be an argument.
<abentley> cprov: That sounds fine to me.
<cprov> abentley: callsites that need a different status should update the record explicitly (only needed by tests, IIRC)
<abentley> cprov: It looks like you're duplicating the functionality of canonical.launchpad.scripts.tests.run_script.  If possible, use run_script instead.
<cprov> abentley: I've not checked the diff, but yes, run_script() should be used instead of local subprocess call.
<abentley> cprov: I meant bigjools.
<abentley> bigjools: It looks like you're duplicating the functionality of canonical.launchpad.scripts.tests.run_script.  If possible, use run_script instead.
<cprov> abentley: no worries, it wasn't an offense ;)
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui || queue: [cprov]
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: cprov || queue: []
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: []
<abentley> cprov: I've approved the code.  Whether you think it needs UI review is up to you.
<cprov> abentley: thank you.
<cprov> abentley: I will go with ui=rs, probably, unless beuno has time ...
* sinzui changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [sinzui]
<sinzui> beuno: I need a UI review as well as a code review for https://code.edge.launchpad.net/~sinzui/launchpad/source-package-ui/+merge/11718
<beuno> sinzui, sure
<beuno> no h1?
<sinzui> hmm
<beuno> in general, it's nice
<sinzui> your right. there is no onw
<beuno> what is alsa-utils-blah?
<beuno> binary? diff?
<beuno> deb?
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui || queue: []
<sinzui> OMG
<sinzui> beuno: the <h1> would be: alsa-utils
<beuno> also, empty relationships are kinda sucky
<beuno> Urgency: Low Urgency
<beuno> can we drop the extra urgency?
<beuno> branch, can we have a bzr icon for "trunk"?
<sinzui> beuno: I really do not know the answers to these questions. No one on my teams knows sourcepackages. And we have no time to do real design
<beuno> ok, I'll just throw it out there
<beuno> you do whatever you can  :)
<beuno> edit icon next to the project instead of "change upstream link"?
<sinzui> beuno: That would be an error
<sinzui> beuno: Having looked at the link, I think you mean Branch, which is actually a productseries
<beuno> sinzui, yes
<beuno> sinzui, ui=me is yours
<beuno> I'll take whatever you manage to change
<sinzui> beuno: I don't think I can drop the redundant Urgency, it is in the Enum
<beuno> sinzui, I suspected as much
<sinzui> beuno: Should I really put a bzr icon next to a product series...maybe I should change the label to say series
<abentley> sinzui: "The HTML page title" seems ambiguous to me.  It could be an HTML fragment which is a page title, or it could be the text to use in the HTML title element.
<sinzui> abentley: yes, it is now ambiguous since barry's branch
<sinzui> abentley: it is still required though. (I reported a bu)
<sinzui> g
<abentley> sinzui: No, I mean that the docstring is ambiguous.
<sinzui> abentley: oh yes, that was true when I started the branch
<beuno> sinzui, change the label
<sinzui> abentley: It was designed to be <title>
<sinzui> beuno: NO the page will fail. barry did not remove the page_title requirement
<abentley> sinzui: All the instances of that docstring are in code that you added.
<barry> that is a bug
<sinzui> abentley: I'll remove them
<barry> i may get to fixing that today, but we'll see
<abentley> sinzui: Okay.
<sinzui> abentley: I am making them simple attrs
<sinzui>     page_title = 'Upstream links'
<sinzui> which I hope will become the breadcrumb when salgado gets his branch finished
<abentley> sinzui: Makes sense.  I did wonder why they were properties.
<sinzui> They use to put the object's displayname in them but that disappears yetsterday
<sinzui> beuno: story I was not saying no to your suggestion to change the label. I am bad at juggling conversations. I changed Branch => Series
* danilos changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui || queue: [danilo]
<danilos> abentley, another UI branch up if you've got time
* sinzui changed the topic of #launchpad-reviews to: on call: abentley || reviewing:  || queue: [danilo]
<abentley> danilo: sure.
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: danilo || queue: []
<abentley> danilo: Where?
<danilos> abentley, waiting for MP to show up
<abentley> danilos: Okay
<danilos> abentley, https://code.edge.launchpad.net/~danilo/launchpad/bug-429414/+merge/11729
<danilos> abentley, apparently, my last branch got some test breakages (my local copy didn't have the latest code by barry when I did a full test), so this one fixes a few of the failing tests from that branch as well
<danilos> abentley, mostly page titles
<abentley> danilos: Cool.
<abentley> danilos: r=me
<danilos> abentley, thanks
<abentley> danilos: np
<barry> sinzui, beuno wanna take a quick look at the new ~team/+invitations page?
<sinzui> barry: sure
<barry> sinzui: http://people.canonical.com/~barry/invite.png and http://people.canonical.com/~barry/noinvites.png
<sinzui> barry: what do you think about the edit icon in the last column
<barry> sinzui: i don't like it much.  that's the way the page currently is, and i couldn't think of anything brilliant to put there.  have any ideas?
<sinzui> no
<barry> yeah.  that's why i left it that way ;)
<sinzui> I presume the edit is the accept/decline edit page
<barry> yep
 * sinzui looks for bugs users may have reported
<sinzui> barry: our 15 minutes are up, and no user has reported an issue about it. The page fine
<barry> sinzui: fab, thanks
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: -|| queue: []
<bac> beuno: since we can ui=rs for mechanical changes can we also seek a single UI review from a non-beuno if we want a second pair of eyes?  the full mentored UI review is a killer for simple changes.
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: -|| queue: []
<bac> sinzui: would you have time to do a code review on my branch?
<sinzui> I do
<bac> cool.  i'll add you to the MP
<Ursinha> hi sinzui
<sinzui> Hi Ursinha
<Ursinha> (wrong channel :)
<bac> sinzui: https://code.edge.launchpad.net/~bac/launchpad/bug-429455-team-pages/+merge/11739
<bac> hi Ursinha
<Ursinha> hi bac
<bac> sinzui: i'm going to step out for a bit and will answer when i return.
<bac> sinzui: thanks for the comments
#launchpad-reviews 2009-09-15
<barry> bac: hi, you're on call tomorrow?
<bac> barry: indeed i am
<mwhudson> thumper: want to look at another simple ec2test branch?
<stub> https://code.edge.launchpad.net/~stub/launchpad/pending-db-changes/+merge/11756 , one line change. Diff at https://pastebin.canonical.com/22095/
<mwhudson> stub: why is the diff in the merge proposal so bogus?
<stub> mwhudson: Cause I targetted the wrong branch. Redoing the proposal now.
<mwhudson> stub: ah ok
<stub> mwhudson: https://code.edge.launchpad.net/~stub/launchpad/pending-db-changes/+merge/11757 should be better :)
<mwhudson> stub: approved
<stub> mwhudson: Ta. Some magic ec2test incantation you want me to use to submit it?
<mwhudson> stub: yeah, tell me your aws id?
<mwhudson> stub: and grab bzr+ssh://bazaar.launchpad.net/~mwhudson/launchpad/cache-the-download-cache
<mwhudson> stub: by "aws id" i mean the 12 digit numeric one
<stub> Merge it in or do I need a seperate branch?
<mwhudson> stub: separate would be best
<stub> 3409-8351-9589
<stub> Got that branch
 * mwhudson slaps elastifox around a bit
<mwhudson> stub: ok, can you run ../cache-the-download-cache/utilities/ec2test.py --headless -s ... (or whatever options you usually use) in your branches dir?
<mwhudson> stub: it should say "Using machine image version 16"
<stub> Yup. Its booting the image now.
<mwhudson> cool
<mwhudson> if you're running --headless it should let go much quicker
<stub> ../cache-the-download-cache/utilities/ec2test.py --headless --email=stuart.bishop@canonical.com --ignore-download-cache-changes -b launchpad=db-devel -s '[r=mwhudson][ui=none][bug=429306] replication_lag() returns an interval, not an integer'
<mwhudson> let me know if it falls over in a heap :)
<mwhudson> hm
<mwhudson> stub: did you see my questions?
<mwhudson> let me know if it falls over in a heap :)
<mwhudson> stub: has it detached yet?
<mwhudson> stub: er, your change just got merged into db-devel
<mwhudson> stub: did you have an ec2test already running against you pending-db-changes branch?
<stub> num
<stub> nup
<stub> didn't see
<stub> it has detached now
<stub> db-devel was the target. No I didn't have another ec2test run.
<stub> Ahh... the merge was my fault.
<stub> I'd sent off an earlier revision of that branch to pqm, and it took forever to land.
<stub> So the tests are still running
<mwhudson> stub: cool thanks
<sinzui> I believe I have  fix for the failing tests.
<mwhudson> oh god there are still failures :(
<mwhudson> sinzui: want me to review the fix?
<sinzui> mwhudson: once I confirm it
<sinzui> mwhudson: The problems are intermingled in my own branch that I was testing
<mwhudson> sinzui: cool, i'm not going anywhere for a while...
<sinzui> I toppled a view, but most of the errors are the one I understood danilo-afk was fixing
<sinzui> mwhudson: Can you take a look at http://pastebin.ubuntu.com/271344/
<mwhudson> sinzui: i'm happy to rs=me that
<mwhudson> sinzui: i don't really know all the details, but i trust you do :)
<sinzui> mwhudson: if I submit with a preemptive testfix, will it be picked up automatically now?
<mwhudson> sinzui: yes
<noodles775> gmb: will you have time to look at a pretty straight-forward 400 line code review?
<gmb> noodles775: I'll take a look, sure.
<noodles775> gmb: thanks, the MP should arrive in a few seconds.
<noodles775> gmb: here 'tis: https://code.launchpad.net/~michael.nelson/launchpad/429263-no-value-option/+merge/11768
<gmb> noodles775: Awesome, thanks. Will look in a sec.
<gmb> Ah, good old Zope. PEP 8 was something that happened to other people, wasn't it?
<noodles775> lol
<gmb> noodles775: r=me; looks good.
<noodles775> gmb: great, thanks!
<jtv> hey stub!  interested in reviewing a not-trivial-but-not-widespread fix for a problem that just popped up in the translations auto-approver? https://code.edge.launchpad.net/~jtv/launchpad/bug-429811
<stub> jtv: IMasterStore(POTemplate) is a better spelling - it should be rare to need the IStoreSelector now.
<stub> (line 73)
<jtv> stub: ah yes, thanks, I'd forgotten about that
<stub> jtv: You sure you want to start spitting warnings if you have more than 2 results? Perhaps you forgot a [:2] to the query, limiting it to a maximum of two rows returned.
<jtv> stub: yes, I think I'm sure.  Unfortunately there is still a case where you can validly have more results, and while I _could_ fold that in by sorting on a boolean, I don't think it's worth the complexity.
<stub> Your better off spitting out your warning explicitly in that case, rather than the 'bad programmer' errors hopefully emitted by shortlist.
<stub> I see we already cope sometimes, in that we proceed if there is only one preferred match, no matter now many results returned.
<jtv> right, and that's worth a warning.  This way I'm sure all the problem cases emit some kind of warning.
<jtv> Ahh wait, it's coming back to me now.
<stub> Just IIRC shortlist uses Python warn, rather than a nice log message
<jtv> A double match should be a rare occurrence, and is worth a warning.  A more-than-double match is downright "weird" and if the numbers get too big, mean that my approach may need the kind of query limit that you suggest, and in that case there'll be a shortlist warning.
<stub> So do shortlist(100) or something, and 'if len(foo) > 2: log.warn('Weird number of matches %d')
<jtv> How would that help?
<stub> You won't need some sort of test ensuring the Python warning framework warning is actually emitted correctly by your script in this case.
<stub> The shortlist warning is more a backup.
<stub> Its to catch developer errors, where the developer assumed the length is bounded to a sane number of results but it isn't really.
<jtv> Well if the shortlist warning doesn't get through, we'll still start seeing more and more of these warnings you see here.
<jtv> The shortlist warning would be an additional reminder that if there's been a big shift in what kind of numbers we expect here, it may be worth updating the code.
<stub> Sure. So the shortlist warning is redundant. You know this case can happen, and you handle it, so why emit noise if it does happen
<jtv> I think that's appropriate.
<jtv> How about I just jack up the expected size on the shortlist?
<stub> I don't. It is to catch developer errors. It spits out a warning to untested channels, possibly at an arbitrary point in the output stream.
<stub> Sure.
<stub> Everything else I didn't mention looks good :)
<jtv> Thanks.  Now about the store selector...  Is there no IDefaultStore?
<stub> IStore(POWhatever)
<stub> That gives you the default store.
<jtv> great
 * jtv runs tests
<jtv> tests pass
<jtv> no lint
<jtv> stub: for the shortlist I just removed the explicit expected-size argument, so the code really just says "this shouldn't be a long list, and if it is, consider re-balancing the code."
<jtv> That okay with you?
<stub> Sounds good.
<jtv> Thanks.  And how's the coffee?  :)
<stub> Great :-)
<stub> Maybe John won't get his delivery ;)
<jtv> lol
<jtv> Robin has a grinder, so I've been enjoying it myself
* cprov changed the topic of #launchpad-reviews to: on call: - || reviewing: -|| queue: [cprov]
<bac> gmb are you on call today?
<bac> i'll be on in 15 minutes.
* bac changed the topic of #launchpad-reviews to: on call bac: - || reviewing: -|| queue: [cprov]
<bac> hi cprov
<cprov> bac: hi there
<bac> cprov: is this the one you've queued up?  https://code.edge.launchpad.net/~cprov/launchpad/bug-421364-build-publishing-status/+merge/11751
<cprov> bac: yes, that's it
* bac changed the topic of #launchpad-reviews to: on call bac: - || reviewing: -cprov|| queue: [-]
<bac> i'll get right on it
<cprov> bac: thank you.
<bac> cprov: your branch has a merge conflict
* bac changed the topic of #launchpad-reviews to: on call bac: - || reviewing: cprov|| queue: [-]
<cprov> bac: uhm, sorry, let me fix it, one sec
<bac> cprov: np.  lots of branches hitting fast and furious
<BjornT> bac: hi. i have two branches ready for review. one is on +activereviews already, and one should appear there soon
<bac> BjornT: ok
* BjornT changed the topic of #launchpad-reviews to: on call bac: - || reviewing: cprov|| queue: [BjornT, BjornT]
<cprov> bac: conflict fixed, branch pushed.
<bac> thanks cprov
<bac> cprov: i'm wrapping yours up.  need to be afk for a few minutes.  brb
* bac changed the topic of #launchpad-reviews to: on call bac: - || reviewing: BjornT || queue: [BjornT]
* bac changed the topic of #launchpad-reviews to: on call bac: - || reviewing: BjornT  || queue: [BjornT]
* bac changed the topic of #launchpad-reviews to: on call bac || reviewing: BjornT  || queue: [BjornT]
<barry> bac: hiya!  up for a review this fine morning?
<bac> barry: throw it on the pile
<barry> bac super!
<bac> hey BjornT could you fix the conflicts in your windmill branch and repush please?
* barry changed the topic of #launchpad-reviews to: on call bac || reviewing: BjornT  || queue: [BjornT, barry]
<BjornT> bac: sure
* sinzui changed the topic of #launchpad-reviews to: on call bac || reviewing: BjornT  || queue: [BjornT, barry, sinzui]
<BjornT> bac: done. i'll resolve the conflicts in the other branch as well; turned out my RF copy was old
<BjornT> bac: both branches are clean now
<bac> thanks BjornT
<bac> BjornT: when i try to merge your windmill branch r9244 into RF r9446 i still get two conflicts
<BjornT> bac: well, i guess i should push up the new revision as well :) done
<bac> BjornT: that magically worked!
<adeuring> bac: can I add an MP to the queue?
<bac> adeuring: sure
<adeuring> https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-parse-submission-udev-node/+merge/11783
<adeuring> bac: thanks!
<bac> adeuring: please add me to the MP
* adeuring changed the topic of #launchpad-reviews to: on call bac || reviewing: BjornT  || queue: [BjornT, barry, sinzui, adeuring]
<bac> sinzui: did i have a ui=sinzui on https://code.edge.launchpad.net/~bac/launchpad/bug-429455-team-pages/+merge/11739 ?
<sinzui> yes
<bac> sinzui: thanks
* bac changed the topic of #launchpad-reviews to: on call bac || reviewing: BjornT  || queue: [barry, sinzui, adeuring]
<bac> hi BjornT
<bac> BjornT: in test.in you say "the option probably wasn't specified".  why the uncertainty?
<BjornT> bac: i'm on a call now
<bac> BjornT: ok, i'm here all day.  :)
<adeuring> bac: can I add another small branch to your queue?
<bac> adeuring: sure
<adeuring> bac:  thanks!
<adeuring> beuno: may I ask you for a small ui review: https://code.edge.launchpad.net/~adeuring/launchpad/bug-430054-hassprints-sprints.pt-lp3-layout/+merge/11795 ?
<beuno> adeuring, sure
<adeuring> beuno: thanks!
<beuno> adeuring, done
<adeuring> beuno: wowm, that was fast !
<beuno> adeuring, the 3.0 train is now going at 400 km/h
<adeuring> beuno: ;)
* bac changed the topic of #launchpad-reviews to: on call bac || reviewing: barry  || queue: [sinzui, adeuring]
<bac> beuno: that 3.0 train isn't Amtrak...
<bac> just ask barry
<barry> let's hope not.  otherwise 3.0 will be 50% over time and have backed up toilets for half the ride.  but at least you won't be mid-air suspended in a flimsy aluminum tube breathing other people's swine flu for 1/10th the time
<BjornT> bac: i'm back now. so, there's some uncertainty, since it might be that the user specified the real default value (as specified in zope.testing.testrunner.options)
<bac> BjornT: i sent the review and asked for more info
<BjornT> bac: for example, if someone did bin/test --tests_pattern=^tests$, we would replace that with our --tests-pattern default. i'd say it's a minor issue
<bac> BjornT: ok
<bac> BjornT: that might warrant a small explanation in the comment.  seeing stuff like "probably" makes people nervous.
<BjornT> bac: yeah, i guess :)
<BjornT> bac: as for the other review. what's your use case for building on the current default options. what are you trying to do?
<bac> BjornT: unsure.  i'm just noting you're changing the behavior
<BjornT> bac: indeed. but i haven't come up with any feasible use cases for the current behaviour.
<jtv> bac: got one for you, and the next OCR I guess is myself.  :)  can I chuck it on the queue?
<bac> jtv: yes.
<jtv> bac: thanks
* jtv changed the topic of #launchpad-reviews to: on call: bac || reviewing: barry  || queue: [sinzui, adeuring, jtv]
<bigjools> abentley: did my review response look ok? if so can you approve it please :)
<abentley> bigjools: So far, I haven't received an email, but I'll go find it.
<bac> sinzui: review done.  thanks.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: barry  || queue: [adeuring, jtv]
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: barry  || queue: [adeuring, jtv,sinzui]
<sinzui> bac: I just sent another one
<bac> one step forward...
<bigjools> abentley: it doesn't list you as a review, that's why...
<bigjools> abentley: https://code.edge.launchpad.net/~julian-edwards/launchpad/ppa-copy-to-main-bug-426163/+merge/11705
<bac> sinzui: could you mark the MP approved: https://code.edge.launchpad.net/~bac/launchpad/bug-429455-team-pages/+merge/11739
<sinzui> done
<abentley> bigjools: r=me
<bigjools> abentley: cheers
<BjornT> bac: could you take another look at lp:~bjornt/launchpad/bug-429375? i've addressed your comments
<bac> BjornT: ok
<adeuring> bac: can I add another small MP?
<bac> adeuring: sure
<adeuring> bac: thanks!
<bac> barry: after the merge TeamInvitationView has two label properties
<bac> barry: and two page_titles
<bac> :(
<barry> bac: dang.  let me work on that
<beuno> adeuring, why is there an edit page to delete something?
<beuno> refering to http://people.canonical.com/~adeuring/spec-branch.png
<adeuring> beuno: I have no idea... As I wrote in the MP, there was also a button "update" on that page. Perhaps it had some time ago some field that could be edited
<beuno> I've heard rumours from flacoste that anything in blueprints is ui=rs, but I don't know how true that is
<beuno> adeuring, how much work would it be to change it to reflect reality?  (eg, rename it to "delete")
<adeuring> beuno: should be easy. Give me a few minutes...
<flacoste> beuno: blueprints UI reviews are rs=
<barry> bac: fix pushed
<bac> thanks
<adeuring> flacoste, beuno: OK, will rs=... further brnaches ;)
<beuno> adeuring, the second bext iluminati player in Canonical has spoken
<beuno> s/bext/best
<beuno> adeuring, I'm still happy to help out if you need me
<adeuring> beuno: the heading is changed
<adeuring> screenshot uploaed (old URL)
<beuno> adeuring, thanks
<beuno> land!
<adeuring> beuno: I'm waiting for the code reiview... Brad has a lot to do today...
 * adeuring notices that the heading is still nonsensical ("Delete specification nranch summary")... 
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: bjornt  || queue: [adeuring, adeuring,jtv,sinzui, adeuring]
<bac> BjornT: done
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: adeuring  || queue: [adeuring,jtv,sinzui, adeuring]
<bac> adeuring: the diff for this MP is messed up:  https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-parse-submission-udev-node/+merge/11783
<bac> adeuring: never mind, i generated a new one
<adeuring> bac: OK, was going to paste-bin one...
<leonardr> gary_poster: i have to leave for dinner in 1/2 hour, no way i'm getting through that queue. are you interested in reviewing https://code.edge.launchpad.net/~leonardr/wadllib/optional-field?
<gary_poster> leonardr: looking now
<leonardr> look at bug 430152, working on a merge proposal now
<mup> Bug #430152: Multipart representation creation crashes when an "optional" field has no value specified <wadllib:New> <https://launchpad.net/bugs/430152>
<gary_poster> ok
<gary_poster> leonardr: yes
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: adeuring  || queue: [jtv,sinzui, adeuring]
<leonardr> gary, i just pushed another branch that updates version.txt
<gary_poster> leonardr: 
<gary_poster> ok
<bac> adeuring: another done
<adeuring> bac: thanks!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: <lunch>  || queue: [jtv,sinzui, adeuring]
<gary_poster> leonardr: r=gary.  I'd have used self as the missing flag, rather than creating a "missing" object for this because it looks like it is part of a loop that might be run a lot.  However, that may be completely unnecessary, and the "missing" name makes the usage clear.
<gary_poster> leonardr: about to run out for lunch
<leonardr> gary: ok. fwiw, 'missing' is defined outside the loop
<gary_poster> leonardr: I saw that, but this method is not called as part of a loop (for each tag or something?)
<leonardr> gary: no, it's called when you want to invoke the operation
<gary_poster> leonardr: ...from lazr.restful's perspective, every time?  or is this a one time parsing?  (Again, really just curious.)
<gary_poster> (one-time parsing for the process)
<leonardr> gary: every time, because it's processing the values you provided when you invoked it
<gary_poster> leonardr: oh.  ok.  thanks.
<gary_poster> have a nice evening.
<leonardr> thanks
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui  || queue: [jtv, adeuring]
<noodles775_> bac: It's fine if you don't get to it, but can I put one on the queue even though I won't be around for question?
* deryck changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui  || queue: [jtv, adeuring, deryck]
<noodles775_> bac: https://code.launchpad.net/~michael.nelson/launchpad/429353-site-message-to-footer/+merge/11813
* noodles775_ changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui  || queue: [jtv, adeuring, deryck, noodles]
<bac> noodles775_: ok
<noodles775_> Thanks.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: adeuring  || queue: [jtv, deryck, noodles]
<bac> sinzui: are you doing a ui=rs for your oath branch?
<sinzui> hmm
<sinzui> bac: I am for blueprints (all blueprints are rd)
<sinzui> bac: I am for blueprints (all blueprints are rs)
<bac> sinzui: but what about the oath branch?  if you aren't getting a UI review i'll mark the MP as approved
<sinzui> bac: sorry. I am on a call
<sinzui> Yes I want to two UI reviews.
<salgado> sinzui, my branch converting person-index.pt is up for (code/UI) grabs.  the diff has around 1000 lines, can you take it?
<sinzui> salgado: yes. I would love to stop
<sinzui> salgado: yes. I would love to stop coding for the day
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv  || queue: [deryck, noodles]
<bac> adeuring: review done
<adeuring> bac: thanks!
<salgado> thanks sinzui!
<sinzui> barry: rockstar: Can either/both of you take a look at the pictures for my branch. The work was mechanical https://code.edge.launchpad.net/~sinzui/launchpad/person-review-oath/+merge/11801
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv  || queue: [deryck, noodles,sinzui]
<barry> sinzui: sure
<barry> sinzui: although, you don't need a ui review for mechanical changes
<sinzui> barry: correct, but there was something wired in these pages...
<barry> sinzui: give me the urls and i'll look
<sinzui> https://code.edge.launchpad.net/~sinzui/launchpad/person-review-oath/+merge/11801/comments/30656/+reply
<sinzui> ^ I think you want to read this to understand that IAccount is not traversable, so it does not get tabs
<sinzui> barry: ^
<barry> sinzui: hmm. why does it follow that if IAccount is not traversable, the page gets no app tabs?
<sinzui> barry: We gave LOSAs a hack to manage user accounts...this is outside of launchpad. To make tabs we need to made a facet menu and a canonical URL. We will never make the later
<barry> sinzui: that makes better sense :)  ui=barry*
<sinzui> thanks
<rockstar> barry, thanks for getting to that.  I'm trying to get lots of stuff done.
<barry> rockstar: no worries
<barry> bac: shite.  another text conflict found :(
<sinzui> salgado: ping
<salgado> sinzui, pong
<sinzui> salgado: I like the treatment of links (related projects ...)
<sinzui> salgado: can we add a link for the owner to mange is oauth-tokens to it
<salgado> sinzui, on the heading slot?
<sinzui> salgado: I was thinking of adding the link to the list that has Related projects and PPA packages
<salgado> right, that's in the heading slot
<salgado> I can do that
<sinzui> salgado: nothing should use the heading-slot. maybe I misunderstand you. I am sure you know the list
<sinzui> salgado: this is the bug I was thinking of https://bugs.edge.launchpad.net/launchpad-registry/+bug/316731
<mup> Bug #316731: provide a link to the +oauth-tokens page on users page <api> <feature> <ui> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/316731>
<salgado> sinzui, team-index.pt (and now person-index.pt) are filling the heading slot with these links
<sinzui> I see
<sinzui> salgado: barry's work deprecated the slot. I think we need to move them.
<sinzui> salgado: they can move before the first <div class="yui-g"> like the distro and product page
<salgado>   <div
<salgado>     class="description"
<salgado>     tal:condition="context/homepage_content"
<salgado>     tal:content="structure context/homepage_content/fmt:text-to-html"
<salgado> sinzui, that's in the heading slot too.  should it be moved to the 'main' slot?
<sinzui> yep
<sinzui> I saw that. The markup will look a lot like the distro and product page after the move
<bac> sinzui: you have two branches on +activreviews.  which would you like me to review?
<sinzui> both
<bac> er, first?
<sinzui> do the blueprint first
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: jtv  || queue: [deryck, noodles,sinzui,sinzui]
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui  || queue: [deryck, noodles,sinzui]
<bac> sinzui: i'm letting you jump b/c deryck's is super long (but easy) and the others are absentee
<sinzui> bac: blueprint changes are auto ui=rs so I think it will be quick
<bac> sinzui: thanks for the screenshots
<sinzui> bac I should have take before examples so that you could see the overlaping text I was seeing.
<bac> sinzui: before captures are handy for the reviewer and as reference for me when i'm working, so i got into the habit of doing them for all pages
<bac> sinzui: i know you didn't touch it, but at line 109 the use of the plural "them" to mean "the person whose gender we don't know" is grating.  could you have a go at rewording that?
<sinzui> I can
<sinzui> bac: I need to pick my children up. I'll be back in 30 minutes
<bac> ok
<bac> sinzui: the branch for https://code.edge.launchpad.net/~sinzui/launchpad/person-blueprint-templates/+merge/11753 has already landed?
<salgado> sinzui, I've updated both templates to not fill the heading slot
* henninge changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui  || queue: [deryck, noodles, sinzui, henninge]
<henninge> bac: I added a branch of mechanical 3.0 changes if that is ok?
<bac> henninge: ok
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: deryck  || queue: [noodles, henninge]
<sinzui> bac: It has?
 * sinzui looks
<bac> sinzui: yeah, it landed just before i approved it...
<sinzui> bac: I see. My test fix has not...
<bac> sinzui: you've been landing so much stuff, perhaps your fingers just typed it while you weren't looking
<sinzui> bac: I am sorry, I sent the wrong window off to PQM
<sinzui> and my text change was not in it of course
<bac> sinzui: no worries.  it's approved anyway
<bac> sinzui: perhaps you can bundle it up elsewhere...
<sinzui> My test re-enable branch comes to mind.
<henninge> bac: thanks. I have to go to sleep now, so please email any questions.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: deryck  || queue: [noodles, henninge, <ask before adding any more>]
<bac> henninge: will do
<henninge> cheers
<sinzui> salgado: I do not think you add person-portlet-involvement.pt to the tree
<salgado> sinzui, I didn't.  I renamed portlet-details to portlet-involvement and later removed it
<salgado> does the diff say I added that?
<sinzui> salgado:config says it does not exist
<sinzui> salgado: I cannot start the server
<salgado> sinzui, oh, right, I forgot to remove it from zcml
<salgado> sinzui, please merge/push again and it will work
<bac> hi deryck
<deryck> hi bac 
<bac> deryck: in bug 427928 you said anecdotal evidence said no one used mentorship.  did you run a query to confirm that?  i'm just curious.
<mup> Bug #427928: Templates for mentoring should be removed in 3.0 conversion <Launchpad Bugs:In Progress by deryck> <https://launchpad.net/bugs/427928>
<deryck> bac, no, I didn't.  See flacoste's recent stats on the bug page redesign which do confirm this.
<bac> ok
<bac> deryck: i'm unclear how complete  this removal aims to be.  I see BugTaskView still extends CanBeMentoredView
<deryck> bac, it's not meant to be complete at all.  it's just to remove links to the feature and templates.  the least possible to do that.
<deryck> bac, still a long diff to do that, though :)
<bac> deryck: sure.  do we have a bug to get rid of the rest of the cruft?  i'd hate for 'out of sight, out of mind' to mean that we don't clean up completely
<deryck> bac, no, not yet.  but flacoste and kfogel are tracking the idea with stats and an upcoming blog post.  So if all that turns out as expected, a bug will be filed at that point.
<bac> deryck: ok
<bac> deryck: will you swear on bear bryant's hat that you'll run this through ec2test?
<deryck> bac, yeah, but since I'm an Auburn guy, that doesn't gain you much ;)
<deryck> bac, but I don't commit without ec2test.
<bac> fair enough
<bac> deryck: i recall seeing auburn play once.  cotton bowl, 1986
<deryck> bac, probably not a bad year for Auburn then, IIRC.
<bac> deryck: cold and icy.  1st and goal on about the five and they ran bo jackson up the middle four times into a brick wall.
<deryck> heh
<deryck> bac, ah, the good ol' days ;)
<bac> deryck: for auburn and a&m
<bac> deryck: the branch looks good.  r=bac
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: noodles  || queue: [henninge, <ask before adding any more>]
<deryck> bac, thanks
<kfogel> bac: posted that blog post, btw
<bac> kfogel: which?
<kfogel> http://blog.launchpad.net/general/removing-mentoring
<kfogel> bac: the one deryck was mentioning
<bac> kfogel: ah, right.  thanks
<kfogel> bac: by the way, I had tried to put an image into that post (see http://www.red-bean.com/kfogel/canonical/mentoring-button.png) but couldn't get it to show up.
<kfogel> bac: any idea how to do that?  I uploaded it twice; I was looking at the HTML codes, and they seemed reasonable to me.  But it was never visible.
<bac> kfogel: i don't.  mrevell would be the answer man
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: [-]
 * bac calls it a day
#launchpad-reviews 2009-09-16
<stub> Simple review needed: https://code.edge.launchpad.net/~stub/launchpad/opstats/+merge/11774
<mwhudson> stub: don't we already have code for telling a robot apart from a browser?
<mwhudson> stub: the oops reports make this distinction somehow or other...
<stub> mwhudson: The oops reports use a list of known robots. I was thinking that for the losas use case, it is better to have false negatives (requests from unknown browsers ignored) than false positives (requests from robots counted because they are unknown)
<mwhudson> stub: ok, fair enough
<stub> I toyed with parsing one of the databases (the oops reports just uses values scraped from our web logs), but it seemed overkill.
<mwhudson> yeah, looks good enough for a first cut
<mwhudson> stub: reviewed
<stub> ta
<BjornT> stub: what about api scripts? shouldn't they be treated as browsers as well?
<jtv> henninge-bbl: the comment you ported over from pagetitles.py to the POFileTranslateView label property does not, in actuality, make sense.  Might as well drop it.
<jtv> henninge-bbl: Also, in menu-pages.txt, afaics the nav link from the +details page back to the +details page seems to be linkified now... is that right?
<stub> BjornT: That would be up to the losas I guess. They requested this for their monitoring. I suspect not, as I think the goal is to filter out the noise that can be generated from automation like robots and scripts etc.
<BjornT> stub: the api is used by real application as well, though, so if the api doesn't work, it has a direct effect to users. but sure, it's up to the losas to decide, it depends on what exactly they intend to measure
* danilos changed the topic of #launchpad-reviews to: on call: - || reviewing: [danilo, danilo]
* danilos changed the topic of #launchpad-reviews to: on call: - || reviewing: [danilo, danilo, danilo]
<danilos> allenap, jtv: three lovely UI branches for your enjoyment on +activereviews
* adeuring changed the topic of #launchpad-reviews to: on call: - || reviewing: [danilo, danilo, danilo, adeuring]
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: [danilo, danilo, danilo, adeuring, noodles]
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: [danilo, danilo, danilo, adeuring]
* adeuring changed the topic of #launchpad-reviews to: on call: - || reviewing: [danilo, danilo, danilo, adeuring, noodles, adeuring]
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: [danilo, danilo, danilo, adeuring, noodles, adeuring]
<intellectronica> allenap: you're back
<allenap> intellectronica: Yes! Thanks for passing on my message.
<intellectronica> allenap: can you review my patch before danilo danilo and danilo? i expect preferential treatment because we're team mates
<intellectronica> also it's a one-liner
<noodles775> lol
<allenap> intellectronica: Of course.
<intellectronica> allenap: http://pastebin.ubuntu.com/271996/
<intellectronica> allenap: this is for turning ubuntu +filebug off on edge
* noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: [danilo, danilo, danilo, adeuring, adeuring]
<allenap> intellectronica: r=me and soon Ubuntu will be bug free, at least in the same way that Windows and MacOSX are bug free.
<intellectronica> ha ha
<intellectronica> allenap: thanks
* adeuring changed the topic of #launchpad-reviews to: on call: allenap || reviewing: [danilo, danilo, danilo, adeuring, adeuring, adeuring]
<barry> allenap: looks like you have a pretty full plate today
<allenap> barry: Yeah :) And I've hardly even started.
<barry> ouch ;)
<barry> s'okay.  i'll harass sinzui when he wakes up :)
<allenap> barry: Cool, okay, thanks for that. Once I get into the flow it'll go okay, so maybe I'll get to you if sinzui isn't able to help.
<barry> allenap: cool, thanks.  i'll put myself on the queue just in case
* barry changed the topic of #launchpad-reviews to: on call: allenap || reviewing: [danilo, danilo, danilo, adeuring, adeuring, adeuring, barry]
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: lunch || queue: [danilo, danilo, danilo, adeuring, adeuring, adeuring, barry]
<danilos> intellectronica, hehe, btw, we are all teammates :)
<intellectronica> danilos: i'm sorry, which department exactly are you?
<danilos> intellectronica, dammit, the office right below you!
<intellectronica> :)
<danilos> intellectronica, second floor, where it says "soft launching pads"
<intellectronica> oh right, i remember, i always go there to get my pink 83B5 forms
<danilos> there you go :) we've got all stuff pink in here!
<adeuring> allenap: can I add another review (like the other, a trivial layout conversion)?
<beuno> bigjools, want to review: https://code.edge.launchpad.net/~beuno/launchpad/bug-423105/+merge/11871
<beuno> I'm not really here though
<bigjools> beuno: r=me
<allenap> adeuring: Sure, please put it on the end. I may not get to it today though.
<beuno> bigjools, thanks
* adeuring changed the topic of #launchpad-reviews to: on call: allenap || reviewing: lunch || queue: [danilo, danilo, danilo, adeuring, adeuring, adeuring, barry, adeuring]
<cprov> eew, the review queue is scary ...
<danilos> cprov, then just add to it
<cprov> danilos: right, why not ?
* cprov changed the topic of #launchpad-reviews to: on call: allenap || reviewing: lunch || queue: [danilo, danilo, danilo, adeuring, adeuring, adeuring, barry, adeuring, cprov]
<danilos> allenap, also, I'll try to get henninge to review one of my branches (430455), so if I do, I'll get it out of the queue
<allenap> danilos: Thanks :)
* henninge changed the topic of #launchpad-reviews to: on call: allenap || reviewing: lunch || queue: [danilo, danilo, adeuring, adeuring, adeuring, barry, adeuring, cprov]
<barry> sinzui: would you have time to review a branch for me?  allenap has a full dance card
<sinzui> I can
<danilos> allenap, btw, blueprints branch is mostly removing templates and replacing them with generic-edit
<allenap> danilos: Yep, I'm just approving it now.
<danilos> sinzui, btw, I wanted to ask you about some thing... I've also migrated sprint-edit.pt to generic-edit.pt, but it used to have an old contextmenu with "Change branding" link; should I keep that template as is if you want to have "Change branding" link only once you are already on "Change details" form?
<barry> sinzui: thanks: https://code.edge.launchpad.net/~barry/launchpad/430068-person/+merge/11832
<danilos> allenap, cool, thanks
<sinzui> danilos: I changed a few context menus into navigation menus. I agree that is slow
<sinzui> danilos The change branding should be in the global action menu after Change details.
<danilos> sinzui, right, I am wondering if it was intentional that the object-branding.txt pagetest was doing getLink('Change details').click() and immediately after getLink('Change branding').click()
<danilos> sinzui, with my conversion, I've removed the first getLink because 'Change branding' is available on the sprint page as well, is that fine?
<sinzui> danilos ah. that is because of the old design. and that hack we kept in place so that we could change pages before the index pages are complete
<danilos> sinzui, ok, cool, then I'll be removing 12 blueprints templates shortly
<sinzui> danilos: registry has a similar problem. Lots of tests will break when we fix person +edit. We need the +index to land first with all the new links in it.
<danilos> sinzui, right
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: danilo || queue: [adeuring, adeuring, adeuring, barry, adeuring, cprov]
<noodles775> Hi bac - sorry, I had to update that branch you reviewed yesterday. Do you think you'll get time to look at the incremental?
<bac> noodles775: yes.  i've got a standup, then the reviewers meeting, then a 11EDT call.  i'll try to squeeze it in the cracks
<noodles775> Thanks bac!
* allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [adeuring, adeuring, adeuring, barry, adeuring, cprov]
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: - || queue: [adeuring, adeuring, adeuring, barry, adeuring, cprov]
<leonardr> gary, whenever you have time, https://code.edge.launchpad.net/~leonardr/lazr.restful/new-version/+merge/11882 and then https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/new-restful/+merge/11881
<gary_poster> https://code.edge.launchpad.net/~leonardr/lazr.restful/new-version/+merge/11882 : do you think we ought to have migration instructions, or is that unnecessary?
<leonardr> gary: old services will still work, so no migration necessary
<leonardr> i could be persuaded to write something about how to use the new stuff
<gary_poster> leonardr: heh.  how long would such usage instructions be, on a guess?
<leonardr> gary: i'd do it by rewriting lazr/restful/example/wsgi/README.txt
<leonardr> it'd end up a little shorter than it is now
<leonardr> take me a couple hours to write
<gary_poster> leonardr: that feels appropriate to me.  WDYT?
<leonardr> sure, i'll finish off the day with that. can i have a provisional r=gary if that's stipulated?
<gary_poster> yes, done.  I'll mark it as such and move on.
<sinzui> salgado: I have one question. The picture show the IRC nicks wrapping together. I was going to suggest making each nick a <dd>, but I see in your diff that they are <div>. Was the picture made before you switch to a <div>?
<leonardr> all right
<gary_poster> leonardr: one other question here actually.  is this change appropriate for a 0.9.6 (bugfix)or a 0.10.0 (feature)?  This feels like a 0.10.0 to me.
<salgado> sinzui, what do you mean by wrapping together?  in the shot they are rendered inside <div>s
<sinzui> ah!
<salgado> sinzui, IRC nicks are rendered like "nick on network"
 * sinzui drinks more coffee.
<salgado> it will look better with nicks that don't have spaces
<sinzui> salgado: If we change each nick to be a <dd> they will get a few pixels of whitespace between them. Your decision. r=me. start landing this
<salgado> thanks sinzui.  I'll see how that looks
<gary_poster> leonardr: also approved the other one
<leonardr> great
* adeuring changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: - || queue: [adeuring, adeuring, adeuring, barry, adeuring, cprov, adeuring]
<adeuring> allenap, EdwinGrubbs: could one of you take a look at one of my MPs (all boring conversions of blueprint templates...)
<EdwinGrubbs> adeuring: sure
<adeuring> EdwinGrubbs: thanks!
<allenap> adeuring: Sorry I haven't started on yours yet.
* allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [adeuring, adeuring, adeuring, barry, adeuring, cprov, adeuring]
<adeuring> allenap: np. Must be a terribla day for you...
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: adeuring || queue: [adeuring, adeuring, barry, adeuring, cprov, adeuring]
<allenap> adeuring: It's not terrible, but I have many things on the go at the same time :(
<bac> ni noodles775
<noodles775> hi bac
<bac> noodles775: i was just about to have a look at your changes when i talked to sinzui and he mentioned bug 430784
<mup> Bug #430784: The launchpad branding in the base-layout footer should be linked <story-ui-3> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/430784>
<bac> noodles775: is there anyway you could include that in your current branch?
<bac> noodles775: btw, that bug says footer but i don't think it's correct
<noodles775> bac: Sure - but from the jml's email to lp-dev, he's already got an MP there for it?
<bac> noodles775: ah, i didn't see that yet
 * bac looks
<noodles775> ok.
<bac> noodles775: right you are
<bac> noodles775: nm, then
<bac> noodles775: in the ==Launchpad Edge== test you say "each page will" but one of the criteria you mentioned was not being locationless.  shouldn't that part of the narrative mention the locationless exclusion?
<bac> noodles775: gah, i see it is the next section
<noodles775> :) great.
<bac> noodles775: still i think s/each page will/most pages will and then later show why locationless is the exception
<noodles775> bac: yep, I agree.
<noodles775> (the current narrative reflects the fact that I only came across the locationless exception after writing it :/ ).
<bac> noodles775: the new changes look good.  thanks
<noodles775> bac: great! Ta.
<sinzui> barry: r=me
<bac> sinzui: noodles775 pointed out that jml already has an approved MP for bug 430784
<mup> Bug #430784: The launchpad branding in the base-layout footer should be linked <story-ui-3> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/430784>
<sinzui> rock
<bac> EdwinGrubbs: may i add to your growing queue?  it's a pretty straightforward fix to +review-licenses
* bac changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: adeuring || queue: [adeuring, adeuring, barry, adeuring, cprov, adeuring,bac]
<EdwinGrubbs> bac: sure, I have no idea how many of the items are tiny so I may or may not get to it.
<bac> EdwinGrubbs: np.
<barry> EdwinGrubbs: before you review my branch, check with sinzui
<EdwinGrubbs> barry: did you ask him to review it, also?
<barry> EdwinGrubbs: yes
<sinzui> barry: I reviewed your code of conduct branch
<barry> sinzui: thanks!
* barry changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: adeuring || queue: [adeuring, adeuring, adeuring, cprov, adeuring,bac]
<EdwinGrubbs> barry, sinzui: If I'm reviewing a template conversion, and the view class is used by several different urls, should I recommend that they create a subclass so they can implement appropriate page_title and label attributes, or should the developer just use the heading slot to speed things along?
<sinzui> EdwinGrubbs: That is what I have done in all cases. I believe other have created views from mixins
<barry> EdwinGrubbs: it would be better to refactor mega view classes, use a mixin or a subclass, probably in that order
<barry> EdwinGrubbs: it is better not to use the heading slot
<sinzui> EdwinGrubbs: The only reason to subclass is if two pages that make titles use it. It is fine for 200 portlets to share a view that make a single page
<EdwinGrubbs> sinzui, barry: cool, I just wasn't sure how urgent the rubberstamping was, but I like the cleaner solution.
<sinzui> EdwinGrubbs: subclassing is pretty trivial. I am sure you can always suggest that
<salgado> sinzui, the link to +related-software is no longer present on person-index.pt, so there's no way to get to that page.  is it ok if I put it together with the ones we have before the first yui portlets?
<sinzui> Oh. Please do
<EdwinGrubbs> salgado: to prepare for your page title fix, should the View.page_title="Foo Bar"     or     View.page_title="Foo Bar: %s" % self.context.displayname
<danilos-afk> EdwinGrubbs, I'd say just "Foo Bar" if context.displayname is in breadcrumbs before that
<danilos-afk> EdwinGrubbs, this should not override the entire page_title
<EdwinGrubbs> salgado: ^^^^
<salgado> EdwinGrubbs, ?
<EdwinGrubbs> salgado: sorry, I forgot that you probably don't see the relavent info, since you were disconnected.
<EdwinGrubbs> salgado: to prepare for your page title fix, should the View.page_title="Foo Bar"     or     View.page_title="Foo Bar: %s" % self.context.displayname
<salgado> EdwinGrubbs, whatever you'd do if not preparing for this fix.  there's really nothing we should be doing in preparation for it
<salgado> just like before, though, it's a good idea to move stuff from pagetitles.py into the view classes
<EdwinGrubbs> salgado: Currently, the page I'm reviewing shows "+documentation : Kubuntu". I would normally change the page_title to     "Current documentation: %s" % self.context.displayname     however, it sounded like that would yield "Current documentation : Kubuntu : Kubuntu"
<salgado> EdwinGrubbs, I'd say don't change the page_title there, as the rules for the title are not yet finished and may change really soon
<EdwinGrubbs> salgado: ok, so I should just live with "+documentation: Kubuntu".
<salgado> yes, that will change when my branch lands
<rockstar> Holy shit.  Is the queue really this long?
<rockstar> abentley, ping
<abentley> rockstar: pong
<rockstar> abentley, given the length of the current review queue, could you do a review for me?
<abentley> rockstar: quid pro quo, clarise
<rockstar> abentley, I'm assuming that's a reference to Silence of the Lambs, but it's meaning is unknown to me.
<abentley> rockstar: I will review yours if you review mine.
<rockstar> abentley, that's a deal.
<abentley> rockstar: Okay, could you review https://code.edge.launchpad.net/~abentley/launchpad/preview-on-create/+merge/11906 please?
<abentley> rockstar: what would you like me to review?
<rockstar> abentley, cool.  I'll send mine out and request you review it.  I have to upload these screenshots still.
<abentley> rockstar: Sure thing.
<EdwinGrubbs> bac: it doesn't look like you are testing that all six date fields can be passed in as date objects. It also seems like you could test all six in a single forReview() call and still have it match the bzr project.
<bac> EdwinGrubbs: i did not test all six, just a representative.  i reasoned this regresssion was unlikely to occur again
<bac> EdwinGrubbs: but i can add it if you feel it is useful and will be happy to do so
<EdwinGrubbs> bac: if this can be done without bloating the test with multiple forReview() calls, I think it would be a good idea, even though it is rediculously unlikely.
<bac> EdwinGrubbs: ok.  i'll just create a very early and very late date object and pass as the other fields.  working on it now.
<bac> EdwinGrubbs: your suggested fix plus the lint changes: http://people.canonical.com/~bac/bug-430748-team-mailinglist/
<bac> er, http://pastebin.ubuntu.com/272333/
<EdwinGrubbs> bac: r=me
<bac> EdwinGrubbs: cool, thanks
<bac> EdwinGrubbs: if you'll mark the MP i'll send it off to ec2 for landing: https://code.edge.launchpad.net/~bac/launchpad/bug-396585-review-license/+merge/11897
<EdwinGrubbs> bac: done
<bac> schweet
<EdwinGrubbs> sinzui: did you already review barry's branch?
<barry> EdwinGrubbs: yes.  it landed
<sinzui> EdwinGrubbs: Not the one he just added for UI
<EdwinGrubbs> cprov-afk: I'm starting to review your branch. Are you around?
<rockstar> abentley, having troubles with bzr send.  It's trying to bundle 358 revisions, which this branch definitely doesn't have.  Chasing now.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: cprov || queue: []
<abentley> rockstar: roger
<bac> EdwinGrubbs: can i add a single template conversion branch?  it's only 56 lines.  https://code.edge.launchpad.net/~bac/launchpad/bug-430748-team-mailinglist/+merge/11924
<EdwinGrubbs> bac: sure
* bac changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: cprov || queue: [bac]
<gary_poster> EdwinGrubbs: I have a branch of almost-entirely mechanical template changes.  May I add that to your queue?  https://code.edge.launchpad.net/~gary/launchpad/launchpad-templates/+merge/11930
<gary_poster> (launchpad 3.0 stuff)
<EdwinGrubbs> gary_poster: sure
<gary_poster> thank you
* gary_poster changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: cprov || queue: [bac, gary]
<rockstar> abentley, so we're creating a review diff AND a preview diff on creation now?
<abentley> rockstar: correct.  On phone.
<rockstar> abentley, okay.  Ignore my questions then until you have bandwidth to answer them.
<rockstar> Why don't we just get rid of the review diff altogether then, and just use the preview diff?  Generating two diffs might not be too big of a deal, I guess.
<rockstar> Or maybe that's the plan for another branch.
<barry> EdwinGrubbs: do you have room on your queue for a 514 line branch?
<EdwinGrubbs> barry: I doubt it, but I could always finish it up tomorrow if that's ok.
<barry> EdwinGrubbs: don't worry about it.  we have a lot of folks on ocr tomorrow.  i'll be up early gettin' on the queue
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: bac || queue: [gary]
<barry> sinzui: are you up for doing a review tonight?
<sinzui> barry: I can when I get back from a school meeting at 9:00
<rockstar> abentley, did you see my questions above?
<barry> sinzui: cool.  if it looks good i'll land it first thing in the morning: https://code.edge.launchpad.net/~barry/launchpad/430065-person/+merge/11935
<abentley> rockstar: Only the first one.
<bac> barry, sinzui: i can do that review now
<barry> bac: that would be great!
<abentley> rockstar: We were continuing to generate the review diff to be cautious.
<abentley> rockstar: We will disable review diff generation, but possibly not until after 3.0
<rockstar> abentley, okay.  r=me
<abentley> rockstar: thanks.
<bac>  EdwinGrubbs: i realize i forgot to remove the page title from pagetitles.py.  i've done so now.
<bac> barry: review down.  sinzui i see you claimed it too.  hope you don't mind...
<barry> bac thanks!  bac, sinzui i requested sinzui's review, but he doesn't have to do it now
<bac> thanks EdwinGrubbs
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: gary || queue: []
<bac> EdwinGrubbs: that zcml issue confuses me.  why does it currently work?
<EdwinGrubbs> bac: because of something either added to the __init__.py or to the importer when sinzui started migrating stuff, so that he wouldn't have to fix every last import by hand that the script wouldn't fix for him.
<bac> EdwinGrubbs: right
<bac> EdwinGrubbs: we should start turning those off to find the stragglers
<EdwinGrubbs> yep
<bac> but not tonight.  :)
#launchpad-reviews 2009-09-17
* sinzui changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: gary || queue: [sinzui]
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: [sinzui]
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: sinzui || queue: []
<jtv1> henninge_: again you've got a label property with a docstring saying that it returns a page.
<henninge_> oops
<henninge> aargh!!
<henninge> I just pushed 'q' in 'less' to loose the output from the test run ....
<henninge> :(
<henninge> but it was only one failed test (I think)
<jtv1> henninge: another thing... you've got the nav links inline in the template.  Since this is a set of common links, isn't it better to move them out to a template of their own?
<henninge> jtv1: how do you mean
<henninge> jtv: this is the way it's done on pofile-translate, too (which has landed) so I will most likely not change that now.
<jtv> henninge: I see.  Probably something we'll want to fix later though.
<henninge> jtv: oh, oh, now I get it ...
<henninge> ;-)
<henninge> You mean common between pofile-translate and tm-translate? Move them to a macro, basically.
<henninge> I'd still like to postpone that as a clean-up for later since I am redoing those pages anyway.
<jtv> henninge: Fine with me, as long as you're aware of it.
<henninge> jtv: https://bugs.edge.launchpad.net/rosetta/+bug/431249
<mup> Bug #431249: Put navigation links in a seperate template/macro. <Launchpad Translations:Triaged by henninge> <https://launchpad.net/bugs/431249>
<jtv> henninge: perfect, thanks
<henninge> jtv: official mp coming your way!
<jtv> henninge: you sent the MPs after me?  Dude, that's harsh.
<henninge> jtv: yup, I'd so anything to keep you on your toes ... ;-)
 * jtv has his toes nicely up on the couch, thank you
<henninge> ;-)
<henninge> jtv: oh, I forgot to take a stab at pagetitles.py. Funny I didn't get a complaint ...
<jtv> Easy to forget an optional removal.  :)  Nothing automatically complains when you don't do that, because plenty of pages still share views so that it becomes hard to produce the right title from there.
<henninge> pagetitles.py just got shorter ...
<jtv> +1
<jtv> henninge: r=jtv, kind but optional request for xx-pofile-details.txt to "print" its title instead of dumping it with quotes and all.  Otherwise, enjoy your birthday!
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775-afk || reviewing: sinzui || queue: []
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775-afk || reviewing: - || queue: []
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: []
<noodles775> If there's anyone able to do a ui review of the following, please let me know: https://code.edge.launchpad.net/~michael.nelson/launchpad/429353-site-message-to-footer/+merge/11813
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [noodles775]
<noodles775> A really simple code-review if anyone is available: https://code.launchpad.net/~michael.nelson/launchpad/429551-view-package-details-link/+merge/11961
<salgado> sinzui, http://paste.ubuntu.com/272777/ are the changes I had to do to my person-three-o branch to make sure the page still works when LP is in read-only mode
<sinzui> salgado: I expected that you would need to define master or slave. Just switching to Storm is the answer?
<salgado> sinzui, yes -- we just need to use the store that is currently in use.  when we use the SQLObject compatibility layer, it forces the use of the master store
<sinzui> oh. I must remember that
<sinzui> r=me
<salgado> sinzui, thanks!
<BjornT> noodles775: got time for a small and simple review?
<noodles775> BjornT: of course :)
<BjornT> noodles775: cool. the mp should appear in lp soon...
<noodles775> Great.
<BjornT> noodles775: https://code.edge.launchpad.net/~bjornt/launchpad/bug-430813/+merge/11967
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: BjornT || queue: [noodles775]
<noodles775> BjornT: is it expected that when I now run `make jscheck` that lots (perhaps all? it's still going) non-js testcases are run?
<BjornT> noodles775: no. did you grab the branch, or simply apply the patch?
<noodles775> BjornT: I merged the branch.
<BjornT> noodles775: you probably have an old version of bin/test. remove bin/buildout, run 'make', and then try again
<noodles775> BjornT: ah, ok. Doing so now.
<sinzui> barry: take a look at https://code.edge.launchpad.net/~sinzui/launchpad/milestone-design-oops/+merge/11946
<barry> noodles775: hiya!  i'm going to have a really simple one for you; 41 lines
* barry changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: BjornT || queue: [noodles775,barry]
<noodles775> barry: great! You wouldn't be able to look at a ui-review or two would you?
<barry> noodles775: i could do some ui reviews
<noodles775> barry: or if rockstar will be on OCR, I can wait for him instead...
<barry> sinzui: the difference there is the full-page-width sections
<barry> noodles775: cool, either way
<sinzui> yes...and the fact you cannot see there is an empty sidebar for the IProject
<barry> sinzui: very nice.  ui=me*
<sinzui> thanks barry
<noodles775> barry: yeah, I'll need two ui-reviews anyway, so if you could take a look at the 3 screenshots in my last comment at:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/429353-site-message-to-footer/+merge/11813
<noodles775> ... that would be wonderful.
<barry> noodles775: sure thing
<barry> noodles775: i almost completely love it :)
<noodles775> heh... but?
<barry> noodles775: question: if you disable redirects, would the green link chang e to "Enable edge redirects"?
<barry> noodles775: question: when you click on the minty green "Disable" link, do you get an overlay (confirmation window?) or does it just happen?
<barry> noodles775: on http://people.canonical.com/~michaeln/tmp/site-msg-footer-on-3.0-narrow.png is there any way to avoid the icky wrapping?
<barry> noodles775: EOT
<noodles775> barry: 1. no - currently that'll still be done via the lp.net root page - but would be good to do (at first I thought it wouldn't be useful to have it on edge, but perhaps it would.)
<noodles775> barry: 2. No overlay, still the ugly alert box that you see currently (I didn't change any of the actual workings as part of this - just moving the site_message and adding the link).
<noodles775> barry: 3. I couldn't find one - I created that image to show the worst-case scenario, any suggestions?
<barry> noodles775: 1. could you file a bug on that?  i think it would be really cool to be able to enable/disable easily from the footer (clearly out of scope for this branch)
<barry> noodles775: 2. i wonder then if the link should be minty green.  isn't that a meme for "inline edit or overlay window coming"?  or it could be my monitor doesn't see the green quite right
<noodles775> barry: it'd be easy to add - I'll check, if it's a 10min I'l just do it now. otherwise yes.
<barry> noodles775: awesome
<barry> noodles775: 3. if possible, in the worst case scenario, put the whole "Disable edge redirect" right aligned on the following line (i.e. visually on the same line as the search box)
<barry> noodles775: other than that, it looks fantastic
<noodles775> barry: 2. aiui the green is for any js action (ie. usually link that doesn't actually go somewhere)
<noodles775> barry: 3. I don't see how I can do that - i've used a yuigrid, but i've no way of knowing whether the text will wrap?
<barry> noodles775: you're right
<barry> noodles775: cool.  it's not a big deal
<noodles775> OK, thanks barry!
<barry> noodles775: ui=me* and thanks!
<barry> noodles775: https://code.launchpad.net/~barry/launchpad/431859-wikinames/+merge/11972
<noodles775> barry: yep, merging now...
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: barry || queue: [noodles775]
<barry> noodles775: btw, re: ~cprov/+archive/ppa.  i think you've done it exactly the right way.  currently IEditableContextTitle can only handle the watermark heading as you've seen.  i think you've found a good reason to fill the heading slot.  might be worth an email to the list and/or some text in the wiki
<noodles775> barry: great, thanks for that.
<barry> np
<noodles775> I'll update the wiki.
<barry> great, thaks
<barry> thanks even
<noodles775> barry: review sent.
<barry> noodles775: thanks
<noodles775> barry: feel free to say no, but I've one other branch that just adds an in-context link, ready with screenshot, at https://code.launchpad.net/~michael.nelson/launchpad/429551-view-package-details-link/+merge/11961
<noodles775> needing ui review too.
<barry> noodles775: np.  i need to go afk for a few minutes, but i'll look when i'm back
<noodles775> Thanks.
<barry> noodles775: ui=me*
<noodles775> Thanks barry. BTW: could you add that to those two MP's?
<barry> oops, right
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: []
<gary_poster> EdwinGrubbs: I have two views that share the same view class but have different templates.  I'm trying to figure out the right way to do the "label" attribute for these.  
<gary_poster> An obvious approach is to subclass the view class and make another one for the second view, only overriding the label.  
<gary_poster> I don't love that because I seem to have some got desire to not make another class just to change a label, but maybe that's silly.  Do you have any recommendations?
<gary_poster> s/got/gut/
<intellectronica> gary_poster: property with a conditional return? not very elegant, but less code than subclassing
<EdwinGrubbs> gary_poster: I know it seems silly, but it seems to be the price we pay to put it in the view instead of in the template.
<gary_poster> intellectronica: right, thought of that too, but then the conditional return is based on the request url which feels fragile (what if the url is changed in the zcml? then everything still works except the label no longer knows to switch)
<gary_poster> EdwinGrubbs: ok thanks
<intellectronica> gary_poster: don't you have something like a context you can examine? if you have to look at the request url then it indeed feels wrong
<gary_poster> intellectronica: context is the same for both (a token)--oh...hm, I could maybe look at the LoginTokenType constant, assuming I can get to that somehow.  Will investigate, thanks!
<gary_poster> yes, self.context.tokentype
<abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/comment-free-for-all/+merge/11987 ?
<gary_poster> EdwinGrubbs: I tried to remove the information in pagetitles.py as you requested.  I added the label attribute.  Now when I try to go to the page I see this: AssertionError: No .page_title or pagetitles.py found for /home/gary/launchpad/lp-branches/launchpad-templates/lib/canonical/launchpad/zcml/../templates/logintoken-resetpassword.pt
<gary_poster> EdwinGrubbs: does that mean that I need to add a page_title too?
<gary_poster> (presumably the same value)
<gary_poster> oh
<gary_poster> I see @property for page_title in the example
<EdwinGrubbs> gary_poster: Let's ask salgado. He may be in the process of working on a branch that eliminates the need to add a page_title attribute, but will use it if it's there.
<EdwinGrubbs> salgado-lunch: ^^^?
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: abentley || queue: []
<gary_poster> EdwinGrubbs: If you mean that breadcrumb branch, I just did a pre-review of it.  It still uses page_title.  I asked him on the channel he asked for a prereview how label and page_title should interact
<gary_poster> (but he's at lunch)
<rockstar> abentley, is isMergable used anywhere else?
<abentley> rockstar: Yes.  For example, it's used to prevent requesting further reviews.
<EdwinGrubbs> gary_poster: I thought page_title became optional, but maybe it alternates between setting the last breadcrumb and the entire title based on override_title_breadcrumbs.
<gary_poster> EdwinGrubbs: maybe so.  I'm not really sure what the story is.  I'll add a @property as shown in the example on the UI page for now.
<rockstar> abentley, ah, okay.
<rockstar> abentley, there were no tests that you had to change? Or have you not run the whole test suite yet?
<abentley> rockstar: Haven't run the whole suite yet.
<rockstar> abentley, okay.
<abentley> rockstar: Gotta meet a friend for lunch in a few minutes.  I can ping you when I get back.
<rockstar> abentley, I think I'm just about done.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: []
<salgado-lunch> EdwinGrubbs, gary_poster, my branch still expects to find a .page_title in views (or an entry in pagetitles.py).  we might be able/want to combine label and page_title in the future, but for now all pages still have to provide a title
<gary_poster> salgado: ok thank you
* salgado changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: [salgado]
<salgado> rockstar, can you take mine?  it's not trivial but I'll be around to answer any questions you might have
<rockstar> salgado, I can.  Link?
<salgado> rockstar, https://code.edge.launchpad.net/~salgado/launchpad/breadcrumbs-for-leafs/+merge/11985
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: salgado || queue: []
* cprov changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: salgado || queue: []
<deryck> rockstar or cprov -- I've got a template delete and mechanical change branch for review, if one of you can look.
* deryck changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: salgado || queue: [deryck]
<cprov> deryck: I can
<deryck> cprov, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/remove-bugs-templates-in-lp-431916/+merge/11992
* cprov changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: salgado, deryck || queue: []
<cprov> deryck: looks good, one question though, what did you have to consider when setting override_title_breadcrumbs ?
<cprov> deryck: why is it not ok to let the breadcrumb set the weirdo (reversed nav path) title ?
<rockstar> salgado, you have a few XXX comments without bugs referenced.
<deryck> cprov, the title was something like "+subscribe : Bugs in Foo : Foo", so if I waited for upcoming changes to the +foo part, I could make it something like "Subcribe : Bugs in Foo : Foo" and I just thought the label of "Subscribe to Bugs in Foo" was better for the page and the page title.
<kfogel> cprov: can I send you a quick-n-easy one (https://code.edge.launchpad.net/~kfogel/launchpad/add-ldu-externally-copyrighted-scripts/, just adds some scripts from lp-dev-utils).
<kfogel> ?
<cprov> kfogel: yes
<kfogel> cprov: done -- https://code.edge.launchpad.net/~kfogel/launchpad/add-ldu-externally-copyrighted-scripts/+merge/11995
<deryck> cprov, but I'm record not liking the reverse breadcrumbs for most bug pages like this. :)
<salgado> rockstar, yeah, I've agreed with gary_poster that the one on line 54 should be a regular comment, but I'm happy to file bugs for the others
<cprov> deryck: yes, that's what I thought ... if the default pagetitle was better we wouldn't have to override it. is anyone still working on it ?
<rockstar> salgado, okay.
<deryck> cprov, no.  We'll have to do it the way I've done it if we want page_title in the <title> for now and we'll revisit after we get 3.0 done.
<cprov> deryck: cool, fair enough, r=me. I will have to do the same in one of my branches.
<deryck> cprov, cool.  thanks!  (And I know, we have several bug pages we need to revisit now, but I don't think we have time.  So reversed breadcrumbs will abound.
<cprov> kfogel: I never had a chance to use the migration script, but you change looks good, thanks for adding it to the tree. 
* cprov changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: salgado, - || queue: []
<rockstar> salgado, I'm pretty happy with this branch.  If you file bugs for those XXX comments, please land it.
<rockstar> salgado, I've always wondered how the breadcrumbs worked.
<salgado> rockstar, there's actually just one XXX that will be left there -- the other will be removed
<salgado> rockstar, I'll file the bug, though.  thanks a lot for the review
<rockstar> salgado, ah yes, you're right.
<kfogel> cprov: thx
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: -, - || queue: []
 * rockstar goes to lunch
<bac> sinzui: you still on your call?
<sinzui> yep
<bac> sinzui: ok, i'll just add you to the MP
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-432026-person-edit/+merge/11999
<kfogel> cprov: in the queue now, thank you.
<kfogel> (I mean the PQM queue.)
<cprov> kfogel: cool, thank you.
<barry> rockstar, cprov either of you have time for a lazr-js related (though not specifically lazr-js) branch?  68 lines; all ui (demo, no test)?
<cprov> barry: ehe, the no-test part is interesting ... I can.
<barry> cprov: cool.  see what you think ;)
<barry> cprov: writing the mp now...
<gary_poster> EdwinGrubbs: did you notice the review reply?
<EdwinGrubbs> gary_poster: oh, no. Looking at it now.
<gary_poster> EdwinGrubbs: thank you
<EdwinGrubbs> gary_poster: r=me
<gary_poster> Thanks Edwin.
<barry> cprov: mp sent.  i'll paste the url when lp processes it
<cprov> barry: k
<barry> cprov: https://code.launchpad.net/~barry/launchpad/413793-inline/+merge/12002
<cprov> barry: I'm on it
<barry> cprov: thanks man
<cprov> barry: it looks good ... so, '9em' == 'magic value', huh ?
<barry> cprov: yep :)
<cprov> barry: I don't mind really, since it can be fixed in the callsite
<barry> cprov: that was a happy revelation :)
<salgado> cprov, can you review http://pastebin.ubuntu.com/273054/ for me?  it fixes https://bugs.edge.launchpad.net/launchpad-foundations/+bug/429194 by simply removing code
<mup> Bug #429194: Breadcrumbs shouldn't have icons in them <Launchpad Foundations:In Progress by salgado> <https://launchpad.net/bugs/429194>
 * barry was so glad not to have to diddle around in lazr-js
<salgado> cprov, actually, that's the wrong diff
<salgado> cprov, http://pastebin.ubuntu.com/273056/ is the correct one
<cprov> salgado: okay
<barry> cprov: rock.  thanks
<cprov> salgado: so, no icons in breadcrumb anymore ?
<cprov> barry: you are welcome.
<salgado> cprov, yep, no more
<cprov> salgado: okay, you did a pre-impl call , right ?
<salgado> cprov, no.  why should I?
<cprov> just for avoiding surprises so close to the 3.0 release ;)
<cprov> if you are sure not one is expecting it to work, it's okay 
<rockstar> cprov, I'm back from lunch, so you're free to shirk your reviewer responsibilities to me if you'd like.  :)
<salgado> cprov, well, the surprise are the icons showing up when nobody was expecting them.  and it was beuno who filed the bug and assigned it to me
<cprov> rockstar: cool, thanks
<cprov> salgado: oh, great! that's what I wanted to hear. r=me
<EdwinGrubbs> sinzui: the http://launchpad.net/rdf page seems completely worthless.
<sinzui> EdwinGrubbs:  that page is awe inspiring
<sinzui> The link is broken too
<sinzui> EdwinGrubbs: does anything link to it? Can we remove it?
<EdwinGrubbs> sinzui: I'll check
<salgado> thanks cprov 
<cprov> salgado: np
<cprov> sinzui: can you take a quick looks at https://code.edge.launchpad.net/~cprov/launchpad/bug-412715-registration-info-in-base-layout/+merge/11874, it's ready to land.
 * sinzui looks
<cprov> rockstar: actually, you have a very good timing, I have 2 branches for you.
<cprov> rockstar: https://code.edge.launchpad.net/~cprov/launchpad/bug-430336-builder-status/+merge/11980
<cprov> err, the other MP is arriving ... slowly 
<bac> sinzui: when you get a chance can you have a look at my MP?  i'm going to be out for a bit.
<sinzui> bac: I sent my reply. r=me + a few suggestions
<bac> sinzui: really?  i just looked...
 * bac looks again.
<sinzui> I really did just do it
<bac> ah so you did.  thanks!
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: cprov, - || queue: []
<cprov> rockstar: the other one is https://code.edge.launchpad.net/~cprov/launchpad/bug-422965-archive-permission-api/+merge/12006
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar, cprov || reviewing: cprov, - || queue: [cprov]
* cprov changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: cprov, - || queue: [cprov]
<cprov> rockstar: thank you.
<sinzui> cprov: the images I was show place the registration information above the line, on the same line as the application buttons
 * sinzui looks for image
<sinzui> cprov: This is the image beuno showed me: https://devpad.canonical.com/~beuno/LP_new_design_Bugs_v3.1.png
<sinzui> ^ Since we know the app buttons are a fixed size, we are pretty certain that they will not collide with the registration info
<cprov> sinzui: yes, that's why I remember too
<cprov> sinzui: but since the application bar doesn't always refer to the context I though it would be strange to put the context registering information there
<sinzui> cprov: You have a good argument
<sinzui> oh
<cprov> sinzui: do you think we should wait martin and discuss this properly ?
<sinzui> No. He can file a bug.
<cprov> right, moving the slot isn't hard.
<sinzui> cprov: I was going to say to remove the italics, but since I think your point is valid, I think the italics are helpful
<sinzui> I'm glad we talked. I think you should land this as is. We can fix this next week if Martin can solve the context question
<cprov> yes, it seemed to be necessary to distinguish it from the page label
<cprov> sinzui: cool, will land and talk to martin next week.
<gary_poster> EdwinGrubbs: if I have a template that does not currently have a class, I need to add one for the label, right?  There's no exception?
<gary_poster> a view class I mean
<cprov> sinzui: thanks for your comments, it's on ec2.
<EdwinGrubbs> gary_poster: I don't think there is any exception for the page_title, but you should be able to put the <h1> in the "heading" slot. Check with sinzui.
<cprov-afk> rockstar: I will be back later to address your comments.
<gary_poster> ok sinzui wdyt?  I was just proceeding to make a view class.  It will have a label and page_titlle and that's it
<sinzui> gary_poster: is this a root object?
<gary_poster> yeah, /+graohics
<gary_poster> graphics
<rockstar> cprov-afk, sounds good.
<sinzui> you can same time by defining a label and forcing it to be the title...it wont get bradcrumbs
 * sinzui looks to the secret property
<sinzui> gary_poster: you can use override_title_breadcrumbs=True on the view to make the label the authoritative title
<sinzui> gary_poster: I did not know that page existed until I scanned the uncompleted work yeterday
<gary_poster> sinzui: :-) is "override_title_breadcrumbs=True" any better than "label = page_title = 'my label and title'" for some reason?
<barry> sinzui: ping
<sinzui> gary_poster: In this case no. page_title has been demoted to mean the last part of the bread crumb. 
<sinzui> Hi barry
<barry> gary_poster: yes, only because we're not changing any of this until after 3.0 :)
<barry> gary_poster: just to have some stability.  after 3.0 we should refine the rules a bit
<barry> sinzui: hi
 * gary_poster is confused :-)
<sinzui> listen to baryy
<sinzui> barry
<gary_poster> ok
<sinzui> gary_poster: listen to barry. He knows what we are going to change
<gary_poster> thank you, all
<barry> sinzui: so, i can either work on bug 403606 or bug 429663 or some templates
<mup> Bug #403606: ExpatError errors should be handled to not generate the OOPSes <oops> <Launchpad Registry:Triaged by barry> <https://launchpad.net/bugs/403606>
<mup> Bug #429663: ~team/+invitation/team does not have breadcrumbs <Launchpad Registry:Triaged by barry> <https://launchpad.net/bugs/429663>
<barry> sinzui: its back to school night tonight, so i have about an hour before i have to disappear
<sinzui> barry: I think you have a better chance of completing the breadcrumb
<barry> sinzui: any blueprint mechanicals perhaps?
<sinzui> barry: yes, I do not know the wiki page at the moment
 * barry looks
<barry> sinzui: there are 19 unconverted blueprint pages.  why don't i take a few of those?
<sinzui> please do
<barry> coolio
<barry> sinzui: i'm guessing there are no bugs for those yet
<sinzui> no, flacoste left us to create the bugs. I have gotten a lot of bug mail form blueprints this week since I am subscribed
<barry> sinzui: no worries.  i'll file bugs as i go
<flacoste> barry: first thing you could do is finish up abel's branch
<barry> flacoste: oh?  what's that one?
<flacoste> barry: he's sick today and edwin made a review
<flacoste> asking for some fix-ups
<flacoste> he might not be in tomorrow to finish this up
<flacoste> so it's worth finishing and landing
<barry> flacoste: do you have a url handy?  otherwise i could search for it
<flacoste> barry: lp:~adeuring/launchpad/bug-430590-specificationgoal-setgoals.pt-3.0-layout
<flacoste> barry: lp:~adeuring/launchpad/bug-430558-specificationtarget-documentation.pt-3.0-layout
 * barry looks
<flacoste> barry: lp:~adeuring/launchpad/bug-430610-sprint-settopics.pt-3.0-layout
<flacoste> barry: lp:~adeuring/launchpad/bug-430663-sprint-register.pt-3.0.-layout
<flacoste> barry: and lp:~adeuring/launchpad/bug-430739-sprintspecification-decide.pt-3.0-layout
<barry> flacoste: ah.  blueprint branches
<flacoste> see the associated merge proposal
<barry> flacoste: k
<flacoste> thanks
<gary_poster> barry, sinzui: override_title_breadcrumbs=True *forces* use of the page_title.  False is the default.  I continue to be confused (and wantd to jfdi by setting label to page_title ad move on)
<gary_poster> am I missing something?
<barry> gary_poster: yes :)
<gary_poster> kewl
<barry> gary_poster: so, right *now* the rule is that page_title is the last component of the breadcrumb, label is the H1/H2, and to completely override the reverse-breadcrumb-in-<title> you need to set the override_title_breadcrumbs=True
<barry> gary_poster: what we've talked about (after 3.0!) is that label would become the last component in the breadcrumb, and setting page_title would do the override
<barry> gary_poster: but we did not want to change the rules yet again so close to the cut off
<gary_poster> I set it to False, and the page rendered.  I guess it inherited a True somewhere.  So is that OK?
<barry> gary_poster: it should default to False in LaunchpadView
<gary_poster> (override_title_breadcrumbs I mean)
<barry> gary_poster: is your view inheriting from LV?
<gary_poster> barry: sorry for taking so long, but I wasted some time today not being careful, so was trying to double check.  So first, I was wrong: False fails also.  It had worked because I had set page_title to label in a fit of pique.  When I removed that, it gave "AssertionError: No .page_title or pagetitles.py" just like with the original view class:
<gary_poster> class LaunchpadGraphics(LaunchpadView):
<gary_poster>     label = 'Overview of Launchpad graphics and icons'
<gary_poster>     override_title_breadcrumbs = True
<barry> gary_poster: ah, i didn't realize you were hitting the assertion
<gary_poster> So, the above class is what I have now, and what I thought I was supposed to use, and breaks.
<barry> (sorry, i probably didn't read the scrollback far enough)
<barry> gary_poster: you basically hit that assertion when you have no pagetitles.py entry (and you shouldn't :) and no +hierarchy adapter can be found for your view
<gary_poster> no, it was all theoretical when I asked--I was asking if I really needed a view class just to set a label (answer: yes)
<gary_poster> right
<barry> gary_poster: currently, yes
<gary_poster> I removed the value from pagetitles.py
<barry> gary_poster: great!  pagetitles.py should die
<gary_poster> barry: So, what should I do? :-)
<barry> gary_poster: let's skype.  it'll be quicker
<gary_poster> ok
<barry`> EdwinGrubbs: ping
<EdwinGrubbs> barry`: hi
<barry`> EdwinGrubbs: hi.  i've finished abel's bug 430558 branch.  would you like to review it?
<mup> Bug #430558: convert specificationtarget-documentation.pt to 3.0 layout <Launchpad Blueprints:In Progress by barry> <https://launchpad.net/bugs/430558>
<barry`> EdwinGrubbs: one caveat.  it's back to school night and i will be leaving in about 5 minutes.  when i get back i'm going to try to update his other branches, but i'll try to get friday's ocr to do the reviews of those
<EdwinGrubbs> barry`: sure, I'll review it.
<barry`> EdwinGrubbs: thanks.  mp is on its way
<barry`> EdwinGrubbs: thanks.  mp is on its way.  it should show up soon and it should be pretty straightforward.  i'll check back in a few hours
<gary_poster> EdwinGrubbs: I am flailing.  I've forgotten the secret place to find out the users and passwords to use in dev.  sample data has accounts but I don't see passwords and "test" only works for no-priv.  Where do I look for these values again?
<EdwinGrubbs> gary_poster: isn't it the accountpassword table?
<gary_poster> EdwinGrubbs: ah yes, thanks.
<EdwinGrubbs> barry`: did you remove the entry in pagetitles.py for that page?
<gary_poster> EdwinGrubbs: would you be willing to review my new template branch (three templates): https://code.edge.launchpad.net/~gary/launchpad/launchpad-templates-2/+merge/12012 ?
<EdwinGrubbs> gary_poster: sure
<gary_poster> thank you very much
* rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: []
<EdwinGrubbs> gary_poster: what is +storeblob useful for?
<wgrant> It's used by apport, Ubuntu's bug reporting tool.
<gary_poster> wgrant: thank you, I would have had no idea.  EdwinGrubbs ^^^
#launchpad-reviews 2009-09-18
<EdwinGrubbs> wgrant: thanks. I guess it can go away if bug 315358 is ever completed.
<mup> Bug #315358: Expose the storeblob service in API <api> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/315358>
<wgrant> EdwinGrubbs: Mmmmm, possibly.
<wgrant> EdwinGrubbs: But the API is awkward right now.
<wgrant> EdwinGrubbs: apport has broken once or twice in the past when field names changed on that page. I think there's a test for that now, but best to just check that manually..
<EdwinGrubbs> wgrant: we're just changing the template which defines the header. So it would be hard to imagine how it could break.
<wgrant> EdwinGrubbs: That sounds safe, yes.
* jml changed the topic of #launchpad-reviews to: on call: jml || reviewing: - || queue: []
<thumper> jml: you on call?
<jml> thumper, yes.
<thumper> jml: bug 407643
<mup> Bug #407643: CodeImportNewView confusing after form changes <Launchpad Bazaar Integration:In Progress by thumper> <https://launchpad.net/bugs/407643>
<thumper> jml: take a look at the attached image
<thumper> jml: the fix is all in the page template
<thumper> branch is pushed
<thumper> I'm awaiting the scan
<thumper> I'd be tempted to rs it myself
<thumper> it is very simple
<thumper> no test changes needed
<thumper> it just indents the fields
<jml> thumper, the screenshot looks fine to me.
<jml> thumper, I'd still like to have a poke at the code
<thumper> jml: it is closer to what it used to be
<thumper> jml: I've proposed for merging with you as reviewer
<jml> thumper, thansk.
<jml> mwhudson, I've replied to your review
* jml changed the topic of #launchpad-reviews to: on call: jml || reviewing: [sinzui] || queue: []
<mwhudson> jml: woo
<jml> sinzui, you too.
* jml changed the topic of #launchpad-reviews to: on call: jml || reviewing: [] || queue: []
<mwhudson> jml: replied
<jml> mwhudson, thanks.
* noodles775 changed the topic of #launchpad-reviews to: on call: jml || reviewing: [] || queue: [noodles]
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: [] || queue: [noodles]
<noodles775> I'm guessing jml has finished reviewing for the day :)
<jml> I has.
<allenap> noodles775: Fancy a incredibly trivial one-line review?
<allenap> https://code.edge.launchpad.net/~allenap/launchpad/rocketfuel-get-overwrite/+merge/12038
<noodles775> allenap: sure - I'd ask for a swap but mine's 500 lines :/
<allenap> noodles775: Heh :)
<allenap> noodles775: The diff is going to take a while, so here's one I prepared earlier: http://pastebin.ubuntu.com/273315/
<noodles775> Cool.
 * noodles775 reads the emails.
<noodles775> allenap: so your branch will result in *all* branches updated including the overwrite flag right? Are there any side-effects or dangers in doing this?
<allenap> noodles775: If someone has edited one of their sourcecode branches it will be clobbered.
<allenap> noodles775: But people - including me - shouldn't really be making a habit of that, because if I forget to revert I might get strange behaviour in other branches.
<noodles775> allenap: ah, so update_branches() is only ever called in update_sourcecode() - right.
<noodles775> Yep.
<allenap> noodles775: Yep, I think so.
<noodles775> Yep, it is (currently).
<noodles775> allenap: r=me sent.
<allenap> noodles775: Thanks!
<noodles775> np.
<wgrant> jml: Did you get around to the update-sourcecode fixes, or should I do it now I've time?
<noodles775> adeuring: are you available for reviews today?
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: [] || queue: [noodles, noodles]
<adeuring> noodles775: i'm afraid that i would produce quite some nonsense today: have th fl
<adeuring> thre flu
<noodles775> adeuring: np! Hope you get better soon!
<adeuring> noodles775: thanks!
<jml> wgrant, I didn't get around to it.
<wgrant> jml: OK, thanks. I'll do it tomorrow.
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: [] || queue: [noodles, noodles, noodles]
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [] || queue: [noodles, noodles, noodles]
<barry> looks like it might be noodles for breakfast?!
<noodles775> heh, barry, could you pls look at this one first:
<noodles775> https://code.launchpad.net/~michael.nelson/launchpad/432416-binary-pkg-index-oops/+merge/12051
<noodles775> it's the most urgent of my three (and related to one you ui-reviewed for me yesterday).
<barry> noodles775: sure thing
<noodles775> Thanks!
<noodles775> barry: I'm just going to take a break for 20 mins.
<barry> noodles775: no worries
<BjornT> barry: i have a branch that needs reviewing as well. it's non-urgent, but also really simple (all it does is to add a test)
* BjornT changed the topic of #launchpad-reviews to: on call: barry || reviewing: [] || queue: [noodles, noodles, noodles, BjornT]
<barry> BjornT: cool.  salgado should be joining me here today, but i think abel is still out
<salgado> barry, I don't think I'll be able to join you, sorry.  I've got 171 tests failing on my breadcrumbs branch
<barry> salgado: double :(
<barry> salgado: good luck!
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, noodles, BjornT]
<noodles775> I'm back barry if you have any questions etc.
<barry> k
<gmb> noodles775: Do you have the time to give me a UI review?
<noodles775> gmb: yep, add me as a ui-reviewer - I'll try to look at it in the next hour.
<gmb> noodles775: Awesome, thanks.
<noodles775> henninge: it's looking great! I've just merged so I can take a quick look at the issues you identified.
<henninge> noodles775: thanks. I am currently adding icons to the "What's Launchpad" list.
<abentley> barry: I can has review? https://code.edge.launchpad.net/~abentley/launchpad/restyle-subscriptions/+merge/12008
<noodles775> henninge: so you can left-align the stats simply by removing the text-align: center from #homepage-stats in style.css.
<henninge> noodles775: it is meant to left-align with the left edge of the search field.
<henninge> at least that's what the mock-up looks like but I am not sure Martin did that on purpose.
<noodles775> henninge: and it does doesn't it? (I'm juts disabling the rule and it seems to do exactly that?)
<noodles775> or did I misunderstand?
<henninge> I am trying now .. ;)
<henninge> wow, it (almost) works!
<henninge> still have to adjust the width.
<henninge> noodles775: I tried a few things this morning but this must be a combination I missed ... ;-)
<noodles775> henninge: aha. Also the 'Take the tour' is pointing to +newteam for some reason.
<henninge> oops, copy-and-paste ...
<barry> abentley: put it on the list!
<henninge> noodles775: wait, which one?
<abentley> barry: Okay.  I always ask first.  Should I not do that?
<henninge> there are two, one left, one right.
* abentley changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, noodles, BjornT, abentley]
<henninge> ah, got it, noodles775 
<barry> abentley: as the day wears on, it's gets better to ask :)
<barry> (or as the queue grows)
<EdwinGrubbs> barry: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/featured-projects-ui-3/+merge/12053
<barry> EdwinGrubbs: you can put it on the queue, but it's going to be a while.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, noodles, BjornT, abentley, Edwin]
<noodles775> henninge: and the un-clickable login/logout/user is because the #locationbar seems to have a calculated height of 0px... still trying to find out why. Explicitly setting it to 1em gets around it, but better to find the real cause.
<henninge> noodles775: oh, wgrant pointed me to a bug about it!
<henninge> noodles775: Bug #429247
<mup> Bug #429247: Locationless <h1>s block login/out widgets <story-ui-3> <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/429247>
<noodles775> henninge: great, I just updated it with that info.
<noodles775> Thanks!
<jtv> Who broke our front page?
<jtv> noodles775?
<noodles775> jtv: probably - if it's looking for isBetaUser then yes :/... 
 * noodles775 looks
<noodles775> jtv: huh? what's the problem that you're seeing?
<jtv> noodles775: something it uses obviously tries isBetaUser, yes
<noodles775> jtv: barry is reviewing a fix for that now... but I don't see it on the front page... ah, the translations front page.
<jtv> I guess that page got the application tabs through means other than the way your check expected
<jtv> Yes.
<noodles775> jtv: More likely your view does not inherit from LPView... checking now.
<jtv> It's more than just our front page, actually.  :/
<noodles775> jtv: it's any view that does not inherit from LPView...
<noodles775> jtv: I wrongly assumed that all non-locationless pages in LP inherit from LPView - but there are *heaps* of old views that don't. I should have checked... sorry.
<jtv> I figured.  And the pages apparently get their application tabs through means other than what the check expected
<noodles775> jtv: the fix is simple, hopefully I can land it soon.
<jtv> Maybe worth updating those views as well, sometime after release.
<jtv> Should I file bugs?
<noodles775> jtv: no, the fix is a general one...
<jtv> I meant about making views inherit from LaunchpadView.  But that too may be best done automatically and centrally.
<noodles775> jtv: oh, perhaps. But yes, as a general bug, I've got a list here: http://pastebin.ubuntu.com/273421/
<jtv> noodles775: cool...  if/when we really want to do something about it, I guess we could correlate that list of class names to stuff we find in zcml.
<rockstar> barry, if your queue still looks like this after I get these two branches landed, I'd be happy to help out.
<barry> rockstar: awesome, thanks
<barry> noodles775: first review sent
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, BjornT, abentley, Edwin]
<noodles775> Thanks barry.
<barry> noodles775: see what you think of my proposed alternative
<noodles775> barry: I'll give it a try... one tic.
<henninge> noodles775: I quite like setting locationbar to 1.5em...
<noodles775> henninge: great, it fixes the issue, so might be a good solution in and of itself.
<henninge> noodles775: it looks good on all pages, I always thought they had too little space at the top.
<henninge> 1em is enough, though.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, BjornT, abentley, Edwin, Edwin]
<intellectronica> barry: can i bother you for a quick ui review? martin has already given a thumbs up for a previous version, so this is just a last check before i can land
<intellectronica> barry: https://devpad.canonical.com/~tom/bugs-homepage-redesign-final.png
<noodles775> gmb: sorry, I'm not going to be able to get to that ui review before I run... you might be best adding barry and popping it on the queue (retrospectively) ;D
<gmb> noodles775: Righto, thanks anyway.
<gmb> barry: Is that okay with you ^^?
<henninge> noodles775: I pushed a new version.
<intellectronica> gmb: i can ui review
<gmb> intellectronica: Good point. I was going to swap with your for code review anyway.
<intellectronica> gmb: yes indeed :)
<henninge> noodles775: if you are fine with it, I'll leave it as it is now (except for upodating the blog posts).
<gmb> intellectronica: https://code.edge.launchpad.net/~gmb/launchpad/bugtask-index-conversion/+merge/12054, if you would. I still need to fix some tests before you review the code changes; I'll let you know when I'm done.
<barry> i'm happy to do quick ui reviews between code reviews if there are screenshots
<intellectronica> gmb: ditto (still doing tests before i can submit for code reviews)
<gmb> intellectronica: RIght.
<gmb> intellectronica: Oh, one point: I know I still need an icon for the Convert to question action; that's my next job.
<intellectronica> gmb: and ditto, if you have a screenshot i can ui review now quickly
<gmb> intellectronica: https://devpad.canonical.com/~gbinns/screenshot.png
<intellectronica> gmb: a smiley will do ;)
<barry> gmb: i can do a ui review.  paste the screenshot url
<gmb> Now now, no fighting please...
<barry> intellectronica: what am i looking for?  the design of the whole page or specific parts of it?
<intellectronica> barry: both. the general concept martin already reviewed, but i just want a last check that everything looks sane
<intellectronica> gmb: looks very nice. it will be _very_ cool if we could get the description/comments/postbox aligned. is it unrealistic given the time, though?
<barry> intellectronica: i love certain this about it.  the fact taht you can change the supervisor and contact right there (are they inline edits?) and the wonderful human friendly breadcrumbs
<intellectronica> gmb: the remove cve link should be on the next line
<gmb> intellectronica: I've played with it and so far have had no joy. Unless you can think of something off the top of your head that might work...
<gmb> intellectronica: Argh. Missed that. Thanks.
<barry> intellectronica: the only thing that looks weird to me is the tag cloud.  i vaguely feel like it should be left-aligned or justified, but if that's difficult it needn't hold up the branch
<intellectronica> barry: they're not inline, and you could change them from that page in 2.0 (but i guess if you noticed now it means that my plan to make the links more discoverable worked)
<barry> intellectronica: definitely
<intellectronica> barry: i like the idea of justifying! do you happen to know the css for doing that?
<intellectronica> gmb: b.t.w if you can't come up with an icon for converting to question i think you can use (+)
<gmb> intellectronica: Right, that was my fallback plan.
<barry> intellectronica: i think: style="text-align: justify;"
<gmb> intellectronica: I'm filing a lot of small tweaks under "fix it later"
<noodles775> barry: your idea works well and is much cleaner... doing an incremental now.
<barry> noodles775: rawk
<deryck> intellectronica, gmb -- for description to stretch out and line up to the side bar on a bug page requires a bug fix from me to the widget, which I can't get today.  But comments could line up to side bar like the table, I would think.
<barry> intellectronica: if that looks good, ui=me* (but i'm also happy to look at it again if you're not sure)
<intellectronica> gmb: the alignment of the main area, especially now that the cves are there, is pretty ugly. but i guess it's quite a lot of work to fix that now
<barry> gmb: did you have a ui you wanted me to look at?
<gmb> deryck: But that then goes against all the other pages' layouts (where commetns are quite compact)
<gmb> barry: An extra set of eyes is always helpful; feel free.
<deryck> gmb, ah, ok.  I didn't realize we had a rule/pattern for that.  If comments are fixed, I could make the widget adjust to the comments pretty easily.
<deryck> fixed is easy, fluid, not so much
<barry> gmb: did you have to fill the heading slot to get the editable h1?
<intellectronica> barry: that was such a good idea. refresh and see
<gmb> deryck: Right. So, under main_side, the boardComments isn't fixed width... I think it just has a big right margin.
<barry> intellectronica: beautiful, thanks!
<gmb> barry: Yes.
<intellectronica> barry: thank you
<deryck> gmb, hmmm, ok, so that may be hard then, too, if that comments are fluid.
<intellectronica> barry: b.t.w i'm going to interpret your ui* and martin's review from yesterday as ui-approved
<gmb> intellectronica, deryck, barry: I just have to step out for a minute (*must* post some documents today and I've forgotten)... I'll address any comments when I return.
<noodles775> jtv: mthaddon updated the configs temporarily on edge so that your pages won't oops.
<deryck> gmb, cool
<jtv> noodles775: acknowledged with thanks
<mrevell> henninge: ping
<henninge> mrevell: here
<mrevell> Hi henninge, I'm just looking at your mock-up.
<henninge> mrevell: its not a mock-up
<henninge> it's a screenshot of my page.
<henninge> s/page/branch/
<mrevell> sorry, yes
<henninge> mrevell: just saying, the design work (Martin) and the coding (Me) is mostly done, I am just wondering about content and wording now.
<henninge> mrevell: and I need the blogposts that will ve released next week ;-)
<henninge> be
<mrevell> henninge: It may be too late to change but I think this design gives far too much prominence to the "latest blog posts" and not enough to getting started. If I take a look at, say, GitHub's home page I fe
<mrevell> henninge: You need next week's blog posts? Surely it's taking a live feed, isn't it?
<henninge> mrevell: no, it's not ...
<henninge> mrevell: these are just copied from the current page which were hard-coded, too.
<barry> gmb: could you do me a favor?
<barry> gmb: file a bug on the fact that you had to override the heading slot to do that?  i'd like to fix that post-3.0
<henninge> mrevell: If I had a function I could call to get the blogposts, I could easily put them on the page
<mrevell> henninge: That's a damn shame: we're just giving "what's new" a fancy new, and inaccurate, name. I think it really must not be "Latest blog posts" as that will very quickly become inaccurate. We'll continue to make blog posts between releases.
<henninge> but coding the polling now seems work
<barry> gmb: other comments: the "link to CVE/Remove CVE links.  why are both visible?  and if they need to be, why aren't they on separate lines?
<henninge> mrevell: yes, I was wondering about that. So we stick with "What's new" for now until we get the polling coded.
<mrevell> henninge: If that's how it has to be, I guess so.
<barry> gmb: shouldn't "Convert to question" have an icon of some sort?  it stands out as odd without it
<mrevell> henninge: And I'll get that list of blog posts to you, in that case.
<henninge> mrevell: please
<barry> gmb: i wonder if the elements in the left-ish portlet under the bug summaries shouldn't be as wide as the bug summaries?
<barry> gmb: EOT
<barry> noodles775: which branch of yours am i reviewing next?
<mrevell> henninge: So, wrt to the design, I'm not sure what to suggest. I liked the initial mock-up but now I see actually coded up I think it lacks the punch we need for our home page. Perhaps the problem is that each section seems to have equal emphasis because the headlines and body text in each segment has the same font size. On the "not logged in" version I'd suggest that the "Launchpad is a..." section should probably have a subtle 
<mrevell> background and larger headline
* sinzui changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, BjornT, abentley, Edwin, Edwin, sinzui]
<henninge> mrevell: ok, kfogel suggested something like that, too.
 * barry will just pick one
<mrevell> henninge: For the logged in version, I feel like we should give more prominence to "Getting started" than to "What's new". perhaps swap them?
<henninge> mrevell: It would be easiest to swap the columns.
<henninge> not just one portlet.
<noodles775> barry: either is fine, if you want an easy one, the sprint templates (they're probably more urgent I'd guess).
<barry> noodles775: i'll take sprint-index-and-attend
<mrevell> henninge: I think that would work well.
<mrevell> henninge: henninge: And this is perhaps for another time but I like the fact that GitHub has a "Recently updated repositories" section on its home page. I wonder if we could do something similar.
<noodles775> barry: I have to run now, so feel free to prioritise the last one as you see fit :)
<barry> noodles775: gotcha
<noodles775> barry: btw, did you see the diff I just sent for the first one?
<henninge> mrevell: I am sure the code guys could give us that data.
 * noodles775 is about to run ec2test, and will do so to land if you're happy with it.
<henninge> mrevell: I mean, live, through some function call.
<mrevell> henninge: Well, I was thinking not so much just code but maybe something more universal. I'll add that as a comment on the bug, as it's probably not possible to do right now :)
<barry> noodles775: looks fine.  i'll update the mp.  my comment about those closing braces is that we're /not/ consistent so either way is probably fine for your branch ;)
<noodles775> barry: great, thanks!
<henninge> mrevell: oh, latest bug, branch, question, etc ... ?
<henninge> mrevell: yes, but not now. ;)
<mrevell> henninge: Yeah, too much work for right now :)
<mrevell> henninge: So, as for text...
<mrevell> henninge: minor quibble: I think "Read the user guide" works better than "Read the manual", but I'm easy if it doesn't fit.
<henninge> should
<mrevell> henninge: Going back to the design, I wonder if we should better emphasise the search box. Take a look at github.com and sourceforge.net and you'll see that the eye's immediately drawn there, whereas I find it easy to skip over ours.
<henninge> mrevell: yes, both have them graphically enhanced and placed top-left.
<henninge> mrevell: maybe the centering is a bad idea.
<barry> noodles775: ping
<mrevell> henninge: I don't mind the centering, but I think the graphical enhancement works. At the moment, our home page looks a bit like the poor cousin of our two main rivals.
<noodles775> barry: yep?
<barry> noodles775: this page doesn't look quite right: http://people.canonical.com/~michaeln/tmp/sprint-index-after.png
<barry> noodles775: you've got an h1 above the app tabs and no heading above the breadcrumbs
<mrevell> henninge: Our home page is quite text heavy, which is fine, but I think we need to throw in some more clues to show people where to look.
<noodles775> barry: yes, I meant to mention that in the MP, sorry...
<mrevell> henninge: I'm not sure, though, that we'll get all that fixed in time for 3.0
<noodles775> barry: afaics, the breadcrumbs should not be appearing according to the heading rules?
* bigjools changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [noodles, BjornT, abentley, Edwin, Edwin, sinzui, bigjools]
 * bigjools sees the queue and cries
<henninge> mrevell: we won't.
<barry> sigh
<noodles775> barry: not sure what you got, but:
<noodles775> <noodles775> barry: yes, I meant to mention that in the MP, sorry...
<henninge> mrevell: even adding a background takes some work to look good and not cheap.
<noodles775> <noodles775> barry: afaics, the breadcrumbs should not be appearing according to the heading rules?
<mrevell> henninge: Right
<henninge> i just tried and ...
<barry> noodles775: this should be a root context index view, so i think that's right.  there should be no breadcrumbs
<mrevell> henninge: I think we should probably spend some more time looking at the design of the page and maybe involve our friends in the User Experience and Design team at Canonical.
<henninge> reminded me of earlier years of the web 
<mrevell> henninge: but that's for another day, obviously
<henninge> mrevell: so the question is: go with this now or leave the old home page in place (minus old navigation).
<noodles775> barry: yep. Any other questions before I run out the door? :)
<barry> noodles775: that's it for the ui.  everything else looks great.  i'll mention this in my comments
<noodles775> barry: btw, I'm assuming that's a bug in the heading stuff, or is there something I can do about it?
<noodles775> barry: ok, great, thanks!
<mrevell> henninge: Hmm, big question! I like the direction of beuno's mock-up but actually seeing it in screenshots I prefer what we have now. I am not, though, in any position to make that decision alone haha
<barry> noodles775: that's an interesting question!  this hasn't come up before.  you might ping salgado about that (it's a +hierarchy adapter issue).  if he doesn't have an ideas off the top of his let, i'll try to think about it
<henninge> mrevell: well, who is?
<mrevell> henninge: At a guess, flacoste
<henninge> mrevell: he just saw it and said it was fine ... ;-)
<mrevell> henninge: In which case, I'll shut up
<mrevell> :)
<henninge> ;)
<mrevell> henninge: I think, though, that after the release we should revisit the home page. I'll post to the list with some idea of what I'm thinking about.
<henninge> mrevell: but please remember that blog post list. When do you think I can have it?
<mrevell> henninge: I'll send something over now.
<henninge> cool
<gmb> barry: So, in order: 1) Yes, I'll file that now; 2) They both have to be visible because you can both add and remove CVEs (if there's no CVE linked the 'Remove' link disappears. The separate lines thing is now fixed; 3) Yes, Convert to question needs an icon - that's my next job after fixing test failures; 4) Er... I don't think I understand what you mean by "left-ish portlet"
<barry> gmb: 1-3) cool; 4) "left-ish portal" means the bug description and everything below that.  see how there's a lot of vertical whitespace below the summaries and in between the description/comments and the right portal?  that's what i think should be collapsed
<gmb> barry: I agree; that looks ugly. I haven't (yet) found a way to fix it though. I'll keep trying, but in the interests of getting at least *something* landed before PQM closes, would you be okay with me filing a bug and fixing it later (assuming there isn't a cheap fix to be had now).
<intellectronica> gmb: guess what, i also just realised that i need to override the page title. how?
<barry> gmb: yes.  ask sinzui though.  he did something related (added some css to stretch things the full page width).  if he can't help come up with something simple, file the bug and land it
<barry> gmb: er, well, by land it i mean ui=me* :)
<sinzui> barry: gmb and I have talked. My tricks did not help with his real problems
<barry> sinzui: k.  gmb file that bug
<barry> gmb: is this bugtask-index-conversion?
<intellectronica> gmb: did you find out how to override the page title?
<intellectronica> barry: maybe you know? ^^^
<gmb> intellectronica: Yes
<gmb> intellectronica: Add a page_title property to your view
<henninge> mrevell: the column swapping is not as easy as I thouhgt it was so I am sorry but we'll have to postpone that, too...
<gmb> Remove the pagetitles entry
<mrevell> henninge: okay
<intellectronica> gmb: lovely, thanks
<sinzui> gmb: barry: I think the problem requires a very invasive redesign of the base-layout. If we can remove the heading-slot. Then we can make a simple slot that is for content before the sidebar.
<gmb> intellectronica: And then add     override_title_breadcrumbs = True to your view class.
<intellectronica> oh ok
<gmb> intellectronica: That last bit stumped me :)
<barry> sinzui: i would love to remove the heading slot.  but yeah, post-30 :)
<barry> intellectronica: i hope you don't need to override the whole <title> though
<gmb> barry: Thanks.
<barry> intellectronica: do you just need to make it human readable?
<intellectronica> barry: i'm not sure what you mean by human readable. it changed from what it used to be to 'something : something'
<barry> intellectronica: right.  human readable means, no +foo thingies
<intellectronica> the override_title_breadcrumbs = True trick worked, so i'm satisfied
<barry> intellectronica: isn't the reverse breadcrumbs what you want?
<intellectronica> barry: no, because the title is a bit more verbose than the breadcrumbs
<barry> intellectronica: hmm.  screenshot?
<abentley> rockstar: why aren't edit_whiteboard and edit_import @enabled_with_permission?
<intellectronica> barry: breadcrumbs is 'Bugs in firefox'. page title should be (and is now) 'Bugs in Mozilla Firefox'
<rockstar> abentley, ah, they probably should be as well.
<rockstar> abentley, edit_whiteboard doesn't need to be, since, if it's an import branch, anyone can write on the whiteboard (thus the problem with whiteboards)
<bac> barry: can i add a 35 line branch to your queue?
<abentley> rockstar: Do you really want to put lolspeak in lib/lp/code/browser/configure.zcml ?
<rockstar> abentley, where?
<barry> intellectronica: is that the full title or just the last component?  because salgado's branch is/will make it so you only need to specify page_title w/o the override.  but if the <title> is now what you want, then go for it.  we might need to re-address this later
<bac> sinzui: you have two minutes to do the review for bug 432163?
<mup> Bug #432163: Team page Join link thinks it is a download button <trivial> <ui> <Launchpad Registry:In Progress by bac> <https://launchpad.net/bugs/432163>
<sinzui> bac: yes
<barry> bac: you can add it, but we'll see ;)
<bac> sinzui: https://code.edge.launchpad.net/~bac/launchpad/bug-432163-join-link/+merge/12064
<abentley> rockstar: "<!-- NEW STUFFS GOES HERES -->" near line 369
<bac> barry: nm, sinzui is going to take it.
<rockstar> abentley, ah, yea, I need to remove that now.
<rockstar> abentley, and the LOLSPEAK was unintentional.  :)
<intellectronica> barry: yeah, the <title> is now what i want, so i won't worry about it until salgado lands his branch
<barry> bac, intellectronica: coolio
<sinzui> bac: r=me
<bac> sinzui: thanks
<barry> okay, i'm going back to code review now, so no ui reviews for a bit :)
<rockstar> Is PQM closing today or Monday?
<abentley> rockstar: It it current behaviour to hide merges for import branches?
<rockstar> abentley, no.
<abentley> rockstar: Were you aware that some projects are deliberately using merges on vcs-import branches to do code review of the upstream branches?
<henninge> mrevell: #homepage-whatslaunchpad ul 
<henninge> mrevell: I increased the font sizes on the headings and for the "What's Launchpad" section.
<rockstar> abentley, oh wait.  The behavior is that you can propose merges into vcs-imports, but you can't propose vcs-imports for merging.
<rockstar> Er, it's supposed to be.  I got a little overzealous.
<henninge> mrevell: http://people.canonical.com/~henninge/screenshots/launchpad-homepage-3.png
<abentley> rockstar: Okay, please fix.
<rockstar> abentley, okay.  Please note that on the review.
<bac> sinzui: i'm going to piggyback bug 432427 with the other.  it was a one line change
<mup> Bug #432427: Person's Code participation arrow is blueprint-coloured <trivial> <Launchpad Registry:Triaged by bac> <https://launchpad.net/bugs/432427>
<abentley> rockstar: What would you think about moving registrant below review team?  It seems like rarely-used information.
<rockstar> abentley, well, technically, it should go away.
<sinzui> bac: +1
<abentley> rockstar: Onto a "wow, you're nitpicky" page?
<rockstar> abentley, no, there was supposed to be a slot to put that info i.n
<abentley> rockstar: I don't understand.
<rockstar> abentley, well, "registered by" is SUPPOSED to go up in the right floating next to the title.  However, I don't think the slot exists.
<abentley> rockstar: I see.
<abentley> rockstar: Should the import URL of an import branch be displayed similarly to the mirror url of a mirror branch?
<rockstar> abentley, I thought about it, but I decided against it because import branches don't have just urls (like in the case of CVS).
<abentley> rockstar: Okay.  Maybe in the future we can display import data in the same place, whether it's a URL or a URL + branchname combo.
<rockstar> abentley, yeah.
<mrevell> henninge: Looks good. I'm just trying to come up with summaries for these blog posts for you
<henninge> mrevell: yeah, make'm good. They don't have to be that short and concise anymore, remember? ;-)
<abentley> rockstar: in branch-management.pt, why do you have this nested tal:condition at the top?
<abentley> rockstar: Isn't there a way to render a Link as HTML directly?
<rockstar> abentley, what do you mean?
<abentley> I mean not <a id="codebrowse-link" class="sprite info" tal:attributes="href link/url" tal:content="link/text" >Browse this branch</a>
<abentley> rockstar: Maybe <a tal:replace=link/fmt:link> or something?
<rockstar> abentley, ah, well, I would normally do that, but I need them rendered the way they are now when I attach javascript to them.
<abentley> rockstar: In order to customize the id?
<rockstar> abentley, basically, we don't have an API to give the link an id.  *groan*
<abentley> rockstar: This is a DRY violation, so we should fix it.
<abentley> rockstar: Not necessarily in this branch, though.
<barry> noodles775: i think i fixed sprint-index.  i'll include the diff in my review
<rockstar> abentley, yeah, the problem is known.
<abentley> rockstar: in branch-management.pt, why do you have this nested tal:condition at the top?
 * rockstar looks
<rockstar> abentley, I think that was a result of moving stuff around.  I'll remove the first condition.
<rockstar> abentley, please note that in the review.
<abentley> rockstar: Cool
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [] || queue: [noodles, BjornT, abentley, Edwin, Edwin, sinzui, bigjools]
 * barry -> lunch, then more reviews
<abentley> rockstar: r=me, subject to the changes we agreed on.
<rockstar> abentley, great.  Thanks a lot.  The next Sunday we're in Dunedin, dinner's on me.
<gmb> intellectronica: How are your test fixes looking? I've got a few more to go, but most of them are just little changes due to bits of the page being moved or restyled, so you can pretty much take a look whenever you like.
<intellectronica> gmb: exactly the same. how about we both create MPs and exchange reviews at 1800?
<gmb> intellectronica: Works for me.
<gmb> intellectronica: I'm going to go grab some food before I review your branch; ping me with the MP link and I'll take a look in half an hour or so. Mine's at https://code.edge.launchpad.net/~gmb/launchpad/bugtask-index-conversion/+merge/12075.
<intellectronica> gmb: i also have to go away for a bit, to eat and call my family (it's the evening of the jewish new year and they'll take me off the will if i don't call). on the positive side i am in the middle of fixing the last broken test, so i should have the complete fing for you when you come back
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles] || queue: [BjornT, abentley, Edwin, Edwin, sinzui, bigjools]
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [] || queue: [noodles, BjornT, abentley, Edwin, Edwin, sinzui, bigjools]
<barry> actually: noodles775, BjornT, abentley, EdwinGrubbs, sinzui, bigjools because there are so many reviews on the queue, i'll prioritize to those for folks who are around.  in that order, who's waiting on a review?
<abentley> barry: I'm around.
<barry> abentley: cool.  let's give a minute or so to noodles775 and BjornT and if they don't respond i'll start on yours
<BjornT> barry: i'm kind of around, but the review is non-urgent
* sinzui changed the topic of #launchpad-reviews to: on call: barry || reviewing: [] || queue: [noodles, BjornT, abentley, Edwin, Edwin, bigjools]
<barry> BjornT: thanks
<sinzui> barry: Mine is not important today
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [abentley] || queue: [noodles, BjornT, Edwin, Edwin, bigjools]
<barry> sinzui: cool
<abentley> BjornT: Thanks.
<barry> abentley: just to be sure, we're talking your restyle-subscriptions branch?
<abentley> barry: Yes.
<barry> cool
<EdwinGrubbs> barry: I'm around
<barry> EdwinGrubbs: okay, i'll do yours after i finish abentley's
<barry> abentley: are there mp's in sample data?
<abentley> barry: I don't think so.
<barry> abentley: ok
<barry> abentley: ui-wise i have one minor quibble, but i don't think it's introduced by your branch (it shows up in the branch subscribers too)
<abentley> barry: oh?
<barry> abentley: [mail-icon] to all changes: doesn't sit right for me
<barry> i think i'd like to see the word 'email' in addition to the mail-icon
<barry> abentley: but if you didn't introduce that, i won't push it
<barry> (and you might disagree anyway :)
<abentley> barry: Not introduced by me.
<barry> abentley: cool.  i'll mention it but leave it at that.  it's a beuno question anyway
<abentley> barry: I figured the icon was meant to represent "email subscribers"
<barry> abentley: other than that, you have two headings, so that'll need to be fixed
<barry> abentley: well, actually that's not in your change either
<abentley> barry: rockstar is working on a general overhaul of that page.
<barry> abentley: r=me, ui=me for your branch.  bonus points if you can get rid of that double header on branch merge proposal pages
<barry> abentley: fab
<barry> nm then! :)
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [edwin] || queue: [noodles, BjornT, Edwin, bigjools]
<barry> EdwinGrubbs: which one do you want me to take first?
<EdwinGrubbs> barry: it doesn't matter.
<barry> EdwinGrubbs: cool.  rdf-index-ui-3?
<EdwinGrubbs> sure
* salgado changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, Edwin, bigjools]
<barry> salgado: yay! you fixed the test failures?
<salgado> barry, yep, they were all mechanical, so it didn't take thaaat long
<barry> salgado: been there :)
<salgado> barry, I'll take noodles775's; that ok with you?
<barry> salgado: yep, thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, Edwin, bigjools, Edwin]
<barry> EdwinGrubbs: rdf branch r=me
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, bigjools, Edwin]
<EdwinGrubbs> barry: thanks, btw, I added another mp to the list
<barry> EdwinGrubbs: cool.  i'll do bug 430663 now
<mup> Bug #430663: convert  sprint-register.pt to 3.0 layout <Launchpad Blueprints:In Progress by adeuring> <https://launchpad.net/bugs/430663>
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, bigjools, Edwin, Edwin]
* henninge changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, bigjools, Edwin, Edwin, henninge (LP 3.0 home page!)]
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, bigjools, Edwin, henninge (LP 3.0 home page!)]
<barry> EdwinGrubbs: minor comments on bug 430663 branch.  should be easy to
<barry>         fix.  review sent
<mup> Bug #430663: convert  sprint-register.pt to 3.0 layout <Launchpad Blueprints:In Progress by edwin-grubbs> <https://launchpad.net/bugs/430663>
<henninge> barry, salgado: I appended my branch with the new LP home page. It's just shy of 800 lines, sorry ;-). I am currently running late for my own birthday party so I hope you are able to approve without asking many questions (in email). ;)
<henninge> I will be back during the (my) night to land it.
<henninge> barry, salgado: Please? (I think I forgot that.:)
<barry> henninge: happy birthday! 
<henninge> Thank you very very much!
<salgado> henninge, I'll try hard to get to it, but all branches today seem to be rather long.
<barry> henninge: rs=me if you blow out all the candles with one puff :)
<salgado> henninge, and happy birthday, btw! :)
<bigjools> barry: I iz here, belatedly
<henninge> barry:  actually, I almost did that yesterday (it was yesterday)
<barry> henninge: :)
<henninge> but they weren't 40 candles .. ;)
<barry> bigjools: cool.  we're getting to 'em
<bigjools> rockin
<barry> EdwinGrubbs: another review sent
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [noodles, BjornT, bigjools, henninge (LP 3.0 home page!)]
* salgado changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [BjornT, bigjools, henninge (LP 3.0 home page!)]
* salgado changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [edwin] || queue: [bigjools, henninge (LP 3.0 home page!)]
<salgado> bigjools, do you need a UI review of your trivial-ui branch?
<bac> sinzui: would you have time to look at https://code.edge.launchpad.net/~bac/launchpad/bug-422334-contact-team/+merge/12089
<bigjools> salgado: probably, it has some stuff other than rs= heading changes
<sinzui> ye
<bigjools> albeit trivial
<EdwinGrubbs> barry: I can't figure out why the SprintAttendanceRegisterView doesn't have an error when I remove the page_title, but FeaturedProjectsView needs it. Your review comments seemed to anticipate this.
<sinzui> bac: r=me
<bac> sinzui: thanks
<sinzui> bac I had forgotten there was a macro. That was nice clean work
<bac> thx
<barry> EdwinGrubbs: it's because FPV is a "top level" view that doesn't have a +hierarchy (i.e. breadcrumb) adapter, so it can't find a default <title>.  SARV has a +hierarchy adapter, thus breadcrumbs, thus default <title>
<EdwinGrubbs> aha
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [] || queue: [bigjools, henninge (LP 3.0 home page!)]
<barry> EdwinGrubbs: one more down
<salgado> barry, would you mind doing UI reviews for bigjools and henninge and I review the code?
<barry> salgado: it's a deal!
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [henninge[ui],-] || queue: [bigjools, henninge (LP 3.0 home page!)]
<bigjools> thanks barry
<deryck> barry, salgado -- I've got a branch for review.  Can I get in line? :)
<barry> deryck: yep, and i can do it after i do bigjools ui review. 
<salgado> deryck, sure!
* deryck changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [henninge[ui],-] || queue: [bigjools, henninge (LP 3.0 home page!), deryck]
<deryck> cool, thanks, guys.
<deryck> it's here when one of you is available -- https://code.edge.launchpad.net/~deryck/launchpad/milestone-portlet-links-385719/+merge/12092
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [henninge[ui],-] || queue: [bigjools, henninge (LP 3.0 home page!), deryck, Edwin]
<rockstar> abentley, ping
<abentley> rockstar: pong
<EdwinGrubbs> barry, salgado: I have another mp but I'll be afk for about 30 mins. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-430590-specificationgoal-setgoals.pt-3.0-layout/+merge/12095
<barry> k
<rockstar> abentley, can you do one more review for me today?  It's not very big, and very mechanical.
<barry> bigjools, salgado am i ui-reviewing trivial-ui branch?
<abentley> rockstar: okay
<bigjools> barry: please
<barry> bigjools: do you have screenshots by any chance?
<bigjools> just make sure I got the heading stuff right!
<bigjools> barry: I don't put I put URLs in the MP for you
<barry> bigjools: k
<bigjools> s/put I/but I/
<bigjools> it's late, I need some beer
<bigjools> barry: there's no major changes, just a few small things lumped into one branch
<barry> bigjools: cool
<barry> bigjools: the demo urls say dogfood, but i figured them out
<bigjools> oops
<barry> bigjools: one of the pages has busted headings, the rest look fine
<barry> bigjools: https://launchpad.dev/~cprov/+archive/ppa/+packages
<barry> bigjools: do you need a screen shot?
<bigjools> ah I didn't fix that one
<bigjools> I can check here, one mo
<barry> k
<bigjools> barry: ok what's bustifuct? :)
<barry> bigjools: the Packages in "PPA for Celso Providelo" heading shoudl be above the breadcrumbs, not below it
<bigjools> okay
<bigjools> easy
<barry> bigjools: should be, yeah.  i'll ui approve it with that caveat
<bigjools> barry: splendid, thanks
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [deryck,-] || queue: [bigjools, henninge (LP 3.0 home page!), deryck, Edwin]
* salgado changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [deryck,-] || queue: [bigjools, henninge (LP 3.0 home page!), deryck, Edwin, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<abentley> rockstar: Could you give me a ui review of https://code.edge.launchpad.net/~abentley/launchpad/restyle-subscriptions/+merge/12008 please?
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [deryck,-] || queue: [bigjools, henninge (LP 3.0 home page!), Edwin, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<bigjools> barry: I can't remember how to fix the ugly +blah breadcrumb ...
<rockstar> abentley, done.
<abentley> rockstar: Thanks.
<rockstar> abentley, got a review coming your way.
<barry> bigjools: i don't remember if salgado's branch has landed yet or not.  i think you set page_title to the last path component and do /not/ set title_override_breadcrumbs.  salgado can you confirm?
<barry> deryck: review sent
* barry changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [-,-] || queue: [bigjools, henninge (LP 3.0 home page!), Edwin, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<deryck> barry, cool, thanks
<bigjools> barry: yeah I have page_title implemented but no change to the crumbs.  I guess I'll wait
<salgado> barry, bigjools, there's no need to do anything to have that fixed.  it will be fixed once my branch lands.  it's on PQM now
<barry> salgado: fantastic
<barry> okay, i need a detox :)  back in a bit.
<bigjools> ossum
<deryck> barry, serieses was there before me ;)  I was being lazy. :)
<bigjools> oh and thanks for the code review salgado
<salgado> bigjools, np. do you have another branch for review or can I remove yours from the queue?
* salgado changed the topic of #launchpad-reviews to: on call: barry,salgado || reviewing: [-,henninge] || queue: [Edwin, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<bigjools> salgado: I only had one, it's not been taken out of the queue
<bigjools> now it has :)
<abentley> rockstar: it looks like the diff includes salgado's work.  Is that right?
<rockstar> abentley, yeah, because I merged in the middle of it.  I think salgado's work has probably landed in devel by now.
<salgado> it hasn't because there's a shipit branch playing on PQM now
<rockstar> abentley, I tried to go into the future where prerequisite branches are supported in Launchpad, but I got to the 3.0 release and decided it's a future I'd rather not see coming.  :)
<abentley> rockstar: It would be nice to diff against salgado's branch, then.
<rockstar> abentley, coming up.
<abentley> rockstar: Your diff also seems to include *my* work-- adding SELECT, INSERT on public.previewdiff
<rockstar> salgado, was your branch against db-devel?
<salgado> rockstar, yep
<rockstar> abentley, alright, I'm going to re-propose against db-devel.  *groan*
<rockstar> abentley, re-proposed against db-devel, attached a diff of just my work.
<abentley> rockstar: You mean, in addition to the bzr send diff?
<rockstar> abentley, no, I didn't send this time.  I just created it through the web ui.
<abentley> rockstar: What's with pageheading in lib/lp/answers/browser/questiontarget.py ?
<rockstar> abentley, what do you mean?
<rockstar> The original var was name pageheading.  I don't believe it's used anymore.  I'll rip it out and see what breaks.
<abentley> rockstar: I'm surprised it's not deleted.
<abentley> rockstar: You're frequently doing pageheading = page_title
<rockstar> abentley, yeah, like I said, I'll delete them, see what breaks.
<gary_poster> EdwinGrubbs: OK, if you are still willing, MP is up: https://code.edge.launchpad.net/~gary/launchpad/launchpad-templates-3/+merge/12101 .
<abentley> rockstar: The i18n Message class is why it says "FAQs for $displayname" ?
<rockstar> abentley, yeah, because the title is actually a Message, and apparently whatever is generating the title and breadcrumbs doesn't understand that.
<rockstar> So it's not making the substitutions correctly.
* salgado changed the topic of #launchpad-reviews to: on call: barry || reviewing: [-] || queue: [Edwin, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<abentley> What are the high-bit characters in lib/lp/answers/stories/question-answers-vhost.txt?  Culy quotes?
<salgado> barry, my head is hurting quite a bit, so I'll stop doing reviews.  I'll do them on Monday (or during the weekend if nobody beats me to them)
<abentley> rockstar: ^^
<rockstar> abentley, I'm not entirely sure, but the original test had the actual characters where I changed them to reprs where I could.
<barry> salgado: looks like there's just to more on the queue.  hope you feel better and have a good weekend
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [Edwin] || queue: [noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<abentley> rockstar: Could you please make the imports in lib/lp/registry/browser/person.py that you changed alphabetical?
<rockstar> abentley, sure.
<rockstar> abentley, also, it seems that there is no permission check currently on "Edit import or review source"  WTF?
<abentley> rockstar: Didn't I point that out in the last review?
<rockstar> abentley, no, I mean that the current code in devel has no checks either.  Like, no check to make sure you're vcs-imports or anything.
<rockstar> Odd.
<abentley> rockstar: Yeah, I certainly observed that in the last review.
<abentley> rockstar: So, do you think pageheading can die?
<rockstar> abentley, does that mean anyone can review imports now?
<rockstar> abentley, I think it can.
<kfogel> Anyone can review a quick and easy one?
<kfogel> barry or EdwinGrubbs: easy one: https://code.edge.launchpad.net/~kfogel/launchpad/cc-script-update-reference/+merge/12102
<abentley> rockstar: I don't think so.  I think it means that people who can't review imports are still given the link, and then it dies when they actually perform the review.
<barry> kfogel: if EdwinGrubbs can't do it, but it on the list.  i might have a few more in me today
<kfogel> barry, EdwinGrubbs: somewhat urgent, as the community-contributions cron update is broken without this (filing a bug on the larger issue now, but this change contains something that happens to work around the bug coincidentally)
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [Edwin] || queue: [kfogel, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
<rockstar> abentley, eep.
<barry> kfogel: i'll do it when i'm done with this one
<kfogel> barry: thank you
<abentley> rockstar: Anyhow, I was serious about adding enabled_with_permission, and when you've done that, it'll be good.
<rockstar> abentley, and edit_whiteboard does need launchpad.AnyPerson as well.  I can't believe we haven't gotten more spam through whiteboards.
<rockstar> abentley, that was an acknowledgement that I was making the change to edit_import as well.
<abentley> rockstar: okay, r=me with the pageheading removed and the imports resorted.
<rockstar> abentley, great, thanks so much!
<rockstar> abentley, shall we have our standup so you can have your weekend?
<abentley> rockstar: you bet
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [noodles[ui]] || queue: [kfogel, noodles(https://code.launchpad.net/~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access/+merge/12035)]
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [kfogel] || queue: []
<rockstar> barry, can I have a UI review for https://code.edge.launchpad.net/~rockstar/launchpad/branch-index-redesign/+merge/12061
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: barry || reviewing: [kfogel] || queue: [Edwin]
<rockstar> There's pictureses and everything.
<EdwinGrubbs> barry: I've added another branch to the queue if you have time.
<EdwinGrubbs> gary_poster: I'm starting on your review now.
<gary_poster> Thank you EdwinGrubbs 
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [kfogel] || queue: [Edwin,rockstar]
<rockstar> EdwinGrubbs, if you have time, you might be able to do the UI review as well for me, please?
<rockstar> ...or sinzui?
<EdwinGrubbs> rockstar: I can do it after gary's
<rockstar> EdwinGrubbs, cool, thanks.
<barry> EdwinGrubbs: don't do kfogel 's; i just approved it
<EdwinGrubbs> ok
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [Edwin] || queue: [Edwin,rockstar]
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [rockstar] || queue: [Edwin,rockstar]
<barry> actually, i'll do rockstar's ui review and then EdwinGrubbs's code review
<rockstar> barry, fank you.
<barry> rockstar: when you say "into unittests" another successful strategy we've taken in registry is to move lots of tests into view tests, which can still be doctests
<barry> rockstar: then actual pagetests are used just for workflow/navigation
<rockstar> barry, yeah, that's actually what I'm planning on doing.
<barry> rockstar: awesome
<rockstar> barry, and the vigor to do such a thing is still in me.  We'll see what happens this weekend.
<barry> :)
<barry> rockstar: the second screenshot is funky.  is there any way not to wrap that h1 heading with the branch url?
<rockstar> barry, not with that extra slot, although I expect that extra slot is in the wrong place.
<rockstar> (but I don't think that's an issue to be addressed in this branch)
<barry> rockstar: agreed. can you please file a bug on that?  i'd like to fix that post 3.0.  other than that, your two screenshots like good
<rockstar> barry, yay!  Thanks.  I'll file that bug before it lands.
<barry> rockstar: thanks!
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [edwin] || queue: []
<EdwinGrubbs> gary_poster: What url is HWDBFingerprintSetView used for?
* barry changed the topic of #launchpad-reviews to: on call: barry || reviewing: [] || queue: []
* barry changed the topic of #launchpad-reviews to: on call: - || reviewing: [] || queue: []
<barry> folks, that's it for me.  i can't eat any more no matter how wafer-thin
<gary_poster> I'm not sure--as I said, I was unable to test that.  It is selected in a navigation class.  I'll find it for you
<gary_poster> EdwinGrubbs: look in lib/canonical/launchpad/browser/hwdb.py .  Look at the HWDBApplicationNavigation class.  Look at the traverse_hwdb_fingerprint method
<gary_poster> EdwinGrubbs: That's kinda te extent of my knowledge. :-/
<gary_poster> the
<gary_poster> EdwinGrubbs: will be afk for dinner but will check back
<EdwinGrubbs> that's fine
<EdwinGrubbs> sinzui: if a page is just used as a macro, it doesn't need a page_title, right? I'm wondering why some exist in pagetitles.py to begin with.
<sinzui> EdwinGrubbs: I am sure you have found a mistake
<sinzui> EdwinGrubbs: The page must have a @@main_template/master by accident, which means it needs a pagetitle, but if it is never used as a page, that can be removed
<EdwinGrubbs> rockstar: I'm starting on the ui review of your branch-index-redesign branch.
<rockstar> EdwinGrubbs, great, thanks.
#launchpad-reviews 2009-09-19
<rockstar> Anyone want to take a really mechanical branch?
<rockstar> sinzui, maybe you're still around? ^^
<sinzui> yes
<rockstar> sinzui, alright, running tests now, will send it your way.
<sinzui> okay. Thanks
<rockstar> sinzui, proposal sent.
<sinzui> I will watch for it
<sinzui> rockstar: r=me Good to land as it is.
<wgrant> Any reviewers around for a fix for bug #432832?
<mup> Bug #432832: update-sourcecode broken for people without access to shipit and c-i-p <Launchpad Foundations:In Progress by wgrant> <https://launchpad.net/bugs/432832>
<rockstar> wgrant, hi
<rockstar> wgrant, where's you patch?
<wgrant> rockstar: Pushing. Give me a moment.
<wgrant> Slooow push.
<wgrant> ... and I of course stuffed up the MP.
<wgrant> rockstar: https://code.edge.launchpad.net/~wgrant/launchpad/bug-432832/+merge/12109
<rockstar> wgrant, you know you can just send the bundle, right?
<wgrant> rockstar: That relies on me having email set up properly.
<wgrant> I've never actually used bzr-send before.
<wgrant> I probably should work out how to do it.
<rockstar> wgrant, if you plan on submitting patches as regularly as you have been, I suggest you learn to.  :)
<rockstar> If there are holes in our documentation, feel free to bug me and I'll fix them.
<rockstar> wgrant, how have you tested this?
<wgrant> rockstar: lamalex installed LP with a very similar patch, and I ran rocketfuel-get with it on both existing and new installations. I haven't tested how it goes with shipit and c-i-p available, of course.
<rockstar> wgrant, how can I test it, since I have both available?
<wgrant> rockstar: Run 'utilities/update-sourcecode ../../lp-sourcedeps' to verify it works on an existing one. Then maybe uncommit a revision or two from a branch, remove another branch, run it again, and verify that they're all up to date.
<wgrant> You should not see any errors, since you have access to shipit and c-i-p.
<rockstar> I never knew about this script.
<wgrant> rockstar: rf-get, so by extension rf-setup, use it.
<wgrant> However it was just rewritten.
<rockstar> wgrant, ah, okay.  I eventually wrote my own script because my folder layout was different, and rf-setup/rf-get only supported one installation location.
<wgrant> rockstar: By overriding the LP_PROJECT_ROOT envvar I'm able to have it working on three different installations.
<rockstar> wgrant, yeah, it used to not support that.
<rockstar> Also, AIUI, it still doesn't support separate branches and trees.
<wgrant> Probably not.
<wgrant> I don't normally use it either, but that's because it was terribly slow.
<wgrant> Now it only takes a couple of minutes.
<wgrant> rockstar: Thanks. Can you land that for me?
<rockstar> wgrant, yeah.  I'll have to figure out how to do that.
<wgrant> utilities/ec2test.py --headless -s "[r=rockstar][ui=none] fix update-sourcecode in the case that optional branches are inaccessible" lp:~wgrant/launchpad/bug-432832 --email=me@williamgrant.id.au
<wgrant> Maybe throw a ' (wgrant)' just after the '[ui=none]'.
<wgrant> Some people do, some people don't.
<rockstar> Yea, I'm not sure I need to run that through ec2test.
<wgrant> True.
<rockstar> I don't see a direct way of doing it with bzr-pqm, so off to ec2 it went.
<wgrant> OK, thanks.
<wgrant> I'll leave the big rocketfuel-setup breakage (bug #432830) to somebody more qualified.
<mup> Bug #432830: rocketfuel-setup does not work <Launchpad Foundations:New> <https://launchpad.net/bugs/432830>
<rockstar> Man, I currently have your ec2 runs going.  My wife is going to think I'm buying crap on Amazon again.
<rockstar> s/your/four/
<wgrant> Heh.
<rockstar> wgrant, I killed the ec2 instance and figured out how to just pqm submit it.
<wgrant> rockstar: Great, thanks!
<rockstar> wgrant, thanks for your patch.
<bac> henninge: could i trouble you for a trivial review?  it's about a four line change.
<bac> if not i'll rs it
<henninge> bac: sure, bring it on
<gary_poster> henninge, btw happy late bday :-)
<henninge> gary_poster: thank you ;-)
<bac> henninge: MP sent.  waiting for it to show up
<bac> hi gary_poster.  do you know about http://www.sparkcon.com/ ?  it sounds fun but i've never been -- all the way in raleigh...
<gary_poster> bac: lol, no, looks cool!
<bac> henninge: https://code.edge.launchpad.net/~bac/launchpad/import_error/+merge/12115
 * henninge looks
<bac> gary_poster: some friends of mine started it a few years ago.  it's really grown huge.
<gary_poster> bac: wow, cool!  I wish there were a better "keep track of the actually cool events in the triangle" feed.  The ones I've seen don't include the stuff I want to see. ;-) until the most exciting thing I knew about this weekend was a greek festival. ;-)
<gary_poster> until now I mean
<bac> gary_poster: yeah, i'm thinking i may have to break my 'no raleigh' rule today and wander down there
<henninge> bac: are you keeping the commented-out import for documentation purposes? Is that common practice so that people know what it means?
<bac> henninge: no, it was an oversight.  i'll remove it
<henninge> bac: ok, r=me then ;-)
<bac> henninge: danke
<henninge> bitte!
<bac> hi abentley
<abentley> bac: Happily, this is only a problem with the test, and yes, I caused it.
<bac> abentley: great.  if you can get a fix in rs=bac it would be great!
<abentley> bac: Will do.
#launchpad-reviews 2009-09-20
<gary_poster> bac or anyone else around for an r or rs to disable the spurious test failure?
<gary_poster> flacoste, jml, mwhudson ^^^ ?
<gary_poster> jfdi-ing it...
<jml> gary_poster, I can do a post-hoc review
<gary_poster> jml thank you.  
<gary_poster> jml: http://pastebin.ubuntu.com/274444/ and https://bugs.edge.launchpad.net/launchpad-registry/+bug/433323
<mup> Bug #433323: test_proponent_is_stored causes a spurious test failure <spurious-test-failure> <Launchpad Registry:New> <https://launchpad.net/bugs/433323>
<jml> gary_poster, changing the test name to disabled_test_foo is more common, I think.
<jml> but no big deal
<gary_poster> jml: oh, ok.  I can change it--you caught me just as I was signing my gpg key :-)
<jml> gary_poster, that'd be good, thanks.
<jml> makes for easier greppage
<jml> gary_poster, also, thanks for demonstrating that it is spurious (rather than actually broken code) on the bug report.
<gary_poster> cool.  (bah, I don't have privs to see https://launchpad.canonical.com/PolicyandProcess/XXXPolicy which is the current link from https://wiki.canonical.com/Launchpad/PolicyandProcess)
<jml> also, man, that test does way too much
<jml> and shouldn't be doing cleanup after assertions that way
<jml> but not relevant to this patch.
<jml> gary_poster, there's a public XXX policy on the real wiki :)
<jml> gary_poster, https://dev.launchpad.net/XXXPolicy
<jml> gary_poster, See also https://dev.launchpad.net/SpuriousFailures
<gary_poster> jml: a thing of beauty, thanks.  I tend to go to https://wiki.canonical.com/Launchpad/PolicyandProcess still.  I updated with those.  Would be nice if there were an dev.launchpad.net equivalent--or is there?  /me looks
<gary_poster> nope
<gary_poster> amusingly, https://dev.launchpad.net/PolicyAndProcess/ReleaseManagerRotation , but not the parent
<jml> gary_poster, there's https://dev.launchpad.net/StyleGuides
<gary_poster> hm, but spurious test failure policy does not fit easily under the title of "style guide"...
<gary_poster> jml: new diff (entire, not incremental) is http://pastebin.ubuntu.com/274445/ .  r=you?
<gary_poster> putting an rs=barry
<gary_poster> thank you jml!  talk to you later
<jml> gary_poster, np.
<jml> gary_poster, thanks for patching it.
<bac> gary_poster: thanks
<wgrant> Anybody have time to review a tiny tiny fix? https://code.edge.launchpad.net/~wgrant/launchpad/bug-433385/+merge/12134
<gmb> wgrant: I'll take a look in a few minutes.
<wgrant> gmb: Thanks muchly.
<gmb> wgrant: Urgh... removeSecurityProxy(). I'm not happy about that, but the alternative is to write an "inEmailAddress(str)" method for IEmailAddress and that seems like an awful lot of work for this little change.
<gmb> wgrant: r=me, though... it's pointless going round the houses on this one.
<gmb> wgrant: Want me to ec2 and land that for you? Should make it in before PQM closes for 3.0 unless a test breaks.
<wgrant> gmb: rSP is already used in that view for a similar purpose, so I thought it should be OK.
<wgrant> gmb: Thanks. Please do.
<gmb> wgrant: Ah, right. Fair enough then. I didn't look too hard at the rest of the view :)
<gmb> wgrant: I'll fire it off now.
<gmb> wgrant: Running. I'll let you know if it breaks.
<wgrant> gmb: Thanks.
#launchpad-reviews 2010-09-20
<mwhudson> lifeless: reviewed
<lifeless> mwhudson: thanks
<lifeless> mwhudson: feedback on my feedback?
<mwhudson> lifeless: ok
<lifeless> mwhudson: there was a question there :P
<lifeless> mwhudson: for inlining the matcher
<mwhudson> lifeless: typing it up now, sorry :)
<lifeless> ah de nada
<lifeless> I thought you were saying 'ok do it go ahead'
<mwhudson> it was just meant to be a generic acknowledgement, though it was unclear
<mwhudson> anwyay, typed up now
<lifeless> thanks
<mwhudson> a review for https://code.edge.launchpad.net/~mwhudson/launchpad/move-some-code-vocabularies/+merge/35974 woudl be nice
<lifeless> done
<lifeless> mwhudson: ^
<mwhudson> lifeless: thanks!
<jtv> Anyone up for a really simple review?  mwhudson maybe?
<jtv> https://code.launchpad.net/~jtv/launchpad/bug-643240/+merge/35984
<lifeless> done
<jtv> thanks!
<jtv> That was fast
<lifeless> de nada
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bryceharrington || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bryceharrington || queue: [nooles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> henninge, Good afternoon. Can I add a branch to your queue?
<noodles775> Hi henninge, when you've time, could you review an incremental for an approved MP? I've linked to a diff from the last comment at: https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
<henninge> gmb: sure, please do
<henninge> noodles775: I'd happy to do so but who is that 'nooles' guy?
<henninge> ;-)
<noodles775> woops :)
* gmb changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bryceharrington || queue: [noodles775, gmb]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> noodles775, Fixed that for you :)
<noodles775> Ta.
<henninge> sleeping people will have to wait ... ;)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: noodles775 || queue: [noodles775, gmb, bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: noodles775 || queue: [gmb, bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> noodles775: Why is the marker interface not defined in interfaces?
<noodles775> henninge: I'd put it there in the browser module because its only used for displaying comments, but right, that's probably an old habit.
<noodles775> henninge: actually, hangon...
<noodles775> henninge: because that's where the code guys had them - I didn't create ICodeReviewNewRevisions (also only used for display).
<noodles775> I assume they had them there for that reason (see the comment for ICodeReviewNewRevisions in the diff)
<henninge> noodles775: I see.
<henninge> noodles775: I don't pretend to fully understand all effects of your change but what makes the attibutes move from the view to view.comment?
<noodles775> henninge: that's not part of that diff - which is why it would be hard to understand. That was why a test failed :)
<henninge> noodles775: ah!
<noodles775> I'll just find the relevant lines of the MP diff...
<noodles775> So from line 72ff on the MP diff, a bunch of attributes are moved from the view to the CodeReviewDisplayComment object.
<noodles775> which caused the test failure because the templates were using the view :)
<noodles775> henninge: sorry, I need to run out the door in a few mins for lunch... are there any other thoughts/questions that you've got?
<henninge> noodles775: thanks, I understand
<henninge> noodles775: no, I'll approve now. I didn't find anything ... ;) r=me
<noodles775> Thanks henninge
 * henninge runs to lunch, too
<henninge> np
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [gmb, bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [gmb, bryceharrington, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [gmb, bryceharrington, sinzui, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> sinzui and salgado.  Hi!  bdmurray wasn't sure how to proceed, so I commented on his +subscriptions work that you guys reviewed.  If either of you could look and see if you agree with me, I'd appreciate it.
<deryck> Just to help move him forward again.
<sinzui> I was waiting for salgado to respond. I think your proposed path is in line with my suggestions
<sinzui> I really hate the so-called story test though
<salgado> deryck, sinzui, just saw it here, will reply
<deryck> yeah, I agree.  I think it's premature at this point.
<deryck> Maybe he should just do view unit tests until we have a complete user story.
<deryck> sinzui, what do you think of that? ^^
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: gmb || queue: [bryceharrington, sinzui, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, - || queue: [bryceharrington, sinzui, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> bryceh, you should also request a DB review if you introduce a db patch.
<abentley> bryceh, around?
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, sinzui || queue: [bryceharrington, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, sinzui || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> henninge, abentley: a trivial one:  https://code.edge.launchpad.net/~bac/launchpad/bug-637102/+merge/36028
* bac changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, sinzui || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> sinzui, would it make sense to extract your monkeypatch of XMLRPCRunner.get_mailing_list_api_proxy to a Context Manager?
<sinzui> abentley, I do not understand your suggestion, but I welcome a graceful way to choose the proxy.
<sinzui> ah, mailman is not running with zopisms so my favorite hack or registering something in the testrunners config will not work
<sinzui> abentley, If you can help me factor out the monkey patch, I will be delighted
<abentley> sinzui, am writing up example.
<sinzui> thanks!
<abentley> sinzui, something like this: http://pastebin.ubuntu.com/497078/
<abentley> sinzui, it would be possible to generalize this to other mokeypatches.
<sinzui> abentley, I love this
<abentley> sinzui, great.
<abentley> s/addContext/useContext
<sinzui> notes
<sinzui> noted
<sinzui> I am do excited to have solved the mailman monkeypatch test strategy. No more anxiety when I hear mailman is broken
<abentley> sinzui, for paranoia, it could make sense to check that self.mailing_list.getReviewableMessages().count() == 0 before you make any changes.
<sinzui> abentley, I can add that. I have a lot of confidence since the team is new in each case, and I am only testing one message each time, that if that fails, then teardown failed
<abentley> sinzui, okay.
<abentley> sinzui, r=me
<sinzui> thanks. I will integrate the changes shortly
* henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: sinzui || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, - || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> gmb: approved. Sorry, I should have looked at it sooner as it was really niece and smooth reviewing ... ;-)
* henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [bryceharrington,bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> henninge, No worries, and thanks.
<abentley> bac, r=me
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: gmb, - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> rockstar, Did you have chance to look at my lazr-js branch yet?
<bac> thanks abentley-lunch
<gmb> (Realising of course that Monday has just started for you... :))
<bryceh> abentley-lunch, yep?
<leonardr> sinzui: i landed the branch you reviewed for me a few days back, and then immediately discovered that the requirements have changed so as to render the branch moot. can you help me a) review a branch that reverts the change i just made, and b) help me construct that branch in the first place?
<sinzui> :(
<sinzui> leonardr, do you want bzr command to do a reverse merge?
<leonardr> sinzui: exactly
<leonardr> i just tried bzr merge -r 11577..11578 and it ran, but i haven't checked if it's right yet
<leonardr> no, that didn't work
<sinzui> you will need --merge3, but let me look locally
 * sinzui pulls tip
<leonardr> sinzui, my revision is 11578
<leonardr> in case that wasn't clear
<sinzui> bzr cdiff -r 11578..11577 | less -R
<leonardr> yes, that looks like it will undo my change
<sinzui> then...
<sinzui> bzr merge --merge3 -r 11578..11577
<abentley-lunch> leonardr, you need to specify the branch, unless you want 11578/11577 to be interpreted relative to the branch you normally merge from.
<abentley-lunch> leonardr, so "bzr merge --merge3 -r 11578..11577 ."
<abentley-lunch> leonardr, though --merge3 is the default, so you only need it if you've overridden the default.
<leonardr> abentley-lunch: i think i've got it...
<abentley> bryceh, since this branch is proposed to land in db-devel, it's unlikely that the db patches would be shown if they were already in db-devel.
<abentley> bryceh, perhaps they hadn't landed at the time you proposed the merge?
<rockstar> gmb, I did look at it enough to realize it needs more of my attention than I could give to it at the time.
<rockstar> gmb, I'll have it finished when you get up tomorrow, is that good?
<abentley> bryceh, I think the new style is to define permissions in terms of interfaces.  Did you consider that option?
<leonardr> sinzui: https://code.edge.launchpad.net/~leonardr/launchpad/revert-oauth-aware-website/+merge/36038
<bdmurray> abentley: Could I get https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-bug-activity/+merge/35897 added to the queue?
<abentley> bdmurray, sure.
<abentley> bryceh, OCR is meant to be interactive.  Please ping me when you are up for an interactive review.
* abentley changed the topic of #launchpad-reviews to:  On call: abentley || Reviewing: bdmurray || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> bdmurray, what is a BugField?
<bdmurray> abentley: double checking
<bdmurray> abentley: its a field that allows one to enter a bug number so maybe its unnecessary
<abentley> bdmurray, I would expect IBugActivity.bug to be a reference to the bug, but it sounds like it's actually an int?
<bdmurray> abentley: yes, an int, the bug number, is correct
<abentley> bdmurray, ISTM that specifying it as an Int will be clearer, and since users don't edit BugActivity, we won't lose any functionality.
<bdmurray> abentley: right, that makes sense to me
<abentley> bdmurray, classes should have blank lines between their entries, e.g. export_as_webservice_entry(), datechanged, etc.
<abentley> bdmurray, the description of message is copy-pasted.
<bdmurray> abentley: could you elaborate on both of those points?
<abentley> bdmurray, I am generalizing from the PEP8 requirement "Method definitions inside a class are separated by a single blank line."
<bdmurray> abentley: I meant I don't see where the error is
<abentley> bdmurray, lines 33, 78, 80, 84, 88, 92, 96, 100
<bdmurray> abentley: hmm, there are not blank lines in IBug from interfaces/bug.py nor in IBugTask from interfaces/bugtask.py
<abentley> bdmurray, that may be so.
<sinzui> leonardr, I think the MP is for the wrong branch. I see your work and the work of others. Maybe you meant to merge into devel?
<leonardr> sinzui, thanks, i was trying to figure out what was the problem
<sinzui> just delete the mp and be sure to choose /devel (launchpad's default is db-devel)
<leonardr> sinzui: try https://code.edge.launchpad.net/~leonardr/launchpad/revert-oauth-aware-website/+merge/36040
 * sinzui waits for diff
<abentley> bdmurray, it's possible that there's some skew in team styles here.  It's also possible that those files are not good examples, because they were created a long time ago.
<bdmurray> abentley: okay
<sinzui> leonardr, r=me
<leonardr> sinzui: think i should ec2 test or is that overkill?
<sinzui> leonardr, I would ec2 to cover the small case where someone made a change on top of your changes
<leonardr> sinzui, ok
<abentley> bdmurray, I can't see anything definitive in our style guide.  https://dev.launchpad.net/PythonStyleGuide  The 10-line example under Properties hints at it.
<abentley> bdmurray, for the copy/paste, "message" is described as "The related property of the bug that changed." which is not accurate.
<bdmurray> abentley: do you have an idea for a more accurate description?  I couldn't find anything describing the field.
<abentley> bdmurray, sorry, I don't know what the field is used for.
<abentley> I think the test at line 154 is not useful.  It would not catch a case where bug activity were exposed as /bug_activity, for example.
<bdmurray> abentley: deryck and I've discussed the description some and have something better
<abentley> bdmurray, great.
<abentley> bdmurray, ping me when you've pushed your changes.
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [bdmurray]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> abentley, here's a really little one.
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/not-logged-in-recipes/+merge/36049
<abentley> rockstar, don't introduce shout_at_engineer.  Use removeSecurityProxy if the usage is legit, or an XXX if there's a fix needed.
<rockstar> abentley, ah, hadn't seen that.
<abentley> rockstar, you can also use getViewBrowser instead of canonical_url/logout/setupBrowser.
<rockstar> abentley, no, I can't.  getViewBrowser logs me in.
<abentley> rockstar, okay, let's fix getViewBrowser, then.
<gmb> rockstar, That works for me, thanks.
<rockstar> abentley, getViewBrowser's login stuff goes pretty deep...
<abentley> rockstar, can't we just add a no_login parameter and call setupBrowser instead of getUserBrowser?
<rockstar> abentley, yup, that's what I'm doing.
<abentley> rockstar, cool.
<rockstar> abentley, updated (and removed the other instance of setupBrowser in another test) https://code.edge.launchpad.net/~rockstar/launchpad/not-logged-in-recipes/+merge/36049
<abentley> rockstar, r=me.
<rockstar> abentley, yay.
<gary_poster> abentley: quick one, maybe (and if it's not, maybe we should ignore the branch for now, because it was a flyby): https://code.edge.launchpad.net/~gary/launchpad/bug643715/+merge/36060
<bdmurray> abentley: I've pushed an updated branch leaving bug as a BugField as that is a link to the bug report and Int returns a dictionary of all the bug properties
<abentley> bdmurray, I'll have a look in a minute.
<abentley> gary_poster, your patch is treating revnos as if they were revision-ids.  It will fail to do an update if the definition of the revno has changed.
<gary_poster> abentley: agreed, but when would that happen for this use case
<abentley> gary_poster, if someone uncommits a revision.
<gary_poster> ew, didn't know you could do that with a public branch but I guess that makes sense within the model
<gary_poster> ok, that's a shame, but if that's a reasonable concern then we should just drop it.  thanks for the look abentley
<abentley> gary_poster, np.
<abentley> gary_poster, I think you also may not have accounted for the possibility that the branch has changed.  Also, it appears that your test will give false negatives if "revision" is not a revno.
<abentley> bdmurray, I notice that you're still using BugField rather than Int.  Rationale?
<bdmurray> leaving bug as a BugField as                   that is a link to the bug report and Int returns a dictionary of all  the bug properties
<abentley> bdmurray, so bug is a reference to the bug, not an Int?
<bdmurray> abentley: yes, afaict
<abentley> bdmurray, okay.  "Int returns a dictionary of all  the bug properties" means that when you use it via the WS, you get a dict?
<bdmurray> abentley: yes, when exported it as an Int I was seeing bug properties like latest_patch_uploaded in a dictionary.
<abentley> bdmurray, r=me.
<bdmurray> abentley: great, thanks!
* abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-09-21
<stub> lifeless: You might be interested in reviewing https://code.edge.https://code.edge.launchpad.net/~stub/launchpad/cronscripts/+merge/36005 -- cronscript oops stuff we discussed and some delinting noise.
<lifeless> what was  LibrarianFormatter for ?
<stub> lifeless: Instead of spitting out exceptions to stderr, it stored them in the Librarian and spat out a URL. For a while some of the batch jobs where insanely noisy and this made them slightly more readable.
<lifeless> kk
<lifeless> CrashScript has huge vertical whitespace
<lifeless> could you compact it a little?
<stub> k
<lifeless> I only ask cause I couldn't really read it without my eyes rolling back in their sockets
<thumper> lifeless: there is also https://code.edge.launchpad.net/~thumper/launchpad/branch-distro-set-old-mature/+merge/36095 if you'd like to approve another trivial branch
<lifeless> line 30, the extra vws weirds me
<lifeless> up to you
<thumper> lifeless: are you vws phobic?
 * wgrant likes LibrarianFormatter.
<wgrant> It's handy until Soyuz gets fixed.
<lifeless> thumper: no, but it often makes things harder to read
<lifeless> thumper: my rule of thumb is that if I need vws I need a new function
<lifeless> thumper: but its a shallow thing - I don't fundamentally care; I'll make it look how I prefer when I touch something, but really, anything goes.
<thumper> :)
<stub> wgrant: I can leave it if it is useful. I didn't think anyone liked it any more.
<wgrant> stub: It's still useful for people to see it in Soyuz upload logs.
<wgrant> Since Soyuz can be a bit harsh sometimes.
<stub> https://bugs.edge.launchpad.net/launchpad-foundations/+bug/641103
<_mup_> Bug #641103: LibrarianFormatter should die <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/641103>
<thumper> mwhudson: interested in the branch-distro script change to avoid the scan/
<thumper> ?
<thumper> mwhudson: it is surprisingly small
<mwhudson> thumper: sure
 * thumper gets it ready
 * thumper sighs
<thumper> other test failures
<thumper> due to my groovey new code
<thumper> which now assumes real database data
<mwhudson> heh
<mwhudson> there are some rather extreme fakes in the tests for branch-distro iirc
 * thumper is rerunning the tests
 * thumper forgot -v
<thumper> it is snowing again
<mwhudson> !
<mwhudson> very pleasant day in wellington today
<thumper> w00t
<thumper> all passing again
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/branch-distro-avoid-scan/+merge/36103
<thumper> mwhudson: I'm going to help with dinner, feel free to leave comments, and I'll look later
<mwhudson> thumper: ok
* StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [StevenK, noodles775]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: noodles775 || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bryceh>  On call: gmb || Reviewing: noodles775 || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bryceh changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: noodles775 || queue: [bryceharrington]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb
<gmb> noodles775, r=me; one change needed and one slight refactoring requested.
<noodles775> Thanks gmb, I'll take a look.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bryceh || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bryceh, lp-617679-code, right?
<noodles775> gmb: with makeLaunchpadService(), why not just put it in TestCaseWithFactory? (ie. is there a reason why I should have to think about a separate subclass etc. when adding an API test?)
<gmb> noodles775, Well, if you added a WebServiceTestCase, you could also set its layer to AppServiceLayer by default. Other than that, though, there's no reason not to just put it in *WithFactory.
<lifeless> gmb: why do you need a WebServiceTestCase
<gmb> lifeless, You don't. It would save people from having to re-do what noodles775 did in one of his test cases for the webservice, but it was just an idea. As noodles775 said, might as well add his makeLaunchpadService() method to TestCaseWithFactory,.
<lifeless> I haven't seen the code, but unless it also uses factory methods, why not just have it standalone ?
<gmb> That would also work. noodles775, can you confirm that that's doable?
<noodles775> It does use factory.makePerson currently.
<noodles775> But sure, it could go in lp.testing._webservice.py
<lifeless> if its using factory.makePerson it should be on the factory
<gmb> D'OH.
<gmb> I blame lack of caffeine for my not spotting the blindingly obvious.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> noodles775, So, as has been said, that method should be on the factory, not on some other TestCase.
<noodles775> Yep, I'll move it to the factory.
<gmb> Cool.
 * gmb grabs a drink; bbiab
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: thumper || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: lunch || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: lunch, Ursinha || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -, Ursinha || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Morning mars
<mars> Morning gmb!
<gmb> mars, I have one for your queue. https://code.edge.launchpad.net/~gmb/launchpad/bug-644346/+merge/36143
* gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -, Ursinha || queue: [gmb]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> Nice and small.
<mars> gmb, thanks
* jcsackett changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -, Ursinha || queue: [gmb, jcsackett]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> hello, mars and gmb.
<jcsackett> https://code.edge.launchpad.net/~jcsackett/launchpad/series-need-usage-attributes-643902/+merge/36145 could use a review, when you have time.
<gmb> jcsackett, I'll take a look shorly.
<gmb> *shortly
<mars> jcsackett, I have two in the queue before that
<mars> thanks gmb
<jcsackett> gmb, thanks. :-)
* gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: jcsackett, Ursinha || queue: [gmb]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * gmb grabs a drink first
<gmb> jcsackett, On line 177 you say "# If translations_usage is set for the Product, respect it" but then you reference self.distribution.translations_usage. I'm assuming you need to s/Product/Distribution in the comment.
<jcsackett> gmb: you're correct. making the change.
<gmb> Same on line 241
<gmb> jcsackett, You need to add some comments or docstrings to the start of your tests explaining what they test (you should phrase this as a statement of expected behaviour, e.g. "The frobnob goes boing." rather than "Test that the frobnob goes boing."
<gmb> Other than that, r=me.
* gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || Reviewing: -, Ursinha || queue: [gmb]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> gmb: you got it; i'll make those changes.
<jcsackett> thanks a lot!
<abentley> rockstar, could you review https://code.edge.launchpad.net/~abentley/launchpad/incremental-diff-driveby/+merge/36156 please?
<rockstar> abentley, sure.
<abentley> rockstar, thanks.
<rockstar> abentley, is any of that lint legitimate?
<abentley> rockstar, I don't think so, but I'm open to other opinions.
<rockstar> abentley, it looks like the first one, line 715 of ./lib/lp/code/interfaces/branchmergeproposal.py, might be.
* gmb changed the topic of #launchpad-reviews to: On call: mars || Reviewing: -, Ursinha || queue: [gmb]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, AFAICT, there are two blank lines above notify_modified.
<rockstar> abentley, yeah, I looked. WTF.
<abentley> rockstar, I think it's because the comment is there, but if that's the reason, it should say "found 0".
<henninge> danilos: https://code.edge.launchpad.net/~henninge/launchpad/recife-poimport/+merge/36165
<danilos> henninge, thanks, can you please find someone else as a reviewer if you are in a hurry to get some rest because I am chasing that nasty bug you filed :)
<henninge> danilos: ok
<henninge> danilos: that is a good excuse ;-)
<henninge> unfortunately it's oversized
* gmb changed the topic of #launchpad-reviews to: On call: mars || Reviewing: Ursinha || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: mars || Reviewing: Ursinha || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> mars I added a large branch to your queue. While most the the branch are deletes, It still looks scary. I can get EdwinGrubbs or bac to do it tomorrow if you do not have time
<mars> sinzui, I'll take it
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: sinzui || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> deryck[lunch], ping when you return
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> hi mars
<mars> hi deryck
<mars> deryck, I have a review here from mbp where he asks for a careful review and some advice on testing his mail handling code.
<mars> deryck, I was wondering if someone on your team would have knowledge in that domain that could help Martin out?
<mars> deryck, it is some work he did on the DKIM code: https://code.edge.launchpad.net/~mbp/launchpad/dkim/+merge/35985
<deryck> mars, perhaps gmb or allenap.  But the code team should have some expertise there, too. abentley wrote BaseMailer, I believe.
<mars> deryck, great. thank you for the help
<deryck> np
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [jml]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> StevenK, I am going to save jml's review for you.  It moves code, so no need to balk at the line-count :)
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [jml]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [[open],jml]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || Reviewing: - || queue: [jml]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/safe-delete-recipe-build/+merge/36195 ?  I think you'll like it.
<rockstar> abentley, sure.
<abentley> rockstar, how about this? https://code.edge.launchpad.net/~abentley/launchpad/upload-failure-email-log/+merge/36205
<abentley> rockstar, mercy buckups.
<rockstar> abentley, no problem.  Thanks for fixing that.
<mwhudson> thumper: sorry for not reviewing branch-distro-avoid-scan yesterday, i don't know how i managed to forget about it
<thumper> mwhudson: np, it isn't super urgent
<thumper> not critical that is
<abentley> thumper, incremental diff screenshot: http://people.canonical.com/~abentley/Screenshot-5.png
<thumper> abentley: do we record the (from, to) revno pairs/
<thumper> abentley: it'd be nice to see a heading on that block
<thumper> perhaps saying something like "Incremental diff from revno 123 - 130"
<thumper> maybe
<abentley> thumper, no, we don't record revnos.  We record the revisions.
<thumper> yay, another reason for the branch revision table
<thumper> abentley: skype?
<abentley> thumper, sure.
<thumper> abentley: no headphones
* mars changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-09-22
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> thumper: you're probably eod, but if notâ¦ care to review?  https://code.edge.launchpad.net/~jtv/launchpad/bug-643345/+merge/36258
<jtv> Anyone else who could review for me?  stub?
<jtv> jml maybe?
<jtv> gmb, you perhaps?
<jtv> allenap then?
<jtv> StevenK?
<StevenK> jtv: I can look, but I can only give you code*
<jtv> StevenK: theoretically that would slow down the final review, but it's not a very long branch so might as well.
<jtv> did noodles775 just join us?
<noodles775> Join you for what? ;) Morning!
<jtv> morning!
<jtv> I'm shopping around for a reviewer for a CP branchâ¦ StevenK could do it, but will need a mentor to approve his review.
<jtv> It's this branch: https://code.edge.launchpad.net/~jtv/launchpad/bug-643345/+merge/36258
<noodles775> Sure.
<jtv> \o/
<noodles775> jtv: maybe you could help me too? I've got an ec2 test failure email, but the tests pass locally, and the version of db-devel that was most recently merged also went through buildbot fine (r9808: https://lpbuildbot.canonical.com/builders/lucid_db_lp/builds/237)
<jtv> noodles775: happy to return the favour
<noodles775> I've stared at it a bit yesterday, but would love a second pair of eyes?
 * jtv looks
<noodles775> Thanks, I'll forward the failure email.
<jtv> noodles775: I know the feelingâI could do with a new pair of eyes myself.
<noodles775> The email has the branch that you should be able to make and test locally. Thanks jtv
<StevenK> noodles775: jtv's MP doesn't have any obvious gotchas that I can see.
<noodles775> Thanks StevenK
<jtv> noodles775: wow, that's not a very informative failure is it?
<noodles775> jtv: no, and it's not just one... there's also 2 windmill errors (which also pass locally, at least did for me).
<jtv> Yes, I'm looking at them
<jtv> noodles775: the test failure (as opposed to the 2 errors) looks familiar
<noodles775> I thought so too, but couldn't see any evidence of it on buildbot.
<jtv> we had something like that recentlyâ¦  I think StevenK investigated but the problem was elusive
<jtv> IIRC the test had just been converted from using getLatestOops (or something along those lines) to self.oopses (or something along those lines) when that happened.
<noodles775> StevenK: http://pastebin.ubuntu.com/498260/
<noodles775> ?
<noodles775> jtv: did it also fail only on ec2 test and not on db-devel? (that would explain why I can't see it on the recent buildbot runs).
<noodles775> jtv: And if that's the case, can you confirm for me that all 3 tests pass for you locally on that branch, and I'll submit it?
<StevenK> Funny you should mention that: https://hudson.wedontsleep.org/job/db-devel/44/testReport/lp.services.job.tests.test_runner/TestJobRunner/test_runAll_mails_oopses/
<jtv> noodles775: the former is something StevenK can answer better, and the latter is something I'm about to try
<noodles775> StevenK: Ah! So it's not just my branch! So why is it failing ec2 test and hudson, but not db-devel?
<StevenK> noodles775: To be completly honest, I'm not sure
<StevenK> But I am happy that hudson has a test failure that is useful and reproducable
<noodles775> Different versions of oops-tools?
<jtv> could that make a difference to the running of the tests?
<jtv> I thought that was all post-facto processing, not related to actual generation of oopses.
<jtv> noodles775: the "failure" one passes for meâ¦ trying the "error" ones.
<noodles775> Thanks.
<jtv> StevenK: found any nits to pick yet? :)
<noodles775> jtv: only things I've thought so far are:
<noodles775> 1) It wasn't obvious to me without reading the code below that committer_string isn't an actual commit string provided (somehow) by the committer. Not sure if there's a better name, or maybe its just me.
<noodles775> 2) In getBzrCommitterID you're using self.committer.preferredemail, even though abentley's change of the related bug title seems to indicate this is wrong?
<noodles775> (but I don't know the background)
<jtv> noodles775: the first of those errored windmill tests passed for me, but the second crashed firefox.  (I'm on maverick beta)
<noodles775> So am I... that's interesting though, I'd not run them together, always separately.
<jtv> noodles775: I'd be happy to come up with a better name for 1.  As for 2, that falls under the "laying groundwork" partâthere must be better fixes, but for now I just want the failures out of the way.
<jtv> noodles775: I'm running the tests separately as well.
<noodles775> Yes, but (regarding 2) couldn't you just remove that elif for the moment to get the failures out of the way?
<thumper> jtv: reviewed
<noodles775> jtv: Ah, another thought... I wonder if the windmill failures are related to the FF version.
<jtv> noodles775: well AIUI it's wrong _because_ it's not guaranteed to be present.
<jtv> Not inherently wrong otherwise that I know ofâpeople on that end made a deliberate decision to use email addresses, and I don't want to controvert them wholesale.
<jtv> thumper: thanks!  I also have noodles775 looking at it just now
<thumper> jtv: ok
<noodles775> Ah, I'll just add my review so far as a comment FTR - I didn't realise thumper was/had looked at it.
<thumper> noodles775: that's fine
<jtv> thumper: but very happy to have your view on it
<thumper> I didn't read all the comments after jtv pinged
<jtv> there were too many :)
<jtv> noodles775: the branch puller test leaves a branch locked, but otherwise its test passes for me.
<noodles775> jtv: so one windmill test errored for you... which one was it? test_pofile_new_translation_autoselect ?
<jtv> yes
 * noodles775 runs it locally
<jtv> and once Firefox gets involved, I'm not even willing to guess at the moving parts and variables that come into playâ¦
<jtv> thumper: pushed changes for your review
<jtv> (moving on to noodles' review)
<jtv> noodles775: pushed changes for your review
<thumper> ack
<jtv> noodles775: just pushed another tiny, tiny thing I missed in renaming that parameter.  Nothing you wouldn't expect.
<noodles775> Yep, looking now.
<jtv> dankeschÃ¶n
<noodles775> jtv: committer_id is much clearer, thanks. And test_pofile_new_translation_autoselect still passes fine for me locally.
<jtv> noodles775: maverick or lucid?
<noodles775> maverick
<jtv> same here :(
<jtv> may be a plugin
<jtv> phone callâ¦
<noodles775> jtv, StevenK: the oops error just appeared on buildbot, so that one seems spurious.
<jtv> :/
<StevenK> Oooh, so it is real
<noodles775> Real in the sense that it happens sometimes even on buildbot, yes.
<jtv> noodles775: by the way, sorry about the double-review hassle; I was knocking on doors and moving on when I got no reply.  Are things to your satisfaction now?
<noodles775> jtv: no problem! Yep - I only reviewed the code as per my comment, but I'm happy with that (after your variable change).
<jtv> noodles775: thanksâthen I'll land on devel
<jml> *sigh*
<jml> no one reviewed my branch yesterday
<jtv> jml: as it happens, I'm OCR.  Putting out some fires today, which is why I haven't gotten around to the backlog, but I could do it now.
<jml> jtv, that'd be great, thanks.
<jml> https://code.edge.launchpad.net/~jml/launchpad/buildd-slavescanner-bustage/+merge/36187
* jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: jml || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jml: you're turning into a Hindu god.
<jml> jtv, oh?
<jtv> Lots of creative destruction and, evidently, more than the normal quota of hands to type with.
<jtv> jml: who is "we" in this narrative by the way?
<jtv> Royal "we"?
<jml> jtv, bigjools & I
<jml> jtv, and Shiva
<jtv> jml: â¦and this is why the comma is a bad way to indicate that you're addressing someone directly in IRC.  :)
<jml> jtv, I hate it the comma autocomplete
<jml> jtv, but xchat doesn't give me a control to change it
<jtv> ah!  there are clients that do that?
<jtv> a mild bowel discomfort be upon them
<jtv> jml: diff line 41 is weird: "Similarly, but"
<jml> jtv, oh yeah. I guess "But" will do
<jtv> jml: I'm unable to find the connection between the narrative and the test there, but that's probably just me.  If you assume me they match, that's good enough for meâas long as I get you to expend some thought on the matter.
<jml> jtv, they do match. the broader narrative is about checking the deb lines.
<jtv> jml: I believe you blindly.
<jml> jtv, I don't think archive-dependencies.txt is a well-written document, and would rather (one day) move all of those to unit tests.
<jtv> I'm glad you're making the effort.
<jtv> jml: diff line 698 would read even better IMHO if you switched conditions around to read "... and not ..."
<jml> jtv, good call
<jtv> jml: also, I see some XXX comments that lack date, name, and bug reference.
<jml> jtv, yeah, I'll quickly review those now.
<jtv> thanks
<jtv> jml: a bit scary that the OkSlave previously did almost nothing but then suddenly Popen'ed in resume()â¦
<jml> jtv, yes, it is.
<jml> jtv, lots of this stuff is scary
<jml> jtv, maybe in a branch today or tomorrow we'll actually kill all of those mocks and do the mocking at the xmlrpclib layer.
<jtv> that sounds like a cool ideaâ¦ so much of it is no-brainer replication anyway.
<jml> exactly
<jtv> The call log is quite close to what FakeMethod does, even.
<jml> yeah, but we are selective about what we append
<jml> the whole thing is really close to niemeyer's mocker
 * jtv sobs quietly into a handkerchief, then composes himself
<jml> which I'm not sure why we don't use.
<jtv> Not in this branch though!
<jtv> But yes, if that fits, by all means do it.
<jtv> Ah, a meaningful docstring on a test case.  The astrologists always said it would happen, but I didn't expect to see it in my lifetime.
<jml> jtv, fwiw, Michael Foord, who was recently hired by Canonical, also has a popular mocking library :)
<jtv> TMTM
<jml> something like that :)
<jtv> jml: why does TestBinaryBuildPackageBehavior.setUp invoke its super, create a factory, and log in?  Doesn't TestCaseWithFactory provide all that?
<jml> jtv, TCWF does indeed provide all of that, but we are subclassing Twisted's TestCase, since we need the Deferred support
<jml> jtv, and multi inheritance for TestCases is a nightmare.
<jtv> jml: I only see it being derived from unittest.TestCase.
<jml> jtv, "from twisted.trial import unittest"
<jtv> that is obscene
<jml> jtv, we all make mistakes in our youth that we later come to regret
<jml> jtv, one of mine was calling the main trial module 'unittest'
<jtv> jml: was it legal in the country you grew up in?
<jtv> ah wait, I'm thinking of drugs there.
<jtv> Different mistake.
<jml> jtv, yes, but socially frowned upon.
<jtv> Good, good.
<jtv> How about, in this special case, importing twisted and qualifying as twisted.trial.unittest.TestCase?
<jml> jtv, well, we often do "from twisted.trial.unittest import TestCase as TrialTestCase"...
<jtv> that'd work too
<jml> jtv, but I didn't do that here because it adds TrialTestCase to the test suite
<jml> (also my fault)
<jtv> Can you "from twisted.trial import unittest as twisted_unittest"?
<jml> jtv, yeah, I'll do something like that.
<jtv> Thanks.  I sorely need a break; brb
<jml> np
<jtv> jml: back from breakâ¦ have you considered taking twisted out of the test altogether?
<jml> jtv, no.
<jtv> the obvious follow-up: would it be worth considering?
<jtv> I did that in one of the slave tests.
<jml> jtv, ok, sorry, what I meant was that I considered and rejected it...
<jtv> I forget how, sadly.
<jtv> Ah :)
<jml> jtv, we're changing the BuilderSlave to have a Twisted API
<jml> jtv, so that the buildmaster can be properly written
<jml> jtv, that API change is going to bubble up and affect all of the *Behavior objects
<jtv> ah
<jtv> And, I suppose, make it far too risky to try and hack your way out of twisted just for the purpose of this test.
<jml> jtv, that's pretty much what's driving this work
<jml> jtv, indeed.
<jml> jtv, we *might* be able to do something tricky later on
<jml> jtv, but until things are stabilized, I'd rather not.
<jtv> But Knuth's Law.  Got it.
<jml> Yes. That one :)
<jml> (as an example, lp.codehosting.vfs has code that works with either a synchronous xmlrpc client or an asynchronous one).
<jtv> In ll. 984â992 of the diff, would it perhaps be clearer to use a list comprehension, perhaps even without the nested function definition?
<jtv> expected = [
<jtv>     ['cacheFile', 'sendFileToSlave', ('ensurepresent', url, '', '')]
<jtv>     for url in [chroot.http_url] + extra_urls]
<jtv> Oh, you're extending, not appending.
<jml> jtv, I'm open for ideas on making that code clearer.
<jml> jtv, I had to switch to Haskell mode to be mentally capable of solving the problem
<jtv> Good start.
<jtv> jml: personally, I've come off the idea of building these great clever assertion methods in tests.  I do like the general approach, but find it better to return something I can compare in a single comparison directly in the test.
<jtv> Makes failures much easier to deal with.
<jml> jtv, certainly that would be nice
<jml> jtv, the current API we're testing is a bit of a blunt instrument. I can't think of anything less bad than the assertion we have now.
<jtv> jml: I think this might be clearerâ¦ http://paste.ubuntu.com/498340/
<jml> jtv, you mean, a) change the function to a list comp; b) change the assertion method into a helper that builds the object to compare against?
<jtv> jml: exactly
<jml> ok. I'm willing to give that a try.
<jtv> (Of course not all of the function will fit into a list comprehension, but forming a list would be the obvious core of it)
<jtv> jml: I'd also extract the dict into a variable, which will break down the structure a bit and also allow the contents to follow our syntax guidelines.
<jml> how do they break from our syntax guidelines?
<jtv> syntax guidelines say dictionary = {
<jtv>     'item1': 1,
<jtv>     'item2': 2,
<jtv>     }
<jml> oh right.
<jtv> ahhh, looking at the calls, I see why you have the assertion method.  It must be fun debugging this stuff.
<jml> jtv, the test failures are relatively easy to grok.
<jtv> btw I guess twisted tests are expected to return a deferred, or watchumacallit?
<jml> jtv, but, well, big fat APIs that do a lot of stuff are always hard to test.
<jml> jtv, yes, they are
<jml> jtv, full story: if a trial test returns a deferred, trial will wait for that deferred to fire before moving on to the next test.
<jml> jtv, that facility is necessary when dealing w/ async code
<jtv> jml: that sort of suggests itselfâ¦ but makes the naming similarity to unittest all the more unfortunate
<jml> jtv, in most ways it's quite similar to unittest
<jtv> It's good to follow the same patterns, but awkward to have a different call model in such a close semblance of unittest
<jtv> jml: in ll. 1073â1077 of the diff, it may be easier to construct the expected error string outside of the assertion.
<jtv> I wish these tests could be shorter, but see no very reasonable way to do it.
<jml> sure.
<jml> me neither.
<jml> I'm hoping that as we push deeper into this stuff something will present itself
<jtv> You could extract the chroot-setting part into a helper.  Bit ugly with the commit in the middle (why not transaction.commit, by the way?) but would extract some unhelpful text from the tests.
<jtv> I suspect the setup of builder and slave could also be extracted.
<jml> maybe.
<jml> their construction is parametrized differently in different tests, so I'm a little bit wary of prematurely refactoring.
<jml> not transaction.commit() because zopeless.
<jml> maybe that doesn't matter these days
<jtv> I don't think it does
<jml> In that case, layer.txn ought to be hidden.
<jtv> I'm not sure but I think layer.txn == transaction
<jtv> jml: the archive and the build are parameterized differently, but the builder and slave..?
<jml> jtv, parametrized by the archive, arch series, virtualized or not.
<jtv> jml: of course there's nothing stopping a helper from accepting similar pameters
<jml> yeah.
<jml> but then when we add a new test we'll probably need to change the helper
<jtv> it's you call though; the question is how relevant it is to the test
<jml> I'd like to leave it for now.
<jml> where are we at more generally w/ the review? bigjools is doing some stuff that I'd like to stick my nose into.
<jtv> jml: I think we're through most of the code you added; after that I'll make one more brief pass to look for lost coverage but I don't expect to find anything.
<jml> jtv, cool. thanks.
<jtv> jml: I suppose the bit in lines 63â99 is really just unnecessarily duplicating Librarian work?
<jtv> jml: I'm saturated, so not much use for me to look through this further.  I'll have to trust that the coverage matches, or if not, that (1) people will be afraid to touch the code anyway or (2) any failures will cause widespread panic and so will be instantly fixed and tested in a modern way.
<jml> jtv, understood
<jtv> jml: got a revision for me?  I'm EOD'ing as far as all else is concerned.
<jml> jtv, the 63-99 is already tested in buildmaster/model/tests/test_builder.py
<jml> jtv, http://pastebin.ubuntu.com/498363/
<jtv> thanks
<jml> jtv, I had to keep the assert helper because the cookie is only created after startJob is called.
<jtv> jml: understood, I thinkâ¦ still think you've made an improvement there.
<jml> jtv, thanks.
<jtv> Brings the interesting part of a failure one level closer to where the reader looks for it.
<jtv> In the traceback, I mean.
<jml> yeah.
<jtv> jml: I've approved.
<jtv> On call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> I meantâ¦
* jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> jtv, thanks.
<jtv> np
<deryck> adeuring, hi, can I get a review from you? (he says, picking a bugs guy at random.)
<adeuring> deryck: sure
<deryck> adeuring, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/displayname-private-team-oops-634847/+merge/36199
* sinzui changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [sinzui, jcsackett]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> MP for me: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738-2/+merge/36242
<adeuring> deryck: r=me
<deryck> adeuring, awesome, thanks!
<mars> StevenK, still online?
<StevenK> mars: I am
<mars> StevenK, heya, did you get to any of the reviews that I left in the queue last night?
<StevenK> mars: You know, I forgot I was OCR today. :-(
<mars> StevenK, no problem, everyone does that from time to time
<mars> StevenK, if you would like, there are two reviews in the channel queue, and one in +activereviews.  But I realize it is late there, well past your EOD
<StevenK> mars: Yup, I don't think I can think straight enough :-)
<mars> StevenK, next week then.  leonardr might be OCR, but we can do the same thing again, and save up some reviews for you.  There are usually a few at the end of the day here
<mars> so it doesn't impact the developers if leonardr and I save them for you
<sinzui> EdwinGrubbs, ping
<EdwinGrubbs> sinzui: hi
<sinzui> EdwinGrubbs,  can you review the two sql scripts that Chex and I tested on staging yesterday to clean up deactivated/merged user data? We need another engineer to agree they are sane: http://pastebin.ubuntu.com/498514/ and http://pastebin.ubuntu.com/498518/
<sinzui> EdwinGrubbs, Staging ran fine, and the UI looked great after we ran them
<EdwinGrubbs> sinzui: wouldn't it be a good idea to check whether Person.merged is not null instead of just trusting the name?
<sinzui> Edwin I think that would be a good idea
<EdwinGrubbs> sinzui: I don't see any other issues. However, it would be nice if a backup of those tables could be taken just in case some user needs to be reverted however unlikely that is.
<sinzui> EdwinGrubbs, since we tested on staging. without any issues. I think I should revise the script, maybe combine the deactivated/merged rule so that it is one script. Then test it on staging next week with fresh data
<EdwinGrubbs> sinzui: will removing the team memberships have an adverse affect on the non-human user ~katie, who is suspended and a celebrity in the code.
<sinzui> Oh :( I bet it will
<sinzui> I need to think about that
<sinzui> Edwin do you think is it safe to remove suspended users from the invited and proposed states, but ignore those that are members? I think it is better to leave some cruft behind and run the parts we trust to work
<rockstar> abentley, could I get a review of https://code.edge.launchpad.net/~rockstar/launchpad/build-farm-job-constraint/+merge/36219
<rockstar> (I was asked to leave it for StevenK so that he could get review experience, but he didn't touch it yesterday)
<abentley> rockstar, r=me
<rockstar> abentley, thanks.
<deryck> adeuring, hi.  So a couple minor thoughts about your branch....
<deryck> adeuring, do you really need "We&#8217;ve" in place of the plain "We've" in HTML?
<adeuring> deryck: I simply copied that from the regular error page
<adeuring> but I can change it to a simple "'"
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: - || queue: [sinzui, jcsackett]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<deryck> adeuring, yeah, let's do that.   Unless there's some compelling reason to encode that character.
<deryck> adeuring, and then I tend to prefer using ellipses to hide more of that error mesaage in the page test.  In case the template changes it doesn't make the test fragile.
<deryck> adeuring, but that's up to you.  Otherwise, r=me.
<adeuring> deryck: right using more "..." makes sense
<adeuring> deryck: and thanks!
<deryck> adeuring, np
<deryck> thanks for the fix.
<leonardr> EdwinGrubbs, who can do a ui review? i haven't needed one since we instituted the process, but i need one now
<EdwinGrubbs> leonardr: I can do it, but I'm not graduated, so you will have to get two reviews. noodles has graduated, and he is oncall tomorrow morning.
<leonardr> ok, cool
<leonardr> i'll get something ready
<abentley> EdwinGrubbs, could you please review https://code.launchpad.net/~abentley/launchpad/fix-groupby/+merge/36360 ?
<EdwinGrubbs> abentley: sure, I'll let you jump the queue since the branch is so small.
<abentley> EdwinGrubbs, thanks.
<jcsackett> EdwinGrubbs: realized i posted this before you came on call: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738-2/+merge/36242; it's the MP i already put myself in the queue for, when you have time.
<EdwinGrubbs> jcsackett: I should get to that in about an hour.
<jcsackett> EdwinGrubbs: fantastic. thanks. :-)
<leonardr> edwingrubbs: https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/36363
<leonardr> if you can't get to it, no big deal
<EdwinGrubbs> we'll see
<EdwinGrubbs> abentley: r=me
<abentley> EdwinGrubbs, thanks.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: sinzui || queue: [jcsackett, leonardr]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> abentley, could you take a look at: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-too-new/+merge/36379
<abentley> rockstar, you need to do monkeypatches in a try/finally block; otherwise, they can leak.
<abentley> rockstar, or you could use a with statement.
<abentley> rockstar, the with statement gives you a little code reuse.
<rockstar> abentley, so, you're suggesting creating a contextmanager that does the monkey patch?
<abentley> rockstar, yeah, I think that's the cleanest way.
<rockstar> abentley, okay.
<abentley> r=me with that change.
<EdwinGrubbs> sinzui: review sent
<rockstar> abentley, where should that context manager live?  lp.code.testing doesn't exist, but that makes the most sense.
<abentley> rockstar, we use lp.code.tests.helpers for that kind of thing.
<rockstar> Ah, even better.
<sinzui> thanks edwin
<rockstar> abentley, a cursory review of http://bazaar.launchpad.net/~rockstar/launchpad/recipe-too-new/revision/11608 please?
<abentley> rockstar, you should do the assignment to RecipeParser.NEWEST_VERSION before the try.  The only thing in the try should be the yield.
<rockstar> abentley, really?  Is there some extra logic in the context manager then?
<abentley> rockstar, in this case, it doesn't matter, because the teardown doesn't require the setup to have succeeded.
<rockstar> abentley, ah, okay.
<abentley> rockstar, no.  This is true of all try/finally blocks.
<rockstar> abentley, yeah, I hadn't really thought that through.
<EdwinGrubbs> jcsackett: r=me
<jcsackett> EdwinGrubbs: thanks.
<EdwinGrubbs> leonardr: review sent
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> edwin, thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-09-23
<wallyworld_> any on-call reviewers around?
<mwhudson> wallyworld_: i only play one on tv
<mwhudson> wallyworld_: link me up?
<wallyworld_> :-) here it comes...
<wallyworld_> https://code.edge.launchpad.net/~wallyworld/launchpad/recursive-branch-resolution-failure/+merge/36403
<wallyworld_> thhanks in advance
<mwhudson> wallyworld_: http://launchpadlibrarian.net/56281944/Screenshot-1.png is pretty funny :-)
<wallyworld_> yes, except if it was your code that introduced the behaviour :-/
<mwhudson> wallyworld_: do you think you can change the name of the 'url' variable in redirect_branch ?
<mwhudson> wallyworld_: perhaps to referer or http_referer ?
<wallyworld_> sure. how about "mary"?
<wallyworld_> :-)
<wallyworld_> ok.
<wallyworld_> that's a better name for sure
<mwhudson> well, meaningless *might* be better than confusing....
<mwhudson> otherwise fine
 * mwhudson goes to do the clicky clicky
<wallyworld_> cool. thanks.
<wallyworld_> mwhudson: btw, tim let the url variable name through the first time around :-)
<mwhudson> hm, really redirectSubTree(None) should explode
<wallyworld_> yeah, on firefox and chrome it does
<mwhudson> i mean serverside
<wallyworld_> not sure, haven't looked into that code path
<mwhudson> it's clearly a programming mistake
<mwhudson> wallyworld_: it seems like it redirects to +branch/None :)
<wallyworld_> yes, it does, and then it comes back into the redirect method and so on
<wallyworld_> chrome/firefox after a while complain hat the target url can never be reached
<mwhudson> right
<wallyworld_> not sure what i did but it took me lots of stuffing around to even be able to get the screenshot attached to the bug report to show up
<wallyworld_> so i just looked at the code and guess the problem
<mwhudson> what i am saying is that calling redirectSubtree(None) should raise, say, AssertionError
<wallyworld_> ack.
<mwhudson> which would result in an OOPS
<wallyworld_> +1
<mwhudson> i guess i can file that as a bug
<wallyworld_> want me to do a drive by?
<wallyworld_> on second thoughts, too much other stuff could potential be affected
<mwhudson> wallyworld_: not really
<wallyworld_> best to do a new bug
<wallyworld_> mwhudson:  i'll fix the variable and land it. thanks for the review
<mwhudson> wallyworld_: np
<mwhudson> wallyworld_: i filed https://bugs.edge.launchpad.net/launchpad-foundations/+bug/645751 btw
<_mup_> Bug #645751: Navigation.redirectSubTree(None) should OOPS <Launchpad Foundations:New> <https://launchpad.net/bugs/645751>
<wallyworld_> mwhudson: thanks. i looked at the variable name and it should not be called http_referer. it is actually the target url that will be redirected to. how about then "target_url"
<wallyworld_> it is just initially set to the refer
<wallyworld_> i can add a comment in the code if you like as well
<mwhudson> oh
<wallyworld_> just to make it explicitly clear
<mwhudson> maybe then a comment to say that the only way it can be None is if the resolution failed _and_ there was no referrer?
<mwhudson> by the "is None" check
<wallyworld_> yes, ok. i think the main thing missing was some code comments to explain things
<wallyworld_> i mentioned stuff in the merge proposal but not the code
<wallyworld_> mwhudson: ping?
<mwhudson> wallyworld_: hi
<wallyworld_> there was a code path that i missed out
<wallyworld_> if the branch was private, it could still redirect to a None url
<wallyworld_> i fixed the code and added a new unit test
<wallyworld_> can you please take a quick look?
<mwhudson> wallyworld_: ok
<wallyworld_> thks
<mwhudson> wallyworld_: have you pushed the branch?
<wallyworld_> yep. lp may be having a little snooze
<mwhudson> wallyworld_: i bet the fact that bazaar.launchpad.net:22 has fallen over is stopping you
<wallyworld_> bugger
<mwhudson> wallyworld_: it's back now
<wallyworld_> ack
<wallyworld_> mwhudson: pushed this time. i didn't notice the error last time.
<mwhudson> wallyworld_: looks fine
<wallyworld_> excellent. thanks!
 * mwhudson goes for the bus
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: sinzui || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> can I get a quick review for https://code.edge.launchpad.net/~jml/launchpad/better-slave-mock/+merge/36417
<noodles775> Sure
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jml  || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jml> noodles775, thanks.
<noodles775> jml: all good, r=me
<jml> noodles775,
<jml> noodles775, cheers.
<lifeless> noodles775: hi
<noodles775> Hi lifeless
<lifeless> noodles775: could you please try reenabling that test.
<lifeless> it was fixed, on devel, I'm positive.
<noodles775> jml: did you get to check the rev of devel when your branch failed ec2?
<jml> noodles775, no, I didn't.
<jml> noodles775, no one gave the revno of the threatened fix, and things are pretty hectic around here
<lifeless> there is a subtle problem in oops with getLastOopsReport; that test didn't need to use it, and my branch which is landed fixed that.
<lifeless> I know it fixed it because it was landed *with* a branch suffering the problem.
<noodles775> jml: if you can forward your ec2 failure email, I'll happily chase it up.
<noodles775> lifeless: sure, I'm happy to re-enable it, I just want to check the revs. Which rev was your fix?
<lifeless> dunno, I just know I got the 'its merged' mail.
<jml> noodles775, http://paste.ubuntu.com/497557/ ; http://paste.ubuntu.com/497555/
<lifeless> 11597
<noodles775> Great, re-enabling now... thanks jml, lifeless
<leonardr> noodles775, yesterday i got a ui review from edwin. i'm making his suggested changes, and then i'd like to put my branch in the queue for a ui/code review from you
<noodles775> leonardr: sure - one of us will take a look (I think allenap will be around too).
<leonardr> thanks
<noodles775> leonardr: actually, if you mean the rename-grant-permissions branch, I think lifeless was keen to have a lot more discussion on the dev list about it? (See comments on the MP).
<leonardr> indeed, i was looking right at it
<noodles775> Great.
<noodles775> leonardr: the need for discussion about those changes came up earlier today on irc (lp-dev) - you can find it in the irclogs (search for your mp url) in case it's useful.
* noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: -  || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: -  || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi allenap, i have a branch that is super-sized with a diff of 1432 lines.  can you review it?
<allenap> bac: Yeah, sure. I'm just having a look at jam's branch right now, deciding if I should take it on.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jam || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> allenap: ok, thanks.  https://code.edge.launchpad.net/~bac/launchpad/bug-643538-code/+merge/36377
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> bac: I get conflicts merging into devel.
<bac> allenap: oh, sorry.  let me fix that
<allenap> bac: It's fairly trivial I think.
<allenap> bac: It merges cleanly into the very latest devel. Sorry, I last pulled it a few hours ago.
<bac> allenap: really?  i got conflicts.
<bac> i've merged and am pushing it up now
<allenap> bac: Weird!
<bac> allenap: just pushed r 11528
<noodles775> salgado: Hi! I was going to ask henninge - but he's not available. Are you up for a pre-lim. UI review?
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442
<salgado> noodles775, sure!
<noodles775> Thanks salgado
<james_w> noodles775: very nice
<noodles775> Thanks james_w :)
<james_w> noodles775: that's all for real?
<noodles775> james_w: yep - you can run it with the instructions on the MP. But there's still lots missing (like the actual generation of the diffs etc.).
<salgado> noodles775, what does blacklisting do here?
<james_w> noodles775: that's great
<james_w> noodles775: thanks for your work on this
<noodles775> james_w: np - I'm enjoying the UI work :)
<noodles775> salgado: it allows you to hide differences that you don't care about in your derived distroseries.
 * noodles775 checks to see if the LEP has a better description.
<noodles775> salgado: see the section 'Marking a package as blacklisted/ignored in the thread at: https://lists.launchpad.net/launchpad-dev/msg04536.html
<salgado> oh, that clarifies things.  I was going to ask what's the difference between blacklisted and ignored (which is what I see in one of the mockups)
<salgado> noodles775, so, when you blacklist all versions of something, they show up at +missingpackages?
<noodles775> salgado: no - missing packages is just for packages that are in the parent series, but not the derived. You will see the blacklisted items on this same page, but only when you select the checkbox in the filter options ('include blacklisted packages' on the mock, not yet implemented).
<noodles775> bug 641290
<_mup_> Bug #641290: Add search for DistroSeriesDifferences <derivation> <Soyuz:Triaged> <https://launchpad.net/bugs/641290>
<salgado> noodles775, this may not be pertinent, but have you thought about what the "This difference has been blacklisted ..." text will look like for blacklisted packages when the search allows them to be shown?
<noodles775> salgado: Yes, a little. Part of the reason for graying them out and using that text was that it would/should be consistent with results when searching. Can you see a situation or reason why it would need to be different?
<salgado> noodles775, I like the graying out because it makes it easy to see everything that was changed at a glance and maybe undo, but the text (as well as the links) is a bit too verbose (IMHO) and don't allow switching between all different states, so I was thinking of using 3 radio buttons there.  "Blacklisted: (*) No,  ( ) This version,  ( ) All versions"
<salgado> but that's more like a nice-to-have thing, not that important
<noodles775> salgado: that actually sounds a bit like what I'd proposed on the mockup (a single 'Add to blacklist' that would then give you those options), but henninge convinced me on a previous MP that it was a bit too much... hangon, I'll find the discussion.
<salgado> well, except that the radio buttons allow you to see all the possible states without having to click anything
<salgado> just like the links do
<salgado> but sure, maybe he'll be able to convince me as well. :)
<noodles775> salgado: ok - if you search for blacklist at https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 you'll see a bit of background.
<noodles775> salgado: but I think your radio button option you mention could work too... Also, if you've got suggestions for making the text less verbose while still being easy to understand, let me know :)
<noodles775> s/you/that you/
<salgado> what I like about the radio button is that you see all the options without having to click anything and there's no repetition
<salgado> the downside is that you need vertical real estate, but you seem to have plenty of that
<salgado> as for the text of the link, the only thing I can think of is something like "Blacklist: _this version_ or _all versions_"
<salgado> but I don't quite like that as it seems to suggest you need to blacklist something
* benji changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> my MP is at https://code.edge.launchpad.net/~benji/launchpad/wadl-refactoring/+merge/36452; should be a quick one
<noodles775> salgado: Yeah. RE the radio buttons, I was going to say that it would require two clicks (ie select/submit), but it doesn't of course... we could do the api update when an option is selected quite easily. And that would mean no need for 'Undo'. I like it more and more :)
<salgado> indeed, I hadn't thought about the undo
<abentley> EdwinGrubbs, I had to tweak my branch slightly since you reviewed it.  Do you mind taking another look? https://code.edge.launchpad.net/~abentley/launchpad/fix-groupby/+merge/36360
<salgado> noodles775, I think that's the only suggestion I have.  shall I paste our chat there for future reference?
<noodles775> salgado: yeah, that'd be great, thanks.
<salgado> noodles775, np.  glad you liked my suggestion. :)
<noodles775> I do indeed!
<salgado> noodles775, if I can make another suggestion (this one not UI related), you could do a 'make newsampledata' after running http://pastebin.ubuntu.com/494656/ and merge just those changes to make it easier to play with that page on lp.dev in the future. :)
<noodles775> salgado: you'd prefer that? I thought we wanted to get rid of sample data (maintaining it etc.), and we can make it a one-liner to add the demo data?
<salgado> noodles775, we want to get rid of the test sample data.  if you run that script and run 'make sampledata', you'll be modifying just the demo data. :)
<salgado> they're separate
<salgado> when you run 'make sampledata', the changes done to launchpad_dev end up in the demo data and the ones done to launchpad_ftest end up in the test sample data
<noodles775> Yep - makes sense..., but currently that flag adds a feature flag too, do you think that's valid for the sample data? I'd think the dev server should have the same features as production by default?
<noodles775> s/that flag/that script/
<salgado> that's a good question. I'd be in favour of enabling everything in .dev, but I can see why that may not be desired
<bac> allenap: have time for another, very straightforward one?  or are you sick of me?
<bac> allenap: it is for bug 644550
<_mup_> Bug #644550: Robots can index blueprints when it is not used <bridging-the-gap> <Launchpad Blueprints:In Progress by bac> <https://launchpad.net/bugs/644550>
* bac changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [benji, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> bac: Yeah. I'm nearly finished on your first branch, so sure.
<bac> allenap: thanks.
<allenap> Unless benji's is *huge* :)
<bac> allenap: but, alas, i must step out, so feel free to skip if you have questions.
<benji> nah, my branch is small and in fact all the code has been through review once before
<allenap> Cool.
<allenap> bac: No worries.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: benji || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: benji || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> allenap: MP for code review is here: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-translations-service-643545-0/+merge/36464
<allenap> jcsackett: I'm probably not going to get to it today, sorry :-/
<jcsackett> allenap: that's fine. i'll find someone else then. just saw you in the on call section. :-)
 * jcsackett has yet to figure out when people's days end.
<allenap> jcsackett: Cool. I would do it, but I'm near my EOD.
<jcsackett> i gathered. as i said, no worries, allenap. have a good evening. :-)
<allenap> You too :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> bac: I'll be back online in ~3 hours. I can review your branch then if someone else hasn't gotten there first.
* allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> anyone available for a code review?
<jcsackett> nm. i'll ping again in a few.
<bac> allenap: thanks for the review!
<bac> hey rockstar, you reviewing today?
<rockstar> FUUUUUUUUUUUUUU...
<rockstar> bac, yes
 * rockstar hangs head in shame
* rockstar changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> :)
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> bac, your review got done?
<bac> rockstar: one did, one didn't
<bac> the one that needs doing is pretty trivial
<rockstar> bac, okay.  Lemme do jcsackett's really quick, and then I'll do yours.
<bac> no rush
* bac changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: jcsackett || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> jcsackett|lunch, thanks for PEP8ing tales.py
<rockstar> jcsackett|lunch, so configure_translations and configure_blueprints head to the same URL?  That doesn't make much sense to me.
<EdwinGrubbs> abentley: I'll look at it now.
<rockstar> jcsackett|lunch, why did you remove the Copyright header in lib/lp/translations/tests/test_hastranslationtemplates.py
<EdwinGrubbs> abentley: that looks good
<EdwinGrubbs> abentley: r=me still
<abentley> EdwinGrubbs, thanks.
<jsackett> rockstar: copyright notice was a goof, corrected.
<jsackett> configure_blueprints and configure_translations are both links to edit, which is the place to set up both of those fields. configure_blueprints was added in the unknown-blueprints branch so that the template could be relatively consistent; the same intent and method was borrowed for this branch with configure_translations.
<jsackett> (this is for the distribution, on product they point to different spots).
<jsackett> rockstar ^
<rockstar> jsackett, okay.
<rockstar> jsackett, I think this looks good.  Thanks for the patch.
<jsackett> rockstar: thanks for the review. :-)
<leonardr> rockstar, trivial review coming up for you
<rockstar> leonardr, ack
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/remove-26ism/+merge/36493
<rockstar> leonardr, great.  I'll take a look when I'm off the phone.
<jsackett> salgado: ping
<salgado> hi jsackett
<jsackett> hi, salgado. sinzui has once again directed me to you for round one of ui review on an MP
<jsackett> https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-translations-service-643545-0/+merge/36464
<jsackett> (this time there's sample data that shows the setup :-P)
<jsackett> if you could take a look at it sometime in the near future, salgado, i would appreciate it. no super rush--i'm not looking to land it today.
<bac> thanks rockstar
<salgado> jsackett, oh, sample data is cool, thanks.  I may not get to it today, but in the worst case I'll review it tomorrow
<jsackett> salgado: sounds good. thanks!
<salgado> dev sample data is cool, I mean
<jsackett> salgado: yes. the testing kind is somewhat irritating. :)
<salgado> jsackett, you're being kind.  but I guess you put it that way because you haven't been here for too long so they haven't caused you that much pain. ;)
<jsackett> heheh.
* rockstar changed the topic of #launchpad-reviews to: On call: - || Reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-09-24
<poolie> mwhudson: (i'm here now)
<mwhudson> poolie: ah yes, probably better
<mwhudson> poolie: nope looks great
<mwhudson> you need me to land it?
<mwhudson> poolie: can you add a commit message?
<poolie> sure
<poolie> do i need any special formats or boilerplate or is that added automatically?
<mwhudson> poolie: i need to scoot out in about 5 minutes, so if you can do that quickly i can fire up ec2 land before i go
<mwhudson> poolie: no, ec2 land adds the boilerplate
<poolie> done
<mwhudson> poolie: thanks, launching ec2 land now
<poolie> thanks
<poolie> i guess i should work out how to do that myself
 * mwhudson afk
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || Reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> .eaw
<allenap> Oops.
<jtv> adeuring: got one for you!  https://code.launchpad.net/~jtv/launchpad/bug-645925/+merge/36540
<adeuring> jtv: sure, I'll look
<jtv> thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jtv: you are changing the permissions for lp-main, brnachscanner and fiera, but you test only the permission for branchscanner. what about the other two users? Do we need these changes, are existing tests / real world code breaking without the extended permissions, or should we add tests?
<jtv> adeuring: lp-main is just on general principle (for UI we want to add, manual db maintenance etc).  The fiera change was just for a bit of extra robustness in case the exact interaction between the ORM and the db changes.
<jtv> I originally had a test that tested against several db users, but ended up culling the ones that weren't directly relevant because I felt it turned the whole db access model into too much of a guessing game.
<jtv> But some kind of SELECT check before doing an INSERT, or an INSERT later becoming an INSERT/SELECT/UPDATE is quite natural.
<adeuring> erm, I think its not the tests that make it a guessing game, I think ;)
<jtv> That's right, but the tests should also reflect what goes on.
<jtv> Not "err it's probably one of these, I'll just test against all of them."
<adeuring> hm, yes... OK, so r=me
<jtv> Thanks!
<jtv> Believe me, I didn't _feel_ like limiting it to 1 db user. :)  I just felt that it was one giant copout until I didânot really better than just giving everyone permission or using the write group.
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> salgado: hi! is someone meant to mentor your ui review from yesterday? If so, I'd be keen for them to do it before I switch to use the radio buttons etc.
<salgado> noodles775, yes, sinzui is my mentor
<sinzui> I will do it shortly
<noodles775> Great, thanks guys.
* sinzui changed the topic of #launchpad-reviews to:  On call: adeuring || Reviewing: - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> good morning adeuring
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: sinzui, sinzui || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Thanks sinzui - I'll update to the radio buttons now.
<adeuring> morning bac
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> sinzui: r=me
<sinzui> thanks
<bac> hi salgado, can you do a UI review for me, por favor?
<salgado> bac, claro!
<bac> bueno!  https://code.edge.launchpad.net/~bac/launchpad/bug-643538-code/+merge/36377
<bac> mrevell: do you have time today to do a text review?
<mrevell> Yes bac, certainly. I have a call at the top of the hour. I'll do it after that.
<bac> mrevell: ok, thanks
<allenap> adeuring, bac: Mind if I push a branch into the fifo?
<adeuring> allenap: I'll look
<bac> allenap: not at all
<allenap> adeuring: Thanks.
<bac> sinzui: what branch do you need to have reviewed?  i don't see it on +activereviews
<sinzui> the one I asked jtv to review earlier this week
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac>  sinzui: found it!  MP-hide-n-seek makes it interesting
<abentley> bac, how was it hidden?
<bac> abentley: he had assigned it to jtv, so it didn't show up under "Requested reviews I can do"
<abentley> bac, adeuring: could you please review https://code.edge.launchpad.net/~abentley/launchpad/unauthorized-email/+merge/36504?
<abentley> bac, ah.  Stale data, then.  Dunno how that could be fixed.  Obviously he can reassign it to you or one of your teams.
<adeuring> abentley: I just started a review for Gavin...
<bac> abentley: or he could paste the URL when requesting a review.  no biggie, i've located it.
<bac> abentley: please add to queue
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: allenap, sinzui || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> allenap: there is a somewhat terse XXX in 218 of the diff. could you exand it either into a "standards comopliant" XX or remove it? otherwise, a nice refactoring, r=me
<allenap> adeuring: Sure, and thanks :)
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring, bac || Reviewing: abentley, sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> abentley: r=me
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, sinzui || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> adeuring, thanks.
<bac> hi sinzui, i'm looking at test_product_change_owner_changes_entry_importer and am confused
<sinzui> bac: yes?
<bac> sinzui: it doesn't seem to test what it claims, as i read it
<bac> the final assert tests that the entry importer is still the old_owner
<bac> i expected it to be the new owner
<jcsackett> i just got subscribed to the cdo mailing list...anyone know what that's about?
<jcsackett> sorry, wrong channel for that question.
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> bac, so, to see your changes I need to create a new project and, change the uses_code thing to all different states and check what the project's code page looks like?
<salgado> the mp doesn't clearly say what was changed...
<bac> salgado: yes.  did you see the screenshots?  sorry for the unclear MP
<salgado> no, I didn't
<bac> salgado: did i forget to put them?  looks...
<salgado> I was looking just at the description, which I got by email
 * salgado checks the mp on the web UI
<bac> salgado, mrevell: please see the first comment on the MP
<salgado> bac, they're there
<bac> sorry that if forgot to include it in the original text
<salgado> it's fine; I should've looked at the web UI as well
<mrevell> Hi Brad
<mrevell> bac, For the first screenshot, I have a couple of questions:
<mrevell> 1. I feel the text looks a little tough to get into. Is there a technical reason, or would you otherwise object to, using a bullet list to present the options?
<bac> mrevell: there is no reason.  the text is basically the same as it used to be.  improvements encouraged.
<mrevell> Thanks bac, I'll suggest some in the comment on the review, in that case.
<mrevell> I have no second question, it seems. At least not for screenshot 0.
<bac> ok
<sinzui> bac: I have no idea how I screwed up the test/code. The test is not running because the attr assignment is independent of the subscriber event. I want to show them connected so I need to refactor the whole testcase to ensure the all events are complete before I do the assertion
<bac> sinzui: that makes sense
<bac> mars: ping
<mars> Hi bac
<bac> hi mars, i'm looking at your branch for the py2.5 checker.
<mars> ok
<bac> mars: what do you think of lifeless' suggestions?
<mars> to trash it? :)
<bac> mars: no
<bac> not to trash it but to add it to 'make check'
<bac> and to answer the question about lucid builders
<mars> It could be done
<bac> mars: you don't sound enthusiastic
<mars> I would a) make it a stand-alone script; call that script from lint; call that script from 'make check'; add Py2.5 to the lucid builders
<bac> mars: yes, i agree that would cover it
<sinzui> bac: I am screwed. setting a field attr does not implicitly call notify with the ObjectModifiedEvent. I either need to restore the getter-setter for owner or I discover a wrapper that does what I expect
<mars> bac, so what to do about the MP status - rejected, I guess?
<mars> to be superceded at a later date
<mars> bac, I'll update it
<bac> mars: i'd mark it 'needs fixing'
* adeuring changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> that is a good idea
<bac> i'll do it
<bac> mars: are you implying it'll be a while before you get to it?
<mars> bac, I don't know when I get to it again, so yes, I am implying that
<mars> could be Monday
<mars> could be... infinitely longer than Monday
<bac> mars: i disagree with lifeless on the 'wouldn't bother' part.  i'd say land what you have now...and commit to the changes outlined above in the near future.
<bac> mars: there are some devs that regularly run make lint
<mars> bac, good point
<bac> mars: so i leave it up to you.   land now with a big XXX or defer.
<mars> ok, I will do it that way, and add a comment about the follow-up
<bac> ok, great
* bac changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: is your unknown-translations ready to land?  can i mark it 'approved'?
<jcsackett> there's still some debate going on with it, i believe.
<jcsackett> oh, no, nevermind, sinzui marked it as cool too.
<jcsackett> yes, go ahead.
<jcsackett> bac^
 * jcsackett needs to keep better track of his email.
<benji> bac: I'm not sure what the procedure is on this: I have a branch which allenap reviewed yesterday and included a few comments, one of which he said was required to fix before landing, but marked his comment "Needs fixing".
<benji> so my question is: should I get it re-reviewed before landing, or just mark it "Approved" myself?
<benji> (https://code.edge.launchpad.net/~benji/launchpad/wadl-refactoring/+merge/36452 is the MP in question)
<bac> benji: generally "needs fixing" means it should be looked at again before landing
<bac> benji: the alternative is for a reviewer to suggest some (generally trivial) changes but mark it 'approved' the first go round.
<benji> bac: in that case: tag, you're it
<bac> benji: gladly
<bac> hey benji
<bac> 284        self.assertIsInstance(matcher.match("foo"), DoesNotContain)
<bac> can you swap those so it is expected, actual
<bac> import _pythonpath # not lint, actually needed
<benji> sure
<bac> Capitalize and add a period to the comment so that it is a complete sentence
<bac> 20from lp.testing.matchers import (
<bac>  21    StartsWith,
<bac>  22    Contains,
<bac>  23    )
<bac> alphamotize
 * bac hmm i guess i should add these to the MP
<benji> bac: actually, assertIsInstance won't work like that; it's defined as taking (obj, class)
<bac> benji: ok, nm then.  :)
<benji> the other two changes are made and as soon as the tests finish will be pushed
<bac> benji: i snuck in a third...  :)
<bac> but i'm done
<benji> k :)
<sinzui> bac: I have a fix for my branch. I am unhappy with it. I now have a good understanding why fields do not notify change events.
<bac> sinzui: well if you're unhappy with it, i'm unhappy with it.
<sinzui> I will clean up my work and explain my sour victory
<bac> ok
* bryceh changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: - || queue: [bryceh] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac, got time for another in the queue?
<jcsackett> https://code.edge.launchpad.net/~jcsackett/launchpad/private-team-to-private-team-519306/+merge/36589
<jcsackett> (it's quite small)
* jcsackett changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: - || queue: [bryceh, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: sure
<jcsackett> thanks, bac.
<bac> jcsackett: how about using httplib.BAD_REQUEST instead of a magic number, albeit a universally recognized magic number?
* sinzui changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: - || queue: [bryceh, jcsackett, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> bac: I put myself back in the queue for a re-review
<bac> sinzui: ok
<jcsackett> in the webservice_error(), bac? i'd be good with that.
<bac> jcsackett: yeah.  well, s/400/.../ in your files
<jcsackett> dig. i can certainly do that.
<bac> jcsackett: r=bac
<bac> with three little requests
<jcsackett> all good ones, bac. i'll take care of it. thanks!
<bac> hey bryceh i didn't see you'd snuck a review into the queue.  pinging the reviewer will get you a faster response.
<bac> jcsackett: do you have another or was it just the one?
<jcsackett> that's it.
* bac changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: bryceh || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bryceh> bac, sorry, you'd sounded busy
* bac changed the topic of #launchpad-reviews to:  On call: bac || Reviewing: sinzui || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui: originally you expected the notify to just happen automatically when the attribute changed and that's just not how it works?
<sinzui> bac: there is no notification. none, nada
<bac> sinzui: but that is because you/we misunderstood the mechanism or is something busted?
<sinzui> I misunderstood
<bac> ok.  i encounter the notify mechanism so infrequently that i don't have a good grasp of it
<sinzui> I was mislead by the existing subscriber to product change events. I assumed that since there was one and I could see no explicit calls for it, that field did the notify for us
<sinzui> I should delete the pretend subscriber
<bac> i think the change you have now is very good.  i've noted two requests in the MP
 * sinzui makes changes
<bdmurray> sinzui: my +subscriptions has been updated and is ready for a rereview - https://code.edge.launchpad.net/~brian-murray/launchpad/limited-subscriptions-page/+merge/35177
 * sinzui looks
<sinzui> bdmurray, I just had a very bad experience as sample person. I looked as my +subscriptions and decided to remove the blackhole one.
<sinzui> bdmurray, https://bugs.launchpad.dev/ubuntu/hoary/+bug/2/+subscribe does not let me unsubscribe, and cancel takes me to a page I have not seen :(
<bdmurray> sinzui: looking into it thanks
<bdmurray> sinzui: okay, I can see the unsubscribe issue you've described but cancel works for me
<sinzui> I think you watned to use checkboxes, https://bugs.launchpad.dev/jokosher/+bug/14/+subscribe is also wrong
<_mup_> Bug #14: There is no link to the bugtrackers config page <Launchpad Bugs:Fix Released> <https://launchpad.net/bugs/14>
<sinzui> no cancel is also broken on that page, I see the bug page instead of the +subscriptions page
<bdmurray> and I pushed revision 11483 right?
<bdmurray> sinzui: so bug 2 shouldn't have shown up in the result set as sample person is not a direct subscriber of that bug.  I've fixed the query in the view.
<sinzui> bdmurray, I think the issue is that I am seeing radio button, but the widget should be checkboxes.
<bdmurray> sinzui: I'm not sure how this is related to the work I've done.  I've only added ReturnToReferrerMixin to the +subscribe page
<sinzui> no, you added it to a base class, I see in the MP class PersonSubscriptionsView(BugTaskSearchListingView):
<sinzui> you did no write a story, and that is why it is surprising that this is broken. Let me attempt to write a real story and we can set the bar for minimum functionality
