#launchpad-reviews 2010-09-06
<StevenK> thumper: Hey, can you look at db-add-ifp-job again, when you have a tick?
<thumper> StevenK: ok
<jtv> mwhudson: maybe you can review this urgent but near-trivial fix?  https://code.edge.launchpad.net/~jtv/launchpad/bug-631259/+merge/34651
<jtv> (Some would call it trivial but the term is overused)
<jtv> mwhudson: you move quietly but surely; hadn't noticed your approval.  Thanks!
<jtv> I'm just adding a test actually.  :)
<mwhudson> jtv: heh
<mwhudson> yeah
<mwhudson> oops
<jtv> I don't think it matters much since I just restored something that's been in production for years, and the tests have to pass since we just removed everything that relies on this field, but it looks more proper
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<henninge> jtv: I have to admit I was not really familiar with the fact that UPDATEs could be fed their values from SELECTs. cool
<henninge> jtv: What happens if a SELECT returns more than one row?
<jtv> There shouldn't be any SELECTsâ¦ did I make a mistake?
<jtv> Nope, there are no SELECTs.
<henninge> jtv: sorry, I am talking about anything after FROM ...
<henninge> jtv: like the second query (for ideal cases) where the condition could be true for more than one POFILE.id (and it's not unlikely I think.)
<jtv> henninge: picks an arbitrary one.
<henninge> Ah, good for us ;_)
<jtv> May even do multiple updates of the same record, but frankly I just don't care.  The result is still the same, apart from performance.
<henninge> jtv: I know we don't care, I was more worried about it breaking.
<henninge> the query, I mean
<jtv> It'll workâI did it this way to keep the query simple to reduce risk of mistakes.
<henninge> jtv: I think you should leave the condition explicitly in the last two queries.
<jtv> henninge: I don't understand
<henninge> you know, POTemplate.iscurrent IS NOT TRUE and TranslationTemplateItem.sequence = 0
<henninge> jtv: I am just thinking that will make it more transparent to others that will read these.
<jtv> henninge: that only introduces more complexity and so more danger of mistakes.  The condition "TranslationMessage.pofile" takes care of it.
<henninge> jtv: ok, it was just a thought.
<henninge> jtv: r=me
<jtv> henninge: great, thanks.
<jtv> Now to get my branch in somehow.  :-(
<henninge_> jtv: as you can tell, I did not do anything about it yet ...
<henninge_> but I heard your thanks.
<jtv> henninge_: I was thanking you for the review of my queries
* henninge_ changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<lifeless> henninge_: hi
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/private-librarian/+merge/31020
<henninge> lifeless: I am sorry but I will have to refuse this one.
<lifeless> henninge: ok
<lifeless> henninge: may I ask why ?
<henninge> lifeless: besides it being oversized it would take me all day to get into it and I think Stuart can do that much more efficiently.
<lifeless> fair enough
<henninge> sorry
<lifeless> thats fine
<lifeless> stub: ping
<stub> lifeless: pong
<lifeless> stub: the librarian rework is ready for another review
<stub> k
<lifeless> stub: code & db & all
<lifeless> stub: I've just pushed up 9588 making the hostname check a little cleaner and fixing the build.
<stub> k
<stub> There are still changes in session.sql that should be in launchpad_session.sql
<stub> This is all Launchpad specific stuff
<lifeless> I must have missed that
<lifeless> moving around
<lifeless> pushing
<stub> Putting session_store() in sqlbase is naughty cause it should really be in lp.*mumble*. But I do that too because foundations has never really been clear on where its stuff should live.
<lifeless> I figured consolidated it into a function anywhere was better than making another callsite with the full spelling.
<stub> re: the comment in MixedFileAliasView, you might want to consider opening bugs on the tech-debt work (remove the old code one we are sure things work with the feature flag on, consolidate it into LibraryFileAliasView). The bug can then be referenced as an XXX:
<stub> Your XXX just below that is naughty as it isn't referencing a bug. If we need to fix it, open a bug. If not, it isn't an XXX but a comment.
<stub> contecxts is not spelled that way
<stub> I think assert is fine over 404, as the view should never be called for deleted files and we want bad links like that to show up on OOPS reports.
<lifeless> I'll use 395960 for the xx bugs here
<lifeless> all the same thing, its not done till its done
<lifeless> the xxx policy of needing a bug, is, IMCO a bit noddy, still, linked it to 395960
<lifeless> re the assert
<lifeless> if someone opens a page with an attachment
<lifeless> and someone else deletes the attachment
<lifeless> then the user clicks on the link
<lifeless> we'll trigger an assert for a normal situation; that makes no sense to me.
<lifeless> I've put that usecase in.
<stub> line 239 of the diff is engrish
<lifeless> +from hashlib import md5?
<stub> +    LibraryFileAlias directly is OK as at that point the status of the file239+    is know.
 * stub reloads the diff
<stub> ProxiedLibraryFileAlias should be rolled into LibraryFileAlias eventually?
<lifeless> I think it should be renamed
<lifeless> LibraryFileAliasReference
<lifeless> not sure about roll-in or not
<lifeless> Proxied... dates to sept last year
<stub> lifeless: Your database patch uses 'timestamp with time zone' but your storm class is using UtcDateTime. I think your DB patch should be 'timestamp without time zone' to match the storm class and our existing timestamp columns.
<stub> I don't think Storm uses notNull in the column definitions? IIRC that is SQLObject specific.
<stub> lifeless: I don't understand why a 16 character hex string is preferable to str(random.random())
<lifeless> stub: running make schema with the tz change
<stub> If you want to make them less guessable, md5(baseline + str(time.time()) + url + thread.get_ident())
<stub> I suspect the store.commit() is unnecessary as I think that connection is running in autocommit mode. I haven't double checked though.
<lifeless> stub: it seemed nicer
<stub> So the only purpose it serves it to issue a redundant call to the database.
<lifeless> the store.commit won't hit the db will it? no changes - noop ?
<stub> I guess the alternative is flush
<stub> lifeless: Hmm... I'm not sure. I guess Storm will know its a noop and noop it.
<lifeless> anyhow, its there as belt and bracers, can definitely take it out
<stub> lifeless: Any reason we are only using multidomain stuff for restricted files?
<lifeless> yes
<lifeless> dns lookups
<lifeless> librarian files are used for branding images
<lifeless> we don't want to cause dns lookups *and* SSL handshaking for every one of those
<stub> ok. I was thinking it might be desirable to trick browsers to load things in parallel, like team emblems.
<lifeless> three-way handshake + dns will swamp the benefits there
<lifeless> what we should do is have a spite generator sort of thing
<lifeless> we can revisit in future obviously
<stub> What happens now if a test module doesn't have a test_suite() function?
<lifeless> the tests are loaded via TestLoader().loadTestsFromModule
<jtv> henninge: could you review this testfix branch? https://code.edge.launchpad.net/~jtv/launchpad/testfix/+merge/34663
<lifeless> i.e. it DTRT if the test_suite was just doing loadTestsFromName
<stub> Ok. I think I added test_suite to the template not to long ago :)
<lifeless> :P
<jtv> henninge: the diff has appeared
<henninge> jtv: yes, I am on it
<jtv> thanks
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<stub> lifeless: So +    def getAlias(self, aliasid, token, path): accepts path as a parameter, and then proceeds to pretend it is a URL.
<stub> lifeless: I think you need to make your mind up and change the parameter to URL and tweak the documentation so it matches reality.
<lifeless> its a path only as per your previous comments
<lifeless> I will update the table definition
<stub> Imports on diff line 1407 are badly formatted (layers.py)
<stub> Annoying that Librarian stuff has spread from just canonical.librarian to canonical.launchpad.database.librarian and other files
<stub> lifeless: But done. Nothing major I can see.
<lifeless> thanks
<jtv> henninge: any progress with my branch?
<henninge> jtv: sorry, distracted.
<jtv> henninge: not a good thing while reviewing a testfix!  :)
<henninge> jtv: I was just looking closer at one thing.
<henninge> sorry
<henninge> jtv: r=me
<jtv> thanks
<jtv> It's in pqm.  I'm taking a break for urgent non-work stuff.
<jtv> gah
<jtv> gmb, I'm not sure but I _think_ the PQM error message says I need RC to land this testfix: https://code.edge.launchpad.net/~jtv/launchpad/testfix/+merge/34663
<gmb> jtv, Landing it on devel, perchance?
<jtv> gmb: yes
<jtv> that's the build it fixes
<gmb> jtv, Right. Yes, you need RC. And you have it.
<gmb> :)
<jtv> gmb: many thanks!
<jtv> â¦on the way.
<gmb> #\0/
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
* lindbohm.freenode.net 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 || gmb is RM; for RC reviews please add him as a reviewer
* henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
* henninge 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 || gmb is RM; for RC reviews please add him as a reviewer
<gary_poster> hm.  no-one is here to review my poor little MP https://code.edge.launchpad.net/~gary/launchpad/bug627442/+merge/34701 :-/
<lifeless> looking
<gary_poster> thank yoU!
<lifeless> gary_poster: needs an rc stamp
<lifeless> also I need to sit on thumper and convince him to ok the use of queues.
<gary_poster> lifeless, thanks.  will ping gmb.
<lifeless> if he's not around, any other TL will do
<lifeless> or we can get someone else to do the code review and I'll make my stamp a release-critical
<lifeless> (lp only allows one type of review per reviewer at the moment, ce la BUG
<thumper> lifeless, gary_poster: it is ok for someone to give an rc and a review
<thumper> it is just the landing script that is not smart enough
<lifeless> thumper: it would be nice if lp could store it though
<lifeless> gary_poster: hi
<gary_poster> lifeless, hi
<lifeless> I've fixed the remaining test failure in my librarian MP, and need the commits-for-that to be reviewed ;)
<gary_poster> heh, ok, where do I look?
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/private-librarian/+merge/31020 is the MP
<lifeless> I'll get you specific commit links in a sec
<gary_poster> k thanks
<lifeless> (lp is just scanning it)
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9594 was abels fix to a test script I've reviewed this, so strictly it doesn't need more eyeballs. OTOH eyeballs are valuable ;)
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9595 I found a missing/unclear thing in feature flags which I just tweaked.
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9596
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9597
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9598
<lifeless> they are all tiny, just split up for clarity
<lifeless> however, loggerhead looks to be on the way down
<lifeless> so perhaps just pull lp:~lifeless/launchpad/private-librarian and do 'bzr log -p | less and look at the top 4 patches
<lifeless> gary_poster: ^
<gary_poster> ack, looking
<lifeless> thanks
<gary_poster> lifeless, IIUC, the current thought is that we want to make sure that .annotations is reliably on every request Launchpad code sees, and then we can do away with other thread locals that are compensating for that lack.  Right?  If so, +1
<lifeless> gary_poster: yes
<gary_poster> cool
<lifeless> gary_poster: we need to extend the concept of annotatable things to scripts
<lifeless> gary_poster: thats the sticking pointy bit
<gary_poster> scripts, or the requests that scripts use?
<lifeless> well, scripts don't used requests today.
<gary_poster> "interactions"
<gary_poster> no, "participations"
<lifeless> yes, participation is the leverage point we should use to sort this mess out
<gary_poster> cool, agreed
<lifeless> right now scripts with with a special security policy
<lifeless> and a single noddy participation
<gary_poster> heh
<lifeless> what they need to do is switch participations when doing work-on-behalf-of-something-else
<gary_poster> absolutely
<lifeless> the special security policy says 'anything goes'
<gary_poster> ...is that special security policy desirable, or legacy?  Would a more careful policy buy us something worthwhile?
<lifeless> legasheee
<lifeless> basically, it went something like this
<lifeless> early scripts used different domain objects - e.g. sql
<lifeless> that was bad
<lifeless> so bits of zope were stubbed up enough to use the same sqlobject objects
<lifeless> 'zopeless'
<lifeless> that had issues
<lifeless> so more was included
<lifeless> 'parseZCMLForScripts' or whatever
<lifeless> anyhow, its all grown organically.
<gary_poster> Gotcha.  yeah, t happens.
<gary_poster> from a review perspective, is any other test code currently doing the dance that you do in librarian.txt to work around bug 631884?  If so, should it be a test helper?  If not, never mind.
<_mup_> Bug #631884: feature flags get out of sync easily <Launchpad Foundations:New> <https://launchpad.net/bugs/631884>
<lifeless> so we should IMO use the same security framework and policies regardless of context.
<gary_poster> Sounds great to me.
<lifeless> gary_poster: no other code is doing it yet.
<gary_poster> ok
<lifeless> next one along can make it a helper - or we may have the bug fixed to not need it earlier.
<gary_poster> I have a had a card to try to audit the security we have in views and see what we can do to rip it out
<gary_poster> it is a problem
<gary_poster> i have not gotten around to it
<lifeless> what sort of stuff are you referring to there?
<lifeless> things like stashing 'this is ok' directly into the permission cache?
<gary_poster> like, direct calls in views to checkPermission, where it is effectively an actual security protection.  I have rarely been in the situation in which this made sense, but canWrite and canRead are safer when you need it, and I'm pretty sure we are abusing these sorts of things.
<lifeless> ah yes
<lifeless> they seem ugly to me
<gary_poster> This makes our automated security worse than nothing in some cases
<gary_poster> Because it lulls into a false sense of security
<gary_poster> for the webservice for instance
<lifeless> I'd like to get us really using zope permissions as properly as possible.
<gary_poster> but also across domain knowledge, or even across forgetfullness
<lifeless> *if* they aren't sufficient/performant-enough we can talk about what to do, but we can't really assess that while stuff is wedged in every which way
<lifeless> gary_poster: I agree completely
<gary_poster> cool
<gary_poster> back to review ;-)
<gary_poster> lifeless: typo "acces" in lib/canonical/launchpad/database/librarian.py : """Return the token path used for authorising acces to url.""" Everything else looks good.
<lifeless> gary_poster: thanks!
<gary_poster> np
<lifeless> I need a) a TL to release-critical and b) a code review for this 5-liner https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/34709
<lifeless> thumper: mwhudson: I pick thee ^
<thumper> lifeless: done
<thumper> rc anyway
<thumper> you could submit with rc and r = me if you like
 * lifeless does that
<lifeless> damn the torpedoes!
#launchpad-reviews 2010-09-07
<lifeless> anyone got a sec? had ec2 find a new failure in my librarian branch; 1 line code change, 6 line test change (3 in two tests)
<lifeless> http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9600
<lifeless> thumper: ^
<lifeless> thumper: hi ?
<thumper> lifeless: hi
<lifeless> can you eyeball http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9600 please
<thumper> what does self._when_streaming() do?
<thumper> also
<thumper> why do you need to import the entire module ? import lp.services.features
<lifeless> self._when_streaming is a hook point for subclasses, I added it earlier but forgot to call it :(
<lifeless> it lets the subclass do $whatever, in this case add a header.
<lifeless> I could do from lp.services import features if you like
<lifeless> per_thread, the variable that needs to be poked, is a package scope variabe
<lifeless> theres a bug, I'll add an XXX to it
<thumper> you could make a null feature fixture
<lifeless> we should just fix the bug
<lifeless> https://bugs.edge.launchpad.net/launchpad-foundations/+bug/631884
<_mup_> Bug #631884: feature flags get out of sync easily <Launchpad Foundations:New> <https://launchpad.net/bugs/631884>
<thumper> ok
<thumper> do you have an MP?
<lifeless> yes
<thumper> or just landing?
<lifeless> its already review on the MP
<thumper> ok
<thumper> fine then
<lifeless> this is fixing the MP because it found a bug
<lifeless> if you know what I mean
<thumper> I've hit a snag with my lp:project for private code
<thumper> a snag in that it isn't working
<mwhudson> thumper: :(
<mwhudson> thumper: which bit isn't working?
<lifeless> thumper: can we help?
<thumper> mwhudson: only the bit that bzr calls to resolve lp:foo
<thumper> possibly
<thumper> if I go `bzr push bzr+ssh://bazaar.staging.launchpad.net/+branch/wikkid` it worked when I had privacy set and no trunk branch
<thumper> but both push and pull fail with:
<thumper> bzr: ERROR: Invalid url supplied to transport: "lp://staging/wikkid": Wikkid Wiki has no default branch.
<thumper> which surprised me
<thumper> I'm looking for the whole in the test coverage
<thumper> I found the problem
<thumper> the tests were there
<thumper> but not checking for the right thing
<thumper> any one on call?
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/xmlrpc-tests/+merge/34719
<thumper> rollout blocker ^^^
<thumper> done by gmb
* noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<noodles775> Hi henninge! If you'd like to, I've got a branch which will require a UI review at: A following branch will add the remaining non-js functionality (the search bar).
<noodles775> grr
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739
<noodles775> but no pressure, or rush... it can wait until someone else is available if you're busy.
<henninge> noodles775: no, that's ok. I am already looking at it.
<noodles775> Thanks henninge
 * noodles775 takes a break for lunch.
<jelmer> bac: Hi
<bac> hi jelmer
<jelmer> bac: I've (finally!) managed to fix up the tests for the branch I proposed on Friday, and I was wondering if you could have another look. Julian's also done a review this morning - we'd really like to get this in for the current release.
<bac> jelmer: ok.
<bac> jelmer: you have gotten all of the tests to work now?
<jelmer> bac: Yes. You were indeed right that there were some failing; what I was running on dogfood was slightly more hackish (but working) than what was in the branch I had proposed.
<bac> jelmer: i'd like to run the tests.  it'll take about 30 minutes.  that's ok for you?
<jelmer> bac: yeah, np
<bac> jelmer: ok.
<bac> gmb: jelmer's branch is approved and awaits your RC approval:  https://code.edge.launchpad.net/~jelmer/launchpad/506256-remove-popen/+merge/34549
 * bac was going to say "Imprimatur" but couldn't spell it.
<bac> gmb: actually, wait a bit on that.  my testing may have been flawed
 * gmb waits
<bac> gmb: done.
<gmb> bac, Looking now.
<gmb> bac, jelmer : rc=me; please land it on db-devel if you can; we want to close devel for landings today.
<gmb> (Once it's un-broken)
<jelmer> gmb: ok
<jelmer> bac, gmb: Thanks for the reviews.
<noodles775> mars or leonardr: Could either of your review a trivial fix that we're hoping to land in the release? https://code.edge.launchpad.net/~michael.nelson/launchpad/db-611568-no-email-for-commercial-subscriptions/+merge/34757
<leonardr> oh, right, it's tuesday
<leonardr> mars, are you around? it's your turn
<leonardr> noodles775, r=me
<noodles775> Thanks leonardr !
<noodles775> gmb: If you've time, the above MP is an RC.
<gmb> noodles775, Sure, I'll take a look now.
<henninge> noodles775: Hi!
<noodles775> Hiya henninge
<henninge> noodles775: I am sorry, I got busy doing other things ...
<gmb> noodles775, rc=me.
<noodles775> Thanks gmb
<gmb> noodles775, That's bound for db-devel isn't it?
<noodles775> gmb: I've set it for db-devel yes (in case devel is closed).
<henninge> noodles775: http://launchpadlibrarian.net/55136007/distro-series-differences.png
<gmb> noodles775, Brilliant. (it isn't closed but will be soon, hopfully)
<henninge> noodles775: ^ that is what we already have?
<noodles775> henninge: no, that's what that branch adds. It's the first step towards the planned UI.
<henninge> noodles775: so the mockup is about what the planned UI is but it will be done in a future branch or branches?
<henninge> noodles775: and the screenshot is what this review is currently about?
<noodles775> henninge: yes, I need to do it in stages to keep it manageable. I'm not sure if you saw my comment on the MP, regarding whether I should wait until the UI is complete before requesting a review... I think we may need to re-think this with feature flags.
<henninge> oh, missed that ;)
<henninge> noodles775: no, UI reviews should be done as early as possible as they influence the coding
<noodles775> I don't want to build up a huge branch (or pipeline of braches) locally, so landing the UI incrementally seemed sane.
<noodles775> Yep, great.
<henninge> noodles775: what happens if the "latest comment" is longer? Wrapping or truncating?
<noodles775> Currently it would wrap.
<henninge> noodles775: I think I'd prefer truncating ...
<henninge> or wrap once, then truncate if that is not too hard.
<henninge> since there will be an option to expand an entry, the user will have easy access to the full comment.
<noodles775> henninge: yep, I agree (and yes, truncating won't stop wrapping, but it will be better than displaying long comments.)
<henninge> noodles775: and maybe "no signer" should simply be "unknown"
<henninge> noodles775: you know, I wish we had an easy way for "personal demos".
<henninge> just push the branch and let it run.
<henninge> Now I have to branch, make schema, make run to see your branch in action.
<noodles775> henninge: I did try to make it easy with the script ;) (ie. merge;make schema;bin/iharness < sample_data.py)
<noodles775> henninge: I think we can (or at least, could) put ec2 instances up for demo, but not sure its worth it.
<henninge> noodles775: there was a description for that, iirc. Maybe there is already an "ec2" command for that.
 * henninge has to explore that but not now.
<henninge> noodles775: I am just wondering what the package name and versions link to.
<henninge> noodles775: and what does the "-" link to in the first column?
<henninge> it looks blue
<noodles775> Both columns link to the source package publishing histories in the parent/derived series respectively.
<noodles775> henninge: it is part of the same link. The first column uses the name-version as the link text, the second column doesn't repeat the name (as in the mockup, as having a separate column for the name looked strange).
<noodles775> (which is why the column headers are "Warty package"/"Hoary version")
<henninge> yes, that is quite a neat solution
<henninge> I was just considering if having an extra column would really look that bad
<noodles775> Regarding the "no signer", that was just based on the precedent of the PPA packages page, but unknown might be clearer.
<henninge> but this solution is fine, too.
<henninge> noodles775: also, the version numbers seem to go *down* from parent to child or is that just a glitch in your sample data?
<henninge> noodles775: what is a blacklist, btw?
<henninge> Argh!
<noodles775> oh, you mean from the mockup (I was confused, knowing it wasn't mentioned on the screenshot).
<noodles775> ?
<henninge> noodles775: yes, mockup
<henninge> Argh!
<henninge> noodles775: I just had a call and have to run out right now. Terribly sorry.
<henninge> noodles775: Is it OK if I sum up our conversation to a review later?
<noodles775> henninge: of course!
<henninge> Thanks!
<noodles775> henninge: for your summary, the choice of "blacklist" is here https://lists.launchpad.net/launchpad-dev/msg04536.html
<henninge> thanks
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<leonardr> noodles775, are you still in the queue?
<noodles775> leonardr: I am, for a different branch: https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739
<leonardr> noodles775: ok, mars is off today, so i'll take it in a bit
<noodles775> leonardr: thanks!
<noodles775> leonardr: I need to be off, do you mind doing the review via email?
<leonardr> noodles775, sure
<henninge> rockstar: ping
<rockstar> henninge, pong
<henninge> rockstar: Hi! ;-)
<rockstar> henninge, how's things?
<henninge> very good, thanks. I just did a UI review.
<henninge> https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739
<rockstar> henninge, great, I'll take a look.
<rockstar> henninge, instead of "no signer" or "unknown" perhaps we shouldn't show it at all.
<henninge> rockstar: I thought the "when" information was still of interest?
<henninge> rockstar: maybe the column should be called "Last uploaded" and the content "18 minutes ago by Foo Bar"
<rockstar> henninge, I was just about to suggest that exact thing.
<henninge> or just "18 minutes ago"
<henninge> do we have similar columns elsewhere on LP?
<rockstar> henninge, basically, if the user can't do anything about it, we shouldn't show "unknown" because the user may think they're doing something wrong.
 * rockstar JUST learned that in his UX class.
<henninge> good point
<henninge> ;)
<rockstar> henninge, we have similar date columns, and I think we could/should adopt a similar pattern to what we're proposing here.
<henninge> sounds good
<rockstar> henninge, great.
<henninge> rockstar: Also, I begin to wonder if the Name of the commenter should not be linkified, too, like the uploader.
<henninge> but that would put more stuff in the comment colmun.
<rockstar> henninge, yeah, and we should use a full display name.
<rockstar> henninge, if we still limit it to two lines, that would still be okay.
<henninge> rockstar: did you noticed that the name of the currently logged-in user (top right) is not the full name anymore?
<henninge> I wonder why that was changed.
<henninge> rockstar: for the comment column: I actually think it should be like the upload column:
<rockstar> henninge, because full names aren't unique.
<henninge> ah
<henninge> "18 minutes ago by Mark Shuttleworth: I am working on this"
<rockstar> henninge, yup. I think that's a good idea.
<henninge> rockstar: are you going to add those suggestions to the MP or shall I do it?
<rockstar> henninge, you could just put our whole discussion from IRC copy-n-pasted into a comment there.
<henninge> rockstar: done
<gary_poster> leonardr: up for a short but critical review?  https://code.edge.launchpad.net/~gary/launchpad/bug632218/+merge/34785 .  The description is a bit bare bones, but I'm hoping the linked bug report (bug 632218) gives you an idea of what is going on and what I set out to do.
<_mup_> Bug #632218: staging builddmaster is having building woes with permissions - template engine <canonical-losa-lp> <Launchpad Foundations:Triaged by gary> <https://launchpad.net/bugs/632218>
<leonardr> gary: ok, if it's shorti 'll take it now
<gary_poster> it is
<gary_poster> leonardr: (did you see that? ^^^)
<leonardr> gary: yes, reading the bug now
<gary_poster> cool thank you
<leonardr> gary: so you remove *.pt.py during make clean?
<gary_poster> yes, leonardr
<leonardr> and you give the 'compile' step responsibility for (re-)creating them?
<gary_poster> yes
<leonardr> ok, r=me
<gary_poster> thank you
<bac> leonardr: have time for a modest branch to review?
<leonardr> bac, sure, put it in the queue
<bac> leonardr: not RC
* bac changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [noodles775, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<bac> leonardr: https://code.edge.launchpad.net/~bac/launchpad/bug-237722/+merge/34790
<benji> leonardr: given the queue and the time, I doubt you have time for any more reviewing today, right?
<leonardr> benji: actually the queue is about to be emptied. i can do one more if it's not too long
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<benji> leonardr: it should be pretty short: https://code.edge.launchpad.net/~benji/launchpad/bug-413174/+merge/34794
<benji> it's just using the new lazr.restful.error.expose functionality to kill a couple of OOPSes in LP
<leonardr> cool
<bac> thanks for the nice review leonardr
<leonardr> benji, why put the message as the default in MultipleProductReleases? i'd rather see it at the single raise site
<leonardr> in fact, why have a separate exception that you only use once?
<benji> I'm not married to it.  My reasoning was that there is only one reason to raise that exception (vs. the other exception I added) so I made it the default text.  In other words, put it in the place that matches its generality.
<leonardr> would it be make sense to raise expose(Exception("A milestone can only have...")) ?
<leonardr> or ValueError or something
<benji> re. separate exception: we want to be able to distinguish it from other exceptions that might be raised from that function
<benji> right, I considered ValueError, but the cost of adding a more specific exception seemed lower than the value of being explicit
<leonardr> ok, it's your call
<leonardr> can you refactor get_last_oops_id into a helper module?
<leonardr> benji, on line 246, if for some reason the exception isn't thrown, the test will pass when it should fail
<leonardr> oh, no, nevermind
<leonardr> you tested that immediately before
<leonardr> benji: looks good, just refactor get_last_oops_id
<james_w`> benji: you might like self.assertNoNewOops
<benji> ooh, I might indeed
<james_w`> currently only used in lib/lp/services/job/tests/test_runner.py
<james_w`> plus, TestCases have an oopses attribute that might be useful
<benji> leonardr: in what way do you want me to "[r]efactor get_last_oops_id"?
<leonardr> let me double check, but i think you define the same method twice
<benji> (I think I'll just be removing it.)
<leonardr> ah, yes, just remove it
<benji> leonardr: ah, indeed!
<benji> thanks
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
* leonardr 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 || gmb is RM; for RC reviews please add him as a reviewer
#launchpad-reviews 2010-09-08
* 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 || gmb is RM; for RC reviews please add him as a reviewer
* 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 || gmb is RM; for RC reviews please add him as a reviewer
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<wallyworld_> rockstar: pretty please :-) https://code.edge.launchpad.net/~wallyworld/launchpad/link-bugs-in-merge-proposal/+merge/34826
<wallyworld_> rockstar: and another https://code.edge.launchpad.net/~wallyworld/launchpad/project-cloud-title/+merge/34655
<rockstar> wallyworld_, so, for UI reviews, it's usually customary to screenshot simple things.
<rockstar> wallyworld_, ack
<wallyworld_> rockstar: what's the best way to post the screenshot?
#launchpad-reviews 2010-09-09
<rockstar> wallyworld_, stick it on devpad.
<rockstar> wallyworld_, and then just link to it on the merge proposal.
<wallyworld_> rockstar: thanks
<thumper> people.canonical.com
<wallyworld_> rockstar: links added. ignore the devpad ones. use the people ones
<rockstar> wallyworld_, cool.
<rockstar> wallyworld_, sorry it's taking a bit.  I'm trying to schedule the planting of trees around the currently inclimate weather.
<wallyworld_> rockstar: np at all. thanks for doing the review
* 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 || gmb is RM; for RC reviews please add him as a reviewer
<rockstar> wallyworld_, I'm having a hard time connecting to people.canonical.com...  I thought I would just wait, but it's still not connecting.
<rockstar> Does the link work for you?
<wallyworld_> rockstar: i had timeout issues also but thought it was going to be transient. the links to devpad should still work - I haven't removed the files there yet
<rockstar> wallyworld_, cool.
<rockstar> wallyworld_, yeah, they don't appear to be transient...
<wallyworld_> rockstar: hmmm. i put something there last week and it worked.
<rockstar> wallyworld_, the project cloud one is approved.
<rockstar> wallyworld_, as far as moving the linked bugs, I'm -1 on that one.
<wallyworld_> rockstar: thanks - yes it was a really trivial change
<rockstar> wallyworld_, maybe you and I and thumper could have a call.
<wallyworld_> rockstar: ok. np.
<thumper> rockstar: ?
<rockstar> thumper, I don't like the linked bugs moved up there.
<thumper> rockstar: what is your reasoning
<thumper> rockstar: why
<thumper> ?
<rockstar> thumper, with our current design, we're saying "we have the most important things at the top."  I think that we're putting the linked bugs above things that are much more important.
<wallyworld_> the "bug" report indicated that perhaps the linked bugs section is considered important too
<thumper> rockstar: surely the bugs that are being fixed is pretty important
<thumper> ?
<rockstar> I think, to some extent, putting the most relevant things near the top is a good idea, and I think the linked bugs are relevant, but not more than the others.
<wallyworld_> and i can see why people would think this to be the case
<thumper> don't you think?
<lifeless> rockstar: I think the bulk of the conversation is -much- less interesting than the linked bugs.
<lifeless> rockstar: because the bulk of the conversation just grows and grows.
<rockstar> lifeless, the conversation, maybe.  Not the votes.
<thumper> where is the bugs now?
<lifeless> after the 'add comment' field
<lifeless> which is after the timeline (comments, new commits)
<thumper> I think the bugs should go after the description, but before the comments
<lifeless> that would work for me.
<thumper> lifeless: sorry, I was asking where they were moved to in wallyworld_'s branch
<wallyworld_> thumper:  https://devpad.canonical.com/~ianb/linkedbug.png
<lifeless> thumper: oh, heh :P
<rockstar> thumper, ah, okay.
 * lifeless butts out
<rockstar> thumper, I think they could go right before the comments, absolutely.
<thumper> wallyworld_: try moving them after the description, but before the xomments
<rockstar> lifeless, no, don't ever feel like you can't add your two cents.  I (personally) appreciate your two cents.
<thumper> personally I'd prefer a few dollars chucked my way
<thumper> rather than just 2c
<thumper> which you can't even get in NZ now
<wallyworld_> thumper: ok, can do
<thumper> then new pic :)
<wallyworld_> of course
<wallyworld_> rockstar: i'll ping you tomorrow your time with the ammended layout. thaks for the input
<rockstar> wallyworld_, I'll be around for a bit tonight.  I'm doing homework.
<lifeless> rockstar: thanks! I didn't mean I would stop commenting, just that on this one I've thrown the 2c in; smart folk are looking at it and I feel happy that whatever you guys do will be good... so I'm off to fight other fires.
<wallyworld_> rockstar: ok. you're a glutton for punishment :-)
<lifeless> like why storm is taking 4 seconds on staging to deserialise 2865 specification objects.
<rockstar> wallyworld_, tell me about it.  I'm currently writing a paper on how we can eliminate the mouse from every day computer use.
<rockstar> lifeless, I'd like to talk to you about f2e at some point this week (probably tomorrow)
<thumper> rockstar: replace it with a rat :)
<wallyworld_> rockstar: i want me one of those screens where you can just gesture like in Minority Report I think it was
<lifeless> rockstar: sure
<rockstar> thumper, if only...
<lifeless> rockstar: thats front end somethingorother ?
<rockstar> wallyworld_, yeah, at least then you can eat buffalo wings and still use your computer (my iPad and my favorite foods don't mix)
<rockstar> lifeless, front end engineering.  Basically, I want FASTAR!
<thumper> wallyworld_: if you get a firefox addin called screengrab you can get an image of the entire rendered page
<thumper> wallyworld_: even if it doesn't fit on the screen
<thumper> wallyworld_: very handy for ui reviews
<wallyworld_> thumper: np. ksnapshot does that too but i thought I would just cut the actual changed part to show?
<thumper> wallyworld_: no ksnapshot won't do that
<thumper> wallyworld_: it can't see the parts of firefox that aren't visible
<rockstar> thumper, or you can just type "scr" in gnome-do and it'll bring up the gnome screenshot app.
<thumper> wallyworld_: due to firefox scrolling
<wallyworld_> thumper: my bad. sorry, i misremembered what it could do
<lifeless> rockstar: I want faster too; stats say the backend is the suck today, given that its a necessary condition to fast, I'm focusing there : I'm delighted to help out on fe2 stuff as well, but it is (IMO, based on the data) not a common/dominating issue for us today.
<rockstar> lifeless, yes, but it will be, and it's where I'm really gaining knowledge currently (due to my classes).
<rockstar> lifeless, I'm sure you'd like some company on Performance Tuesday, ent?  :)
<lifeless> rockstar: btw the SSL experiment we designed at the epic should be going ahead soon.
<lifeless> rockstar: absolutely!
<rockstar> lifeless, for instance, if we kept track of the scope of our variables in javascript, we could double (and in some cases, triple) how fast our javascript executes.
<lifeless> rockstar: do we have any automated figures on that ?
<rockstar> lifeless, before I do anything, I'm going to get hudson set up to track it in lazr-js.
<lifeless> nice
<wallyworld_> rockstar: we need to ensure we get the Chuck Norris and Charles Schneider plugins for Hudson :-)
<rockstar> lifeless, if the API gets faster, I think we could offload many of the expensive things on our pages to be on-demand.
<lifeless> rockstar: how does that help?
<lifeless> rockstar: let me rephrase that with some context
<rockstar> lifeless, well, it doesn't help actual performance, but it at least helps perceived performance.
<rockstar> (Both should be goals of ours.)
<lifeless> rockstar: we /should/ be able to deliver a page with 3000 objects in 2 seconds backend time
<rockstar> lifeless, how close are we to achieving that?
<lifeless> rockstar: making things that are /always/ shown loaded separately by the browser adds work to the appservers and decreases efficiency
<lifeless> rockstar: so I have a bit of a kneejerk reaction against it.
<lifeless> rockstar: we're moving on it; I can't put a dial on completion yet.
<lifeless> rockstar: storm performance is 4 seconds on staging in a dedicated test of 3000 rows deserialisation; a profile suggested 10 seconds.
<rockstar> lifeless, well, "always shown" is kinda dynamic, right?
<lifeless> rockstar: we have to fix it for the appservers to fix it for apis because they are the same thing.
<lifeless> rockstar: bug subscribers - always shown, loaded in separate transaction. As an example of it done in a way I don't really like.
<rockstar> I mean, Facebook offloads a lot of its stuff to be done on demand, i.e. when you scroll down halfway, it thinks "oh, the user probably wants to read the newsfeed, so let's give them more of that"
<lifeless> rockstar: *that* sort of on-demand I love.
<lifeless> rockstar: should be able to do it today.
<rockstar> lifeless, okay.  thumper and I had already talked a bit about some things like that.
<lifeless> did you see the google instant announcement ?
<rockstar> lifeless, I've seen some Twitter traffic, but I don't know much more about it.
<lifeless> typeahead searching for their main search engine
<lifeless> refreshing the entire page as you type
<lifeless> the instant bit is that the last character you type generally won't change the search results
<lifeless> so by the time you've finished inputting, you've got a full result
<rockstar> Ah, yeah.  I think YUI's had some new changes to value-change that support similar things.
<wallyworld_> rockstar: linked bug changes made and committed. see my new comment re: horizontal separator. no hurry. thanks.
<rockstar> wallyworld_, looking now.
<wallyworld_> rockstar: thanks. sorry about the delay. i had some bzr (pbkac) issues
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<jtv> oh hi noodles775 :)
<jtv> Just working on modernizing our template builds as well
<noodles775> jtv: Great!
<jtv> Just trying to get makeJob working.
 * noodles775 just merged your latest recife changes before we go offline :)
<jtv> Ah, almost time for that!
<jtv> Thanks.
<jtv> hi henninge
<henninge> hi jtv!
<jtv> noodles775: thanks for the review btwâ¦ I _would_ point you to my buildfarm work now, but Maintenance.  :/
 * noodles775 breathes a sigh of relief ;)
<noodles775> jtv: if you can paste your MP (and a diff), I can do that too :)
<jtv> noodles775: the MP is at https://code.edge.launchpad.net/~jtv/launchpad/translationtemplatesbuild/+merge/34952
<noodles775> jtv: sorry, I meant paste the text of your MP to paste.....com :)
<jtv> noodles775: yes, I'm otp so a little slow sorry :)
<jtv> noodles775: my diff is here: http://paste.ubuntu.com/490815/
<noodles775> jtv: Was that a decision to allow both interfaces for TranslationTemplateBuild via ZCML (rather than having ITranslationTemplatesBuild inherit from IBuildFarmJob - like IPackageBuild does)?
<jtv> noodles775: barely registered.  :)  I tend to avoid interface hierarchies because they can get a little confusing in combination with class hierarchies, but if that's wrong I'll happily change it.
<jtv> (not that zcml can't get a little confusingâ¦)
<noodles775> I'm not sure if its better or worse... I think of it with the question: Is the interface ITranslationTemplatesBuild also an IBuildFarmJob? I'm not fussed either, I was just surprised.
<jtv> We're sort of probing the boundaries between is-a and has-a here, aren't we?
<jtv> I guess in python we're doing inheritance, just not in the dbâin which case perhaps I should change this after all.
<noodles775> jtv: sheesh, it'll be nice to get rid of the BFJOld stuff (and makeJob). A question, in your makeJob() method, you don't add the TTBJ to the store... am I missing something? I'm wondering if you meant to call TTBJ.create() there?
<jtv> noodles775: ah good point, thanks
<jtv> noodles775: ah!  Actually I do add it to the store.
<jtv> noodles775: in terms of db storage, the BranchJob _is_ the TTBJ
<noodles775> jtv: ah, so TTBJ isn't a storm class itself. Right.
<jtv> just one more reason to clean out these stables :)
<noodles775> jtv: so r=me, assuming that you won't be landing this without further branches (ie. to ensure BFJ.getSpecificJob() works for TranslationTemplatesBuild.
<noodles775> Thanks!
<jtv> noodles775: that's actually a lot more than I was hoping for, thanks!  Any other points I need to cover?
<noodles775> jtv: er, you could refactor the code and get rid of IBuildFarmJobOld? ;)
<noodles775> I can't think of any other things... the above will make it self apparent when you look at current builders or builder histories after having dispatched TTBs.
<noodles775> (sorry, where "the above" is ensuring BFJ.getSpecificJob() works for the translation template builds)
<jtv> OK, so I'll move on to that next then.
<jtv> noodles775: since I'm being difficult today, perhaps I can interest you in yet another review?
<jtv> The diff is 342 lines.  It fixes bug 632880.
<noodles775> jtv: if it's non-urgent, StevenK is keen for reviews? Otherwise either myself or jelmer can take it.
<jtv> StevenK: what say you?
<jtv> (it's non-urgent)
<StevenK> jtv: I'll take it, but not right now.
<jtv> Then maybe I'll just wait until I can file a proper MP anyway
<noodles775> jtv: I think you can now? (at least, I've updated your previous one already)
<jtv> oh, good
<noodles775> jtv: can you assign it to StevenK so it doesn't get gobbled up by eager reviewers ;)
<jtv> OK
<jtv> noodles775: just pushing my getSpecificJob branch now.
<jtv> StevenK: I assigned that review to you earlier btw
<jtv> noodles775: you seem to have your "reviewing" state stuck permanently on "jtv" today.  I don't know whether that's optimistic, pessimistic, devoted, or simply practical.  :)  The getSpecificJob branch is here: https://code.edge.launchpad.net/~jtv/launchpad/buildfarmjob-getspecificjob-translationtemplatesbuild/+merge/34965
<noodles775> heh, I just hadn't updated it.
<stub> Four line diff to review if anyone is bored - https://code.edge.launchpad.net/~stub/launchpad/trivial/+merge/34948
<noodles775> stub: done.
<stub> ta
* 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 || gmb is RM; for RC reviews please add him as a reviewer
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer
<allenap> rockstar: Just the man :) Please can you review a very small Tarmac branch? https://code.edge.launchpad.net/~allenap/tarmac/fix-votes-copyright/+merge/34987
<rockstar> allenap, great!
<allenap> rockstar: It's not very interesting though :(
<rockstar> allenap, by the way, thanks for filing those bugs.
<allenap> rockstar: No worries. I'll try and fix one or two of them too :)
<rockstar> allenap, great.
<allenap> rockstar: Thanks :)
<rockstar> allenap, you keep not putting a commit message in!
<allenap> rockstar: Oops, I'll add one.
<rockstar> allenap, too late, I did it already.
<allenap> Ah, you've done it.
<rockstar> :)
<bigjools> rockstar: can I grab you for some help please?
<rockstar> bigjools, absolutely not.  :)
<rockstar> bigjools, what's up?
<bigjools> http://pastebin.ubuntu.com/491088/
<bigjools> test_enabling_enabled_flag is always failing
<bigjools> and I dunno why :/
<bigjools> the assertFalse fails
<rockstar> bigjools, how does it fail?
<bigjools> assertionError
<bigjools> if I test it in launchpad.dev it all works as expected
<bigjools> which means my test is borkenated
<rockstar> does on/off actually work?
<bigjools> yes
<rockstar> Odd.  I would have thought those would be bools.
<bigjools> well, hmm I just lifted it from aonther test
<bigjools> I changed them to True/False and it fails in exactly the same way
<rockstar> bigjools, hm, that's annoying.
<bigjools> lib/lp/soyuz/browser/tests/test_archive_admin_view.py was my inspiration for this test BTW
<rockstar> Do DASs default to enabled?
<bigjools> yes
<bigjools> I did wonder about that
<rockstar> bigjools, so I'm suspecting that somehow, the enabled param isn't making any difference at all.
<bigjools> that's gonna be frustrating if that's the case :/
<rockstar> bigjools, what if, for grins, you manually set self.das.enabled before messing with the form values in initialize_admin_view.
 * bigjools tries to get grins
<bigjools> aaand no difference :/
<bigjools> well - depending on how I set it affects which test fails :)
<rockstar> bigjools, can't you say "self.das.enabled = enabled" right before the "if enabled:"
<rockstar> I suspect that form setting stuff isn't working the way one would expect it to.
<bigjools> I expect the same
<bigjools> so obviously that made it pass
<bigjools> now, wtf is up with the form :/
<rockstar> bigjools, yeah, so what we suspected is correct.
<bigjools> rockstar: I have to finish for the day now, I'll try and grab someone tomorrow to fix it, unless you feel the urge to dive in in my absence
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/deleting-individual-branch-revisions/+merge/34943
 * thumper runs kids to school
<rockstar> thumper, on the phone with abentley
<lifeless> rockstar: hi
<lifeless> rockstar: are you still OCR?
<lifeless> rockstar: if so, https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/35044
<lifeless> mwhudson: thumper: ^ its 6 added lines, no mods
<mwhudson> lifeless: otp
<thumper> lifeless: chasing critical thingy
<thumper> lifeless: but I'll look
<thumper> because I'm awesome
<lifeless> no tests because it would be noddy
<lifeless> like testing that a for loop iterates
<thumper> lifeless: done
<lifeless> thanks
<sinzui> lifeless, ping
<sinzui> lifeless, can you review my word press theme hack: https://code.edge.launchpad.net/~sinzui/launchpad-news-wordpress-theme/escape-from-monkeys/+merge/35046
<sinzui> thumper, ^ maybe you can take a minute to look at the MP.
<lifeless> sinzui: done
<lifeless> sinzui: I know nothing about wp but the method name seems sensible
<sinzui> lifeless, I spent too many hours remembering how to install moin and wp today. the good news is now that I have working instances, I can fix the styles so that behave more like the main sites
<lifeless> o/
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/scanner-permissions/+merge/35048
<mwhudson> thumper: approved
<thumper> mwhudson: thanks
<thumper> OH FFS
#launchpad-reviews 2010-09-10
* 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
<rockstar> wallyworld_, I did a ui review for the link-bugs-in-merge-proposal, but I didn't do a code review.
<wallyworld_> rockstar: thumper did the code review so it should be good. i'm going to double check that he is happy with it before i land it
<wallyworld_> rockstar: the change was just a simple pt file edit
<rockstar> wallyworld_, okay.
<wallyworld_> rockstar: so you don't think we need a light grey horizontal separator between the linked bugs and comments sections?
<rockstar> wallyworld_, not really.
<wallyworld_> rockstar: np. thanks for reviewong
<lifeless> mwhudson: are you on the phone?
<mwhudson> lifeless: no longer
<lifeless> mwhudson: are you @ lunch ?
<mwhudson> lifeless: not yet
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/librarian/+merge/35053
<lifeless> 2 character patch
<lifeless> save hundreds of unnecessary DB calls.
<mwhudson> no diff yet
<lifeless> +++ lib/canonical/launchpad/database/librarian.py       2010-09-10 00:32:25 +0000
<lifeless> @@ -236,7 +236,7 @@
<lifeless>  
<lifeless>      @property
<lifeless>      def deleted(self):
<lifeless> -        return self.content is None
<lifeless> +        return self.contentID is None
<mwhudson> oh right
<lifeless> mwhudson: yeah.
<mwhudson> lifeless: reviewed
<lifeless> a bit of a facepalm moment, I've seen the queries in the logs for weeks and just hadn't stopped to think about it.
<mwhudson> also, yay for orms i guess
<lifeless> ye
<lifeless> yes
<lifeless> and yay for metaprogramming
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/35060
<lifeless> jtv: hi
<jtv> lifeless: hi
 * jtv looks at channel name
<lifeless> jtv: interested in reviewing a small refactoring ?
<jtv> ah, you wish for a review?
<jtv> sure
<lifeless> +2 total lines, no tests (because it just refactors)
<lifeless> jtv: https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/35060
<jtv> lifeless: I see 211 lines, 4 files
<lifeless> yes; measured that way :)
<lifeless> diffstat is what I was referring too :)
<lifeless>  doc/timeout.txt       |    2 -
<lifeless>  webapp/adapter.py     |   85 +++++++++++++++++++++++++++++++-------------------
<lifeless>  webapp/configure.zcml |    2 -
<lifeless>  webapp/servers.py     |   21 ------------
<lifeless>  4 files changed, 56 insertions(+), 54 deletions(-)
<jtv> lifeless: "if no timeouts are enabled, this returns None"âdo you mean if there is no time left, or that this function is useless if no_timeouts == True?
<lifeless> its not useless
<lifeless> it returns None
<lifeless> thats the existing defined behaviour that wasn't previous documented
<lifeless> I can rephrase
<lifeless> perhaps
<lifeless> 'If timeouts are disabled, None is returned.'
<jtv> I think the confusion stems from the new name.
<lifeless> the root cause is that we check for timeouts outside of requests
<lifeless> this is bogus
<lifeless> but we do it because 'request' is only well defined in principle, not well defined in reality.
<jtv> It's bewildering.
<lifeless> same root issue I was referring to the other week, that makes things complex
<jtv> ISTM this function combines two different things: getting the time budget, and checking for timeout.
<lifeless> I'm sneaking up on fixing it by consolidating all the awkward code
<lifeless> jtv: yes, but its less code this way (I tried the alternative)
<jtv> Does this mean we're not going to get any time information where timeouts are disabled?
<lifeless> no, this doesn't affect duration and start checks
<jtv> I hope you shoot the lot of these functions and replace them all with one. :)
<lifeless> I want Request.annotations.start_time and Request.annotations.timeout_time
<jtv> But given the dual role (compute remaining time and check for timeout), maybe this should be called check_* instead of get_*?
<lifeless> and then we can avoid recalculating stuff all the time
<lifeless> jtv: I think either is fine
<jtv> I find it hard to figure out from the docstring what this is meant to do.  Under what circumstances will this return exactly 0 if the time budget has expired?
<lifeless> no_exceptions=True
<jtv> But that's documented as returning a negative number.
<lifeless> oh
<lifeless> stale docs - I found more things to cleanup as I went
<lifeless> let me tweak
<lifeless> just delete that paragraph in your mind
<lifeless> I've just deleted it
<jtv> excuse me while I go buy a keg of beer
<lifeless> lol sure
<jtv> now let's hope I hit the braincells with that particular paragraph in them pretty soon, or this could get ugly
<jtv> lifeless: maybe note in the set_launchpad_default_timeout docstring that this is triggered from the start event or whatever?
<jtv> Wow, this diff has some weird effectsâ¦ is _check_expired left empty but for the docstring?
<jtv> ah, I see that you merely moved set_launchpad_default_timeout
<jtv> hey danilos
<danilos> hey
<jtv> hi henninge!
<jtv> lifeless: approved
<lifeless> jtv: thanks, will add that note
* 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
<lifeless> mwhudson: 8 second time reduction from that LFA patch I did
<bigjools> adeuring: hello!  I have a small branch for your delight.
<bigjools> https://code.edge.launchpad.net/~julian-edwards/launchpad/disable-arch-bug-633139/+merge/35072
<adeuring> bigjools: I'll look
<bigjools> thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: bigjools || 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: adeuring || Reviewing: bigjools || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi adeuring, when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/627957-differences-schema-update/+merge/35073
<noodles775> I'm requesting the db-reviews too.
<adeuring> noodles775: sure
<noodles775> Ta.
<adeuring> bigjools: r=me
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> adeuring: thanks!
<adeuring> noodles775: _updateType() does not return anything. So, "return type_updated or status_updated" in update() looks a bit odd ;)
<noodles775> adeuring: thanks, yeah, it should be returning whether it updated the model.
<adeuring> noodles775: that's worth a test, I think
<noodles775> Indeed... I'd thought I had tested that, but it must have just been testing the return value of the other _updated helper method.
<adeuring> noodles775: the IPropertyCacheManager(ds_diff).clear() in a test look a bit odd: If this is necessary in a test, wouldn't it be necessary in "real life"?
<noodles775> adeuring: It's necessary there because the test creates a new difference and then publishes a new version of the package, before then updating the difference.
<noodles775> so no, it's not something that would happen in the duration of a request.
<adeuring> noodles775: could you add comments with this explanation to the cache.clear() calls?
<noodles775> adeuring: to each one? (I think there are 8... how about at the top of the test case?)
<adeuring> noodles775: yes, that should be enough ;)
<adeuring> noodles775: ok, with these changes, r=me
<bac> good morning adeuring.   just checking in.  i'll be along shortly to help.
<adeuring> bac: morning bac
<noodles775> Thanks adeuring. I was also thinking, it may makes sense to clear the cache at the start of the update() method too, which would remove the need for them in the tests.
<adeuring> noodles775: yes, that would be even better
* jelmer changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: noodles775 || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> adeuring: If you have time to do another soyuz review, there is one at https://code.edge.launchpad.net/~jelmer/launchpad/634045-lp-bugs-fixed/+merge/35096
<adeuring> jelmer: sure
<adeuring> jelmer: r=me, 1 nitpick
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: -775 || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> adeuring: Thanks
<jelmer> adeuring: reviewing -775 ? :-)
<adeuring> argh... thanks for the hint ;)
* 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
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> adeuring: could you review https://code.edge.launchpad.net/~brian-murray/launchpad/display-dupe-in-portlet-dupe-subscribers/+merge/34501?
<adeuring> bdmurray: sure
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bdmurray, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bdmurray, - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738/+merge/35018
<jcsackett> bac, are you a UI reviewer as well?
<bac> jcsackett: i am not
<jcsackett> okay; i'll ping sinzui about it.
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: bdmurray, jcsackett || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * jcsackett sees he's already in the channel.
<jcsackett> sinzui: any chance i could request you for ui review on my unknown blueprints branch? all nitpickery welcome.
<sinzui> jcsackett, I am asking salgado do the first round of UI reviews so that he gets experience. I can follow his review with mine
<jcsackett> ok.
<jcsackett> he's not online now, it looks like; i'll just set him as the requested reviewer.
<bac> yowzer, jcsackett, 1400 lines?
<jcsackett> ...dependent branch maybe not working?
<bac> jcsackett: perhaps
<jcsackett> shouldn't be anywhere near that size...
<jcsackett> ...it's a little bit of code, a test, and a small template...
<bac> it is listed...
<bac> jcsackett: look at the generated diff and tell me if it is all for this branch
<bac> lots of answers stuff in there
<jcsackett> bac: yeah, this all looks like stuff you shouldn't see b/c of the prereq branch.
 * jcsackett snaps fingers.
<jcsackett> one sec, it may be b/c the prereq branch was updated but this one isn't.
<jcsackett> bac: yes, i failed to update this one this morning before pinging you.
<jcsackett> give me one second while i do that.
<bac> thanks
<jcsackett> bac: updated, and i've attached a diff from my machine in case there's still enough divergene post merge (both branches had conflicts, i may not have resolved them identically).
<bac> ok
<jcsackett> bac: it's a much more manageable ~300 lines. :-P
<bac> hurrah
<adeuring> bdmurray: r=me nice work!
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring, bac || Reviewing: -, jcsackett || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: i tried reproducing your branch.  started with fresh branch from devel, merged in your pre-req, and then merged your branch.  got a criss-cross merge with conflicts
<bac> jcsackett: were these branches previously based db-devel?
<jcsackett> bac: no, i've created them with rocketfuel-branch of devel.
<jcsackett> db-devel should never have entered into it.
<bac> hmm.  conflicts were easy to resolve.
<jcsackett> bac: well, that's something.
<jcsackett> criss-cross means there's some weird cyclic merge or something, right?
<bac> jcsackett: i think it means there is a clear ancestor relationship
<bac> P begets A
<bac> A begets B
<bac> P changes and B merges from P
<bac> A merges from P
<bac> B merges from A
<bac> craziness like that
<bac> on paper you can see the relationships but the accounting in the branch gets confused.
<bac> that's my best understanding
<jcsackett> bac; that make sense.
<bac> jcsackett: this is interesting:  if hasattr(self, 'index'):
<jcsackett> bac: yeah, it's actually less interesting and more shaky.
<bac> jcsackett: why does that imply zcml registration?
<bac> jcsackett: if you keep it, you should use safe_hasattr
<bac> jcsackett: pretent i'm using my spock voice when i say "interesting"
<jcsackett> bac: dig. :-P
<bac> jcsackett: so, where did you come up with that?
<jcsackett> bac: experimentation. if you remove the zcml registration of the template in order to use a template defined in the browser class, it loses the index attribute. since the class that's used for the base services page is the also the baseclass for a bunch of other blueprints stuff (portlets &c) it's a way to determine that this is in fact the correct view.
<jcsackett> bac: the other means i tried failed for various reasons. checking if it was a subclass of the browser class fails, b/c the code is within the base class, so it always returns true. same problems for a number of other identifying attributes.
<bac> jcsackett: it may be worth running past gary_poster to see if there is a more obvious way to do the same
<jcsackett> bac: dig.
<gary_poster> backlog-reading...
<jcsackett> the magic of IRC: speak of a person and watch them appear.
<gary_poster> :-)
<gary_poster> so...
<gary_poster> this is using the browser zcml tag?
<bac> gary_poster: we're discussing line 154 of https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738/+merge/35018
<gary_poster> which does the horrible "let's dynamically make a class for the fun of it" stuff?
<gary_poster> ow
<jcsackett> oh good, i've caused physical pain with code.
<gary_poster> :-)
<gary_poster> ok, so let me see if I understand the goal
<gary_poster> this base class is supposed to sometimes be used with a template, and sometimes not
<gary_poster> in all cases, we use that horrible browser:page zcml, or something like that
<gary_poster> if we use browser:page and this base class and a template...
<gary_poster> then this base class will win
<gary_poster> and so you have to go sniff for the original template
<gary_poster> and you are doing that with the index thing because it seems to work :-)
<gary_poster> Correct me if I'm wrong; I'll proceed as if I'm right.
<jcsackett> gary_poster: yes. index works, there's no other argument for it--we're looking for a cleaner solution.
 * jcsackett is not super clear on some zcml stuff yet.
<jcsackett> basically you sound like you've got it.
<gary_poster> Cool.  Don't bother trying to be clear on browser:page is my advice ;-)
<jcsackett> gary_poster: advice i'm happy to take. :-)
<jcsackett> the reason for the template being set in the code (sometimes) is b/c when the base class should win, we want to be able to set the template based on how blueprints is being used.
<jcsackett> but the base class is also used for portlets that those set templates call. so we have to sniff so that we don't have the template calling the portlet, which calls the template, which calls the portlet...&c.
<gary_poster> My fist thought is "don't do that."  I would prefer to see a base class that always expects to control the template.
<gary_poster> I and others generally regard browser: page as a mistake.  You can register views as adapters if you follow another pattern, and then you just use Python to decide what template is supposed to be used where.  You can have a base class for this base class for the legacy zcml browser:page usage.
<gary_poster> ...But that's probably a bigger conversation than should be having right now.  Let me go find that horrible mixin class that browser:page uses and we can see if it is gives us anything usable.
<gary_poster> (a bigger conversation because you are following established LP patterns, so the discussion I am raising has to do with LP, not this review)
<gary_poster> I appear to be feeling crusty and cantankerous this morning :-P :-)
<jcsackett> that's fine. it does feel like this could use some bigger redesigns, but it's definitely out of scope for this. :-)
 * bac eavesdropping and learning
 * gary_poster was called away, but is back :-P
<gary_poster> z3c.ptcompat also makes angels cry, even though it is very useful for switching to chameleon tentatively... :-/
<jcsackett> so, basically, i've sinned against code-god. :-P
<gary_poster> :-) no, not you. The environment in which you find yourself
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || Reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gary_poster> Sorry for taking so long.  I'm working my way through gems like this:
<gary_poster>     if offering is None:
<gary_poster>         offering = sys._getframe(1).f_globals
<gary_poster>     if isinstance(offering, dict):
<gary_poster>         offering = package_home(offering)
<gary_poster>     bases += (simpleviewclass.simple, )
<gary_poster>     class_ = type("SimpleViewClass from %s" % src, bases,
<gary_poster>                   {'index': ViewPageTemplateFile(src, offering),
<gary_poster>                    '__name__': name})
<jcsackett> ow.
<gary_poster> see?  see?  I told you :-)
<jcsackett> heheh.
<gary_poster> ok, jcsackett, bac. What you've done, under the circumstances, is actually one of three reasonable paths.  My first, preferred, path is "don't do it" and have separate base classes for sick horrible things like that mixin code and for normal Python things that work as you'd expect. :-)
<gary_poster> The other remaining paths are to check for index, as you have done, which checks whether the code I copied above slams an index page template on the class, or (I think) to check if the class subclasses zope.app.pagetemplate.simpleviewclass.simple (as you see in the bases line).
<gary_poster> As a reviewer, I personally would have encouraged you to go down path 1, but gently, because you are doing things exactly as the rest of LP does them now.  I really am supposed to be complaining about these sorts of things more loudly to the reviewers meeting or something like that, but I always have other things to do that seem like good ideas at the time.
<gary_poster> So, I would have accepted the "index" check, but requested that you add a comment that the "index" is a  page template added by the browser:page template machinery and usually used by the zope.app.pagetemplate.simpleviewclass.simple class that is magically mixed in by the browser:page zcml directive.
* adeuring 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
<jcsackett> gary_poster: do you think separating things out into separate classes could be tracked in its own bug? i think it's worth doing, but is a little out of scope on this branch (especially since this is part of bridging-the-gap and we really need to be moving on from that).
<gary_poster> jcsackett, pretending that I were your reviewer, I would be happy to be convinced by that argument, but would say that the bug is that we should not be using browser:page.  I then would mutter something about needing to go check my underarms for mites, because, of course, that would be a foundations bug, for which I'm responsible. :-P
 * jcsackett laughs.
<jcsackett> okay, thanks gary_poster.
<gary_poster> np, jcsackett :-)
<bac> jcsackett: i'm fine with the above.
<jcsackett> bac: okay. i'll add the comment explaining what's actually going on with the index, (as well as use safe_hasattr). gary_poster, do you want me to go ahead and file that bug for foundations?
<bac> jcsackett: push up a diff and i'll look at it again.
<gary_poster> jcsackett: yeah, sure.  thanks. :-)
<jcsackett> np.
 * bac lunches
<jcsackett> bac, when you return, changes were pushed.
<bigjools> hey bac!  I've got a delightful branch if you would care to review it.
* bac changed the topic of #launchpad-reviews to: On call: bac || Reviewing: lamont || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> bac, ping, I have a series of three branches for the qa-tagger code I would like to have reviewed.  Would you be willing to look at some of them?
<gary_poster> jcsackett: btw, this is missing the JS, CSS and images to make it pretty, but it is still useful: https://devpad.canonical.com/~gary/
<gary_poster> you can see same in apidoc.launchpad.dev when you run a local instance
<jcsackett> gary_poster: awesome. thanks!
<gary_poster> np :-)
* 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> mars: where are your branches?
<allenap> bac: Thanks for the review earlier. Also, do you mind if I resume a Thursday review slot, alongside noodles?
<bac> allenap: mind?  heck no!
<allenap> bac: :)
<bac> glad to have you back, gavin!
<allenap> Cheers bac, good to be back.
<lifeless> allenap: welcome
* bac 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-12
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/features/+merge/35217] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/features/+merge/35217, https://code.edge.launchpad.net/~lifeless/launchpad/bug-631884/+merge/35219] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/features/+merge/35217, https://code.edge.launchpad.net/~lifeless/launchpad/bug-631884/+merge/35219, https://code.edge.launchpad.net/~lifeless/launchpad/memcache/+merge/35220] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* lifeless changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/features/+merge/35217, https://code.edge.launchpad.net/~lifeless/launchpad/bug-631884/+merge/35219, https://code.edge.launchpad.net/~lifeless/launchpad/memcache/+merge/35220, https://code.edge.launchpad.net/~lifeless/launchpad/test/+merge/35224] || This channel is logged: http://irclogs.ubuntu.com/
<lifeless> morning
<lifeless> I can has review?
 * mwhudson stabs xchat's url-finding 
<mwhudson> lifeless: which is the most urgent of the reviews?
<lifeless> they're all basically tiny; any of them would be great to have reviewed.
<lifeless> test is trivial - bare except -> not bare except
<mwhudson> ah, so the reviews are not related to the fire?
<lifeless> no
<lifeless> I'm just pouncing
<mwhudson> heh
<mwhudson> i need to persuade someone to fix the ath9k drivers, then my arrival in the morning will be less obvious
<lifeless> mwhudson: actually, it was saying 'hi' that tipped me off
<lifeless> thanks for test approval, to make ec2 land happy could you click on 'comment' change to 'approved', click in the text field, click on 'save commnt' ?
<mwhudson> lifeless: i did, and it timed out :-)
<lifeless> hah
<lifeless> yes, waiting for a gsa/losa to appear
<mwhudson> i think it took the second time
<lifeless> elmo is trained
<lifeless> by which I mean, a train is messing with his bandwidth
<mwhudson> ah
<mwhudson> lifeless: in https://code.edge.launchpad.net/~lifeless/launchpad/bug-631884/+merge/35219 i don't really understand the comment
<mwhudson> "features have two homes" -> i don't see how the request home matters here?
<lifeless> if you use LaunchpadTestRequest
<lifeless> and create_initialized_view
<lifeless> and the code you are calling calls into getFeatureFlag
<lifeless> it all goes boom
<lifeless> because nothing glues the two things together.
<lifeless> what needs to happen is:
<lifeless>  - the thread local should die
<lifeless>  - it should be found via participations
<lifeless>  - login() and scripts should set participations up
<lifeless> its another case of building on mud.
<mwhudson> i see
<lifeless> the problem is that there are lots of tests that do stuff outside of publication
<lifeless> or even outside of views
<lifeless> so using that as the place to set/remove the thread local won't work well
<mwhudson> lifeless: would it more appropriate to say something like "feature flags really should live on the request, but currently don't for reasons as stated in bug 623199" ?
<lifeless> I don't konw.
<mwhudson> s/request/participation/ i guess
<lifeless> I think what I've written is correct without specifying /where/ etc : and its easier to change our mind in a bug than in the comments in the code
<mwhudson> otoh, it's very likely that the next person to edit this code will be you and you can figure it out then :)
<lifeless> thats true too
<lifeless> I'm happy to change it, I don't see that it makes a lot of difference either way.
<lifeless> I looked at zope.app.testing.functional.FunctionalTestSetup
<lifeless> zomg
<lifeless> its -terrible-
<mwhudson> lifeless: for the pageid feature thing, i don't see how pageid is ever set to stuff like scoped:thing#type
<lifeless> check out api publication
<mwhudson> i am
<lifeless> servers.py
<lifeless> no, sec
<lifeless> https://code.edge.launchpad.net/~matsubara/launchpad/bug-606184-pageid-for-collections/+merge/33801
<lifeless> is what landed
<lifeless> 'FakeContext:FakeCollectionResourceView:#milestone-page-resource'
<lifeless> is an example
<lifeless> so its :#
<mwhudson> ah
<lifeless> which my code will handle because it elides empty segments
<lifeless> (but I knew there was a good reason)
<lifeless> see also https://bugs.edge.launchpad.net/launchpad-foundations/+bug/627027
<mwhudson> ah ok
<mwhudson> there are no tests of the empty segment handling
<lifeless> yes
<lifeless> I shall add one for that example pageid
<lifeless> actually there are tests of empty segment handling
<lifeless> pageid:
<lifeless> -> ['']
<lifeless> without the empty segment code
<lifeless> e.g. tests will fail if you remove the 'if name' clause
<mwhudson> i guess
<mwhudson> that's not very test as documentation though
<lifeless> yes
<lifeless> I shall add one
<lifeless> just saying :)
<mwhudson> ok
 * mwhudson stares at an ajax spinner thing
<mwhudson> third time lucky, sigh
* mwhudson changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
<lifeless> thanks!
<mwhudson> huh, https://code.edge.launchpad.net/launchpad/+activereviews just got truncated on me
<lifeless> yaup
<mwhudson> huh, googling "truncated haproxy" finds some interesting stuff
<mwhudson> i guess you're ahead of me on that game though :-)
<lifeless> if there were specific ones that would be good
<lifeless> however http/1.0 and truncation is a designed in 'feature' ><
<mwhudson> lifeless: well http://www.mail-archive.com/haproxy@formilux.org/msg02145.html
<lifeless> heh, thats a bug indeed
