#launchpad-reviews 2010-06-07
* henninge changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge: got one for you!  https://code.launchpad.net/~jtv/launchpad/bug-590688/+merge/26929
* henninge changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: jtv || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: I wonder if the method should not be called "switchDbUser" like the Layer function.
<jtv> henninge: I wanted this to be "attractive" and have a name that doesn't say "you probably don't need to know about this."  At the same time, this doesn't just switch the db user but also commits because otherwise the switch doesn't make much sense.  So naming it after one part of the task would be misleading.
<henninge> I had a feeling you'd say something like that ... ;)
<jtv> ah :)
<jtv> Hang on... Network Manager _may_ be trying to tell me the wifi connection is back up (but then again it may just be trying to confuse me)... I'll see if I can get off this gprs.
<henninge> jtv: I am not sure I go a long with your assesment of "attractive" method names but it's not all that important, I guess.
<henninge> jtv: so, r=me
* henninge changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge: thanks.  Suggestions for a better name always welcome, of course.
* abentley changed the topic of #launchpad-reviews to: On Call: henninge || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On Call: henninge, abentley || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mars> gary_poster, when you have a moment, could you please review https://code.launchpad.net/~mars/launchpad/use-zope.testing-3.9.4-p0/+merge/26961 ?
<gary_poster> mars, ack, will be available in 5
<gary_poster> mars, done, thanks
<abentley> rockstar, mumble?
<rockstar> abentley, sure.
<adiroiban> jtv, henninge: I have pushed a MP to start the pre-implementation chat for bug 583934 https://code.edge.launchpad.net/~adiroiban/launchpad/bug-583934/+merge/26965
<mup> Bug #583934: API for POFile attributes <api> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/583934>
<adiroiban> should I add you as reviewers?
<jtv> adiroiban: personally I wouldn't worry about that just nowâit's not a merge proposal as such quite yet.  :)  The main thing is that pre-imp.
<adiroiban> jtv: yes. I just wanted to make sure that you are aware of that branch and to schedule a pre-imp chat based on that branch
<jtv> adiroiban: and that you have achieved.  There's no stopping you, is there?  :-)
<jtv> You're really getting into the tough ones here...  there's also the fact that we're not really keeping up with refreshing the POFile statistics data.
<adiroiban> jtv: yes this branch is more about deciding an API for IRosettaStats ... I have also briefly discussed with Henning during the UDS
<adiroiban> jtv: how come that the POFile statistics are not updated?
<jtv> Cool.  It's an area that could do with some fresh thinking.  It's something I haven't touched that much myself... sort of been hoping that if I don't touch it, it won't bite.
<adiroiban> jtv: I mean the per POFile statistics
<adiroiban> not the aggregated per distroserieslanguage
<jtv> Short answer: message sharing.
<adiroiban> I see
<jtv> Makes it much harder to keep track of exactly which POFiles have been changed.
<adiroiban> so the POFile statistis will not be updated for the shared POFiles?
<jtv> Exactly.  We do refresh all of them in a cron job, sort of as a stopgap,
<adiroiban> ok
<jtv> but with message sharing that's become very inefficient.
<jtv> It's too costly to find and update all the POFiles that may be affected by a translations update, so we can't do that.
<adiroiban> but this should be solved in a new bug/branch
<jtv> Yes.
<jtv> We've already done a bit of research into that.
<adiroiban> this branch is mainly about finding the right names for those statistics attributes
<jtv> Right.  But be aware that this is not a particularly good time to expose them; they'll be pretty badly maintained.
<jtv> What we need is a cron job that figures out which POFiles share with ones that have been updated, and updates the stats for those.
<henninge> jtv: they are exposed anyway through the webui.
<jtv> Yes...  what I mean is that anyone writing clients based on this would have to be acutely aware that the stats aren't particularly reliable.
<adiroiban> yes. No new data will be exposed in the current API and in fact the whole Translations Statistics API work is to replace the web crawling of those data
<henninge> what I meant.
<adiroiban> jtv: I assume that in the next year only I and dpm will use the API and we will use them for weekly or monthly reporting
<adiroiban> one day delay will not be much of a problem
<jtv> Personally I'd prefer clear documentation on the point to an assumption that nobody will use this.  :-)
<adiroiban> in this state, the Translation API is to scarce to be used in a real application
<adiroiban> that could be affected by those cronjobs
<bac> henninge, abentley: may i get a review for https://code.edge.launchpad.net/~bac/launchpad/bug-588773/+merge/26968 ?
<adiroiban> but if you think that those cronjobs are a blocker, we can suspend the for on Translations Statistics API
<adiroiban> and try to first solve those problems
<henninge> adiroiban: well, that would be better for sure ... ;-)
<adiroiban> :)
<abentley> bac, I can have a look when I'm back from lunch.
<bac> abentley: that's perfect as it allows me to go eat too!
* henninge changed the topic of #launchpad-reviews to: On Call: abentley || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> henninge, jtv : I think that the work for this branch is somehow independent of the one day delay in statistics. With or without real time statistiscs we still have to decide for what statistics attributes to expose via the API
<jtv> adiroiban: very true
<henninge> adiroiban: it is one week delay, btw, is it not, jtv?
<henninge> up to one week
<jtv> Not quite that simple
<jtv> In theory, we run one full statistics update job on all POFiles per week, and a smaller job on just the more recent POFiles per day.
<jtv> Those runs have started to take much, much longer
<jtv> And they share a lock.
<jtv> This all needs serious work.
<adiroiban> But since most of the translation work is done on the lates Ubuntu series, at least Ubuntu Lucid statistics are preety accurate
<adiroiban> I did not encouter any error in Romanian Lucid translation statistics
<adiroiban> jtv: is there a bug report for that?
 * jtv looks
<adiroiban> bug 373269 ?
<mup> Bug #373269: Message sharing and POFile statistics <message-sharing> <Launchpad Translations:Triaged> <https://launchpad.net/bugs/373269>
<jtv> Yes
<jtv> Frankly I'm not even sure we still run the daily updates, since I'm not getting output for them, nor how long the weekly runs take.
<jtv> The daily partial runs were always meant as an interim measure; at that time it covered all the POFiles that did any sharing at all.
<henninge> jtv: I am pretty sure the daily runs are disabled.
 * henninge pulls the crontabs
<henninge> yup, disabled
<jtv> The daily runs don't make as much sense now that we, but neither do we have anything to replace them.
<jtv> s/now that we/now that we have been migrating existing POFiles to message sharing/
<adiroiban> jtv, henninge: ok. so back to POFile API branch. are we going to halt the work on Translation Statistics until this bug is fixed? or how do you suggest we should proced with this work?
<henninge> adiroiban: no, I don't think we should do that. As you pointed out, they don't really depend on each other.
<jtv> Independently from all this, I think it's still useful to figure out an appropriate structure for the statistics.
<jtv> The work we're doing towards sharing between ubuntu and projects is going to affect the design.
<henninge> You mean the design of the API?
<jtv> Yes.  For starters, the meaning of what is now current/imported changes a bit.
<henninge> true
<jtv> In fact, it may even become simpler.
<henninge> yes, should
<adiroiban> jtv, henninge: in the current branch lib/lp/translations/interfaces/statistics.py
<adiroiban> there are the names and docstring for the exported attributes
<jtv> Arguably the only sensible way to look at the distinction is now to keep completely separate views of the statistics per POFile.
<adiroiban> and they are not using current/imported terminology
<jtv> current/imported is more the technical underpinnings of what you see there.
<jtv> For instance, rosettaCount counts translationmessages that are "current" but not "imported."
<jtv> Message sharing complicated that, but the combination with the Ubunbu/projects sharing makes it even more treacherous.
<adiroiban> yep, I had to digg a little to understand rosettaCount and updatesCount
<jtv> And so arguably the only sensible way to look at it is to make it simpler.
<adiroiban> jtv: ok
<adiroiban> jtv: this branch should be about finding a name and a docstring for those simple attributes
<adiroiban> and it should not care about the internal implementation
<jtv> My expectation right now is that rosettaCount and newCount would just not exist in the future.
<adiroiban> and the MP details I have added the list of exported attributes
<adiroiban> jtv: I can understand why rosettaCount should not be exported
<adiroiban> by why we should not export newCount() ?
<jtv> I'm not just saying they shouldn't be exported, but that I feel they won't exist.
<adiroiban> ok
<adiroiban> but still, API users will want to know the ammount of new translations in Launchpad
<jtv> newCount counts POTMsgSets that have a "current" TM but no "imported' TM
<adiroiban> how is this going to be implemented in the future if newCount will no longer exist
<jtv> I think that would have to be lifted out of POFile, to a slightly higher level of abstraction.
<jtv> It'd become a kind of difference operation between two POFiles.
<adiroiban> jtv: ok. then we can no longer export those statistics as POFile attributes
<adiroiban> but have a method that returns them
<henninge> jtv: why do you think that?
<adiroiban> and so POFile will no longer have to implement all those individual statistics attributes
<jtv> henninge: because of divergence.
<jtv> henninge: a TM can be upstream-diverged or it can be ubuntu-diverged, but it can't be both because in one case it diverges by being specific to an upstream template, and in the other it diverges by being specific to an ubuntu template.
<jtv> So there's no single POFile that you can query to compare the ubuntu translation state to the upstream translation state.  You need to compare an ubuntu pofile to an upstream pofile.
<jtv> That's not saying we can't have a fixed coupling between an ubuntu pofile and an upstream pofile, of course.
<henninge> jtv: I am not sure I follow you
<henninge> but, it's too late for me to think myself into that ...
 * henninge know about jtv's time zon
<henninge> e
<henninge> ;-)
<jtv> heh... midnight already
<henninge> adiroiban, jtv: I have to go, sorry.
<jtv> anyway, I just mean that if we want to figure out how many messages in an ubuntu pofile differ from upstream, we can no longer look at just that pofile's messagesâbecause any upstream-diverged messages will be in a different pofile from any ubuntu-diverged ones.
<jtv> OK, but glad we had some opportunity to start thinking about this.  adiroiban, do you need a quick update on what's happening on this front?
<henninge> seriously, I don't understand the consequence of that. I will have to draw a picture ... ;)
<jtv> pictures help!
<henninge> but not today anymore ...
<jtv> exactly.  :)
<adiroiban> jtv: so upstream-diverged statistics will no longer be cached?
<jtv> They would be, but they are something that's recognizable in the upstream pofile, not in the ubuntu one.
<adiroiban> ok. but we will continue to represent the upstream-diverged value in the web browser, together with other attributes specific to downstream file... or no?
<jtv> For purposes of the statistics, I expect we'll still want to follow the usual divergence/sharing protocol: "if this POTMsgSet has a diverged current TM specific to this POTemplate, that's the current TM.  Otherwise, look for a shared current TM."
<jtv> adiroiban: I think that would make sense simply because it has value in the process we want to support, but I don't know if POFile will remain the right place to keep that information.
<henninge> bye guys
<adiroiban> jtv: and where it will be stored?
<adiroiban> from an API user point of view, I will like to just get a POFile and have all its statistics as attributes
<adiroiban> or as a POFile method
<jtv> adiroiban: give it timeâI'm off duty and I've been looking at this for less than an hour, with a Simpsons episode on the side.  :-)
<adiroiban> :)
<adiroiban> ok
<jtv> What I'm saying is that the distinction we currently have of upstream and downstream within one pofile is going away.
<adiroiban> then can you please add your comments on the MP so that we can move forward... not now. when you will have time
<jtv> With pleasure.  I'd want to sleep on all of this.  What I'm trying to do for now is highlight what changes will affect the statistics we keep.
<adiroiban> yes. basicaly this is what we need to discuss in the pre-imp phase :)
<jtv> In the future, when you're looking at one template in one language, there will be one view for each ubuntu release series it's in, as well as one for each project release series.
<adiroiban> if statistics are going to be changes
<jtv> (I'm using "view" as "way of looking at things" here, not anything formal)
<jtv> Each of these views is basically a pofile.
<jtv> Some of these pofiles will be upstream ones, others will be ubuntu ones.
<jtv> They will all share most of their translationmessages.
<adiroiban> we can create the API to handle the new way of viewing things :)
<jtv> Sure.
<jtv> The big change is in what happens when upstream messages differ from ubuntu ones.
<jtv> No longer will the ubuntu one contain a "dual view" of upstream ("published") and downstream messages.
<jtv> Each just contains its own active translations.  Most will be shared; others will be specific to that pofile.
<jtv> So when you look at an ubuntu pofile and ask "how does this differ from upstream?" you need to compare to a matching upstream pofile.
<jtv> If we make sure we have such a matching upstream pofile for each ubuntu pofile, that makes things relatively simple, because we'll always know what to compare.
<adiroiban> but the API end user will only ask for how_does_this_differ_from_upstream(void).
<adiroiban> or the API user will have to first identify the upstream template and then ask for how_does_this_differ_from_upstream(upstream_template)
<jtv> Right.  So that's an open design question depending on how the ongoing ubuntu/project sharing work comes out.
<jtv> adiroiban: btw your branch has landed and passed buildbot.
<adiroiban> ok. then we should first see how ubuntu/project sharing work will be done and then come back to upstream_changed statistics attribute
<adiroiban> what about the other attribtes listed in the MP ?
<jtv> adiroiban: last update and last translator have become a bit shaky and the way we keep track of them may have to change, but that's a generic sharing issue.
<jtv> As a simple example, if you translate an untranslated message in Maverick and the same message was also present in Lucid, that should probably also count as an update to the Lucid translation as well.
<jtv> Again that probably doesn't speak against aggregating and caching the information in the pofile.
<adiroiban> jtv: yes, and how does this affect the API ?
<adiroiban> or the API design
<jtv> I hope it won't affect the API; all we know for now is that something there needs to change.
<jtv> Probably just implementation, but I'd be amiss not to mention it.
<adiroiban> :)
<adiroiban> ok
<jtv> Don't see any risks with the rest.
<jtv> By the way, it may not hold true that (length - translated messages) == untranslated messages.
<jtv> So depending on your needs, you may want to include a count of untranslated messages.
<jtv> (think translation credits)
<adiroiban> but (lenght - untransalated) == translated ?
<adiroiban> of this is also not true :)
<jtv> All depends on the definitions I guess :)
<adiroiban> I was thinking to keep the API simple
<adiroiban> but if you think it is needed
<adiroiban> I can add an translation_message_untranslated_count attribute
<jtv> Not saying it is, just pointing out something to think about in the requirements.  :)
<jtv> Maybe counting credits messages as translated would satisfy everyone.
<adiroiban> agree :)
<jtv> adiroiban: the branch I landed for you will be on staging with revision 9434.
<adiroiban> jtv: it will not be landed on edge?
<jtv> adiroiban: yes, that too.  But staging's nice if you want to play around freely.
<adiroiban> jtv: I saw that it was commited to edge in r10953
<jtv> Right, that's the revision.
<adiroiban> jtv: ok. so future work on ubuntu-upstream translation sharing is a blocker for this branch ? is they a way to predict those changes and move forward with the API design?
<jtv> adiroiban: one easy thing to do is start with the uncontroversial stuff.
<jtv> And last update & last translator, as far as I'm concerned, can be exported with a big caveat in the documentation.
<adiroiban> jtv: why is it so ?
<adiroiban> even if the translations are shared
<adiroiban> ah...
<adiroiban> I see
<adiroiban> they will not be updated
<jtv> Exactly.
<adiroiban> just like any other shared statistic
<jtv> But I would expect that whatever we do to them, they would probably stay unchanged API-wise.
<jtv> (We can't freeze all work out of fear of any API ever changing in any way :-)
<adiroiban> well, the note on documentation that they are not updated in real time should be made for almost all attributes
<jtv> True.  But personally I like to have a realistic sense of proportion: "may be as much as a week out of date" versus "may be a few seconds behind depending on replication lag" is a pretty big distinction, but weasel-word them enough and they'll look the same.  :-)
<adiroiban> :)
<adiroiban> are we going to have ubuntu-upstrea message sharing implemented for Maverick ?
<adiroiban> as far as I know, API freeze is done on each Ubuntu relase
<adiroiban> so we can change the API during this devel timeframe
<jtv> Hmm... it may not be a completely black-and-white thing.
<jtv> It could be that we land a working feature, but find lots more things to change, fix & adjust afterwards.
<adiroiban> so that work may also affect the API ... thus we can push the API regarding the ubuntu-upstream divergence as it is implemented now
<adiroiban> and change it later
<adiroiban> :)
<jtv> That's an option, though on general principle we'll want to minimize such changes.
<jtv> But now I must go.  Good night!
<adiroiban> yes, but as of this pre-imp talk I would like to have some conclusions and next step actions or creat a list of blockers for those next steps
* bac changed the topic of #launchpad-reviews to: On Call: abentley || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> abentley: you around for my review?
<abentley> bac, sorry, been distracted by production problems.
<bac> np abentley
<abentley> bac, all I see is a few places where an extra blank line could go.
<bac> abentley: ok, i'm good at adding blank lines.
<abentley> bac, e.g. between "EditByOwnersRegistryExpertsOrAdmins(AuthorizationBase):" and "permission = 'launchpad.Edit'"
<abentley> bac,  and between lines 100 and 101.
<abentley> bac, similar for EditProject and EditPersonBySelfRegistryExpertsOrAdmins
<bac> abentley: ok.  i just looked at security.py and it is a real mixed bag wrt to blank lines between the class/docstring and the first line of code.  i can clean it up.
<bac> s/can/will
<abentley> bac, cool.
* abentley changed the topic of #launchpad-reviews to: On Call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.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.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-06-08
* sinzui changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> rockstar I have a branch that needs UI review. I hope you or noodles has time to take a look at it tomorrow.
<rockstar> sinzui, I can look at it now if you want to send it to me.
<sinzui> rockstar, https://code.edge.launchpad.net/~sinzui/launchpad/team-participation-0/+merge/26996
<sinzui> ^ sorry, I had a child demand my attention
<rockstar> sinzui, looking
<rockstar> thumper, can you do a review for me?
<thumper> rockstar: sure
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-buildds/+merge/26990
<rockstar> thumper, there are two other cherry picks that need to occur before we can re-enable recipe builds.
<thumper> rockstar: yeah, I read your conversation with wgrant
<rockstar> thumper, cool.
<rockstar> thumper, I'm composing an email to the list an copying the appropriate people.
<thumper> rockstar: done
<rockstar> thumper, thanks.
<rockstar> thumper, can you do a review for me?
<rockstar> jtv, buddy!  Can you do a review for me?
<jtv> uh-oh  :)
<jtv> rockstar: fork it over!
<jtv> and does this mean you're polyphasic again?
<rockstar> jtv, no, just trying to clear off my plate of things I planned to do today.
<rockstar> jtv, https://code.edge.launchpad.net/~rockstar/launchpad/bug-589454/+merge/27012
<jtv> rockstar: the bug title says "creating recipe for build-from-branch oopses."  Is that what the MP is about?  At first sight they look like different bugs.
<rockstar> jtv, yeah, they are the same bug.  See the description.
<jtv> oic
<rockstar> jtv, basically, LaunchpadFormView's validation has some dirty little secrets...
<jtv> So the presence of "name" (which I suppose would be required) isn't enforced before you get to the validation method?
<jtv> Yup, the interface marks it as required, and yet your validator may get run when it's not given.  Nasty.
<rockstar> jtv, well, it adds the error to data.errors, but it continues on stupidly...
<jtv> Ah, that does make sense.
<jtv> On a coding style note, I'd prefer keeping the ISourcePackageRecipeSource utility in a variable over breaking the line all the way over on the right and continuing at what looks like a completely arbitrary offset.
<jtv> Especially in an "if."
<jtv> In fact, why not:
<jtv> name = data.get('name')
<jtv> if name is not None:
<jtv>     recipesource = getUtility(ISourcePackageRecipeSource)
<jtv>     if recipesource.exists(self.user, name):
<jtv>         self.setFieldError(
<jtv> etc.
<jtv> rockstar: ^^
<rockstar> jtv, sure, I can do that.
<jtv> thanks
<rockstar> jtv, http://pastebin.ubuntu.com/446468/
<rockstar> And the tests still pass, so I didn't screw anything up.
<jtv> Lovely.
<jtv> Also, you may want to use double-quotes for free-form text strings.  Saves the hassle when an apostrophe sneaks in.
<jtv> (e.g. if someone should ever decide that "There's" is better than "There is")
<rockstar> jtv, that's what we have syntax highlighting for...  :)
<jtv> Ah great, a high-tech solution for minimizing the pain of the tack you taped to your chair...
<rockstar> jtv, I have a standing desk now, so I don't have a chair...
<rockstar> That's how I solved that problem.
<jtv> rockstar: Telling.  :-)  The syntax checker I use in gvim is very nice, but it throws up a lot of complaints for a thing like this.
<rockstar> jtv, yeah, abentley turned me onto one than runs pyflakes for me as well.  I never have to run pylint anymore now.
<jtv> I think that's the one I use.
<jtv> Unfortunately if I were to add an apostrophe to this string, it would make me alternate between commanding vim and acknowledging error notices.
<jtv> ^<enter>r"<enter>$<enter>r"<enter>
<rockstar> jtv, oh, that's obnoxious...
<jtv> rockstar: now, stop me if I'm going too far here but in the test, instead of finding all error messages on the page and indexing at [1], would it be possible to find specifically the error message for the "name" field?
<rockstar> jtv, I think it is doing that.  Lemme look.
<jtv> Well it's my particular thumbtack of choice I guess.  :)  I've gotten used to a style where free-form strings use double quotes (because it's nobody's business if they contain apostrophes) but short, identifier-style strings use single quotes.  I find it nice and consistent, and believe others do something similar.
<rockstar> jtv, yeah, we don't have a style for strings specifically, but we do for docstrings.
<jtv> When writing style rules, it's probably a good thing not to enforce things you wouldn't be willing to clean up in the existing code (both as a limit on the body of law and a cure for existing nearby code overriding style guides) but in this case I think it's common sense to use double quotes.
<rockstar> jtv, I'll do it just this once for you, but I generally don't ever use double quotes unless I really need to.
<rockstar> Probably because I can't be arsed to use the Shift key.
<jtv> So... what do you do when you've got a sequence of lines of concatenated single-quoted strings and one of them gains an apostrophe?  Change all lines or just one?
<jtv> Or avoid the apostrophe?
<rockstar> I'll escape it if I really need.
<jtv> ouch!  :)
<rockstar> This probably stems from my PHP days, where a single quote was good for making sure you weren't doing something stupid in SQL.
<jtv> For that situation I can see it making sense...  but of course if Launchpad were PHP we wouldn't be having this conversation because I wouldn't be working here.  ;-)
<rockstar> But it's one of the niggles that I continue to use in every case except C (where single quotes and double quotes mean different things) and javascript (probably because it reminds me of C)
<jtv> Wouldn't the rule generalize to "use double quotes when it's okay to have single quotes or apostrophes in here, or single quotes when it's not"?
<rockstar> jtv, I think the important thing is that there is no rule.  I think every time it's been brought up, it's the equivalent of doctests v. unittests...
<jtv> Sure.  I'm talking about your rule, not a rule in Launchpad.
<rockstar> jtv, I don't have a rule, more than a habit.
<rockstar> It's like crack.  I don't necessarily WANT to do it, I just do.
<rockstar> jtv, so, "Required input is missing" is apparently the ACTUAL error message it puts next to the widget.
<rockstar> That's about as worthless as teats on a bull...
<jtv> rockstar: I'm not sure I follow... actual as opposed to what?
<rockstar> jtv, like, maybe "Name is a required field" or something.
<jtv> Ah.  Yes, could probably be better but I guess it's clear enough when it shows up right next to the input box.
<rockstar> <jtv> rockstar: now, stop me if I'm going too far here but in the test, instead of finding all error messages on the page and indexing at [1], would it be possible to find specifically the error message for the "name" field?
<rockstar> I'm responding to that...
<jtv> Ah, then I probably expressed myself poorly.
<jtv> What I meant was: is there no more precise way to identify the error message that you're testing?
<jtv> Right now the test finds all messages and then uses the list index [1] for this.
<jtv> I was wondering if there's an easy way to, say, find a tag surrounding just the "name" part of the form, and then pick out the error message from that.
<rockstar> jtv, I don't think there is a better way.
<jtv> Then I won't waste any more of your time.  I'm approving the MP.
<rockstar> jtv, I'm sure we can improve it, but this is the way the doctests do it as well.
<jtv> Yes, I know and it always saddens me a bit.  Someday I guess...
<rockstar> jtv, so, abentley and I have been trying to use unittests for testing pages.
<jtv> It strikes me as a worthwhile idea, since pagetests have a tendency to convolute and become brittle.
<rockstar> jtv, my favorite part is that if the page oopses, I get the actual exception instead of crap printed to my terminal.
<jtv> Nice, yes.
<rockstar> jtv, when the experiment is over, I'll post to the list about it.
<jtv> Looking forward to that.
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb, another one for you when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/587113-buildbase-handleStatus/+merge/27022
* noodles775 changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [sinzui, noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775, Sure, I'll put it in the queue.
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Lol... too quick :) Thanks gmb.
<gmb> Heh.
<stub> gmb: https://code.edge.launchpad.net/~stub/launchpad/db-stats/+merge/27026 is trivial
<stub> And as soon as I said that, spotted the bug :-P
<gmb> stub, Thanks; I'll take a look at that before starting on sinzui's branch
<stub> The 'per_second' like is going to get another * 60
<stub> Which I will shortly test on production to see if it fixes the odd results I'm seeing
<gmb> stub, r=me with that change in mind, then :)
<stub> Ta.
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: sinzui || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> Ahh... much nicer. My daily report now has reasonable values, matching the short term reports ;)
<gmb> :)
<stub> All because it is a pain in the bum converting a timedelta to a number of seconds.
<stub> https://code.edge.launchpad.net/~stub/launchpad/oauth-db/+merge/27025 is simple, but possibly security sensitive - if you don't want to review I can designate a victim familiar with OAuth.
<gmb> stub, That sounds like a good idea.
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> noodles775, r=me
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks gmb.
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> gmb, the small change we discussed:  https://code.edge.launchpad.net/~deryck/launchpad/ubuntu-bugs-front-timeout-590992/+merge/27036
* deryck changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: lunch || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> ah, he's away.
<deryck> BjornT, perhaps you could review the above MP for code and RC.  Fixes a persistent timeout on bugs.lp.net/ubuntu.
<deryck> meant that as a question, not statement. :-)
<leonardr> gmb, need nearly trivial review of https://code.edge.launchpad.net/~leonardr/launchpad/toggle-representation-cache/+merge/26725
* bigjools changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: lunch || queue: [deryck, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> one more for you gmb, thanks
<BjornT> deryck: have you confirmed that that view isn't used on other pages?
<deryck> BjornT, the view is for every top-level bugs page, not just distro.  a grep for bug_count_items turns up nothing.  doc and browser tests pass.  I'll look at zcml to make sure nothing else uses it, though....
<deryck> BjornT, yeah, not used except for bugs home page for product, distro, and series of those.
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [deryck, bigjools, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb sucks at IRC again
<bigjools> gmb: my branch is a CP candidate BTW, but that probably doesn't change much
<gmb> bigjools, I'll take a look at leonardr's first because it's 'nearly trivial'. Also, he asked first, looking at scrollback; seems I've done him a disservice
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: leonardr || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> 'tis fair
<gmb> deryck, Can I assume that BjornT's got you covered, review wise?
<deryck> gmb, yes, he did.  Sorry I didn't update you.
<gmb> deryck, Not to worry. Sorry I had to dash for lunch; it was that or fall over.
<deryck> gmb, no worries :-)
<gmb> Ah, codebrowse, your reliable suckage heartens me.
<gmb> leonardr, r=me
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> bigjools, r=me
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> thanks gmb
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb || reviewing: - || queue: [gmb(http://bit.ly/cy3QjX)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> gmb: could you review the mp: https://code.edge.launchpad.net/~adeuring/launchpad/expectations-bug-556499-model/+merge/27052 ?
* leonardr changed the topic of #launchpad-reviews to:  On Call: gmb, leonardr || reviewing: - || queue: [gmb(http://bit.ly/cy3QjX)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> adeuring, i'll take it
<adeuring> leonardr: thanks!
<leonardr> adeuring, am i reviewing the whole branch or your diff against intellectronica's work?
<adeuring> leonardr: Tom's work has already been reviewed
<leonardr> ok
<adeuring> so I think it is enough if you look at the diff I added to the MP
<leonardr> adeuring: i don't know much about storm, can you explain
<leonardr> Store.of(self.distribution).add(dsp_in_db)
<leonardr> it looks like you're just adding a row to the distribution table?
<adeuring> leonardr: no, Store.of(...) just returns the database, not a table.
<leonardr> so it's whatever database the distribution is in?
<adeuring> leonardr: yes
<adeuring> that's a typical pattern we use to find the "right" store
<leonardr> i don't see where you use the bug_reported_acknowledgment property.  was it used outside of that diff without being defined properly?
<leonardr> or, is that whole thing defined inside a model class?
<adeuring> leonardr: exactly. There is a test, let me look for the filename...
<leonardr> doc/bug-reported-acknowledgement.txt ?
<adeuring> leonardr: right, that's the one.
<leonardr> ok, i'll take a quick look
<adeuring> leonardr: and I'm working on another branch which will really sue this proerty
<adeuring> ...property...
<adeuring> erm, _use_ the property...
<leonardr> adeuring, r=me
<adeuring> leonardr: thanks!
<leonardr> gmb, i'll take the review you've got in the queue
<gmb> leonardr, Thanks.
* bigjools changed the topic of #launchpad-reviews to:  On Call: gmb, leonardr || reviewing: - || queue: [gmb(http://bit.ly/cy3QjX), bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> gmb: another CP candidate branch fer ya
<bigjools> short one
<gmb> bigjools, On it
<bigjools> ta
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb, leonardr || reviewing: bigjools, gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> gmb, r=me with some minor changes
<gmb> leonardr, Thanks.
<gmb> bigjools, r=me
<bigjools> thanks again
<gmb> leonardr, Gah, yes, that should be targeted at devel. Never mind, I'll fix it when I pqm-land it.
<gmb> leonardr, And apparently I can't spell.
<leonardr> gmb, i have the same problems
* gmb changed the topic of #launchpad-reviews to:  On Call: gmb, leonardr || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * gmb -> afk for a short while
* gmb changed the topic of #launchpad-reviews to:  On Call: leonardr || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, you're on call?
<rockstar> bac, can you do a review for me?
<leonardr> rockstar, yes, i'm here
<rockstar> leonardr, can I jump in your queue?
<leonardr> rockstar, yes, especially since there is no queue
* leonardr changed the topic of #launchpad-reviews to:  On Call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, https://code.edge.launchpad.net/~rockstar/launchpad/edit-recipe-name/+merge/27072
<leonardr> rockstar, i assume your first validate() method is in the 'add' view?
<rockstar> leonardr, the original validate is in a mixin, since I do share some code.
<rockstar> leonardr, but yes, I took some code out of there, put it into the add view, and then modified it for the edit view.
<rockstar> leonardr, oh crap, it didn't set the pre-req branch correctly.
<rockstar> leonardr, you can ignore test_create_new_recipe_empty_name
<leonardr> ignore it, or come back to it after you fix it?
<rockstar> leonardr, ignore it.  It's from the pre-requisite branch that jtv reviewed last night.
<leonardr> i see
<rockstar> leonardr, see also: https://code.edge.launchpad.net/~rockstar/launchpad/bug-589454/+merge/27012
<leonardr> rockstar, you removed a 'source_package = ' line from a test. it looks like it was simply unnecessray?
<rockstar> leonardr, yeah, it was an artifact of a branch from last week.  I must have forgotten to run make lint then.
<rockstar> leonardr, I caught it when I ran lint this time.
<leonardr> rockstar, r=me with an additional test case
<rockstar> leonardr, ack.  I'll get on that now.
<mars> leonardr, ping, have some time for a very small review?: https://code.edge.launchpad.net/~mars/launchpad/re-enable-windmill-ec2-suite/+merge/27077
<leonardr> mars, sure
<leonardr> mars, so you've already fixed the windmill test suite, and this is just restoring the status quo where 'make check' runs correctly on ec2?
<mars> leonardr, yes.  I ran it in parallel to reproduce what would have hung the suite, and now it passes the error just fine.
<leonardr> ok, r=me
<mars> leonardr, cool, thank you
<mars> leonardr, did you mark the MP as approved?
<leonardr> i think so., but maybe not
<leonardr> no, i got distracted, doing that now
<bac> leonardr: do you have time for a short MP?
<bac> leonardr: otherwise i could ask sinzui
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-591345/+merge/27084
<leonardr> bac, lemme see, but sinzui might be better--i just eoded
<leonardr> yeah, sunzui's a better choice, sorry
* leonardr changed the topic of #launchpad-reviews to:  On Call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> I hope that is a malapropism for Sinzui and Sun Tze.
<bac> i think i like sunzui better than sinzui.
<sinzui> The characters for Sun Tze looks like Sun-ko in Japanese, making him a her
 * sinzui is done passing useless knowledge
<sinzui> bac: we must have 2000 lines dedicated to pmts and we have only 17 of them...and we do not want any of them
<bac> ye
<bac> s
<bac> pardon me while i reboot
<bac> sinzui: thanks for the review
#launchpad-reviews 2010-06-09
<deryck> hmmm, it seems no one is on call reviewer today.
<deryck> adeuring, could I beg a review of you?
<deryck> warning that it's a long diff but a simple change -- moving duplicateof to markAsDuplicate.
<adeuring> deryck: sure
<adeuring> sorry, was out for lunch
<deryck> adeuring, no worries.  And thanks!  See:  https://code.edge.launchpad.net/~deryck/launchpad/do-the-right-thing-dupe-move-78596/+merge/27144
<adeuring> deryck: in BugMarkAsDuplicateView.setUpFields(), you set self.form_fields twice. Can't we remove the first assignment?
* noodles775 changed the topic of #launchpad-reviews to:  On Call: - || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> adeuring, let me look at the diff...
<noodles775> Well-tested trivial review for anyone with a spare few mins (71 lines, mostly tests): https://code.edge.launchpad.net/~michael.nelson/launchpad/589058-upload-rejected-date-started-not-set/+merge/27147
<adeuring> deryck: zthatÃs lines 54, 55 of the diff
<deryck> adeuring, I cripped this from the methods below where setPrivate is done, for example.  I assumed we had to explicitly omit the original field.  If not, I can change this.
<deryck> cribbed, rather
<adeuring> deryck: admitteldy, i am not sure... don't know enough about the "field magic"...
<deryck> adeuring, I can try it and test.  If so, I'll change it.
<adeuring> sounds good
<adeuring> deryck: r=me, with 1.5 comments
<deryck> adeuring, cool.  thanks!
* gmb changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [noodles775, gmb(http://bit.ly/bu2arK)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: On Call: Edwin || reviewing: - || queue: [noodles775, gmb(http://bit.ly/bu2arK)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> EdwinGrubbs: just for when you're ready, mine's pretty small: https://code.edge.launchpad.net/~michael.nelson/launchpad/589058-upload-rejected-date-started-not-set/+merge/27147
<EdwinGrubbs> I'll start on it soon.
<henninge> jtv: can you please look at this before you go?
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-591731-is-published-removal/+merge/27166
<jtv> henninge: looking...
<henninge> jtv: it's mostly mechanical
<jtv> henninge: get a chance to look at the wiki update?
<henninge> jtv: no, will do that now.
<jtv> Thanks.
<jtv> henninge: hope the drive-bys won't cause any conflicts.
<jtv> Or not too many, at any rate.
<henninge> jtv: I plan to merge stable (or was it db_stable) after my message sharing branch has landed.
<jtv> henninge: instead of a variable called diverged_and_not_current_upstream, it may be easier to read to have one called diverged and another called current_upstream.
<jtv> recife was based on db-stable.
<henninge> jtv: ah, good idea
<jtv> Although of course that's long since been released and become devel, and stable, etc.
<jtv> henninge: why do you introduce the imported1 and imported2 variables?  Are they used somewhere?  If not, they'll just cause lint warnings.
<henninge> jtv: they are used in the assignment right after it.
<jtv> OK
<henninge> the tests were breaking because they were missing.
<jtv> henninge: I've approved your MP.
<henninge> jtv: thanks ;)
<jtv> np
* gmb changed the topic of #launchpad-reviews to: On Call: Edwin || reviewing: - || queue: [noodles775)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar, I can has review? https://code.launchpad.net/~abentley/launchpad/config-disable-build-requests/+merge/27172
<gary_poster> sinzui: my branch for the tal test convenience failed in ec2.  The fix I made for it might be objectionable, or possibly improvable.  Could you look at the diff again?  The only part I personally really think you need to review are lines 109-133 of the diff.  https://code.edge.launchpad.net/~gary/launchpad/bug586466/+merge/27174
<sinzui> gary_poster, you should use push() and pop() when working with the config
<gary_poster> sinzui: but I don't think I can do that with changes of that variable, can I?
<sinzui> push()  is intended to change one or more variables
<sinzui> that is how we test instances
<gary_poster> OK, will try.
<abentley> EdwinGrubbs, could you please review https://code.launchpad.net/~abentley/launchpad/config-disable-build-requests/+merge/27172 ?
<sinzui> gary_poster, look at lib/canonical/launchpad/pagetests/basic/demo-and-lpnet.txt, line 88
<EdwinGrubbs> abentley, is it urgent, or can you wait until I finish another review.
<sinzui> gary_poster, the teardown is on line 141
<gary_poster> ack, looking, thank you
<abentley> EdwinGrubbs, not urgent.
<gary_poster> sinzui: had to adjust thinsg a bit, but is all for better: https://code.edge.launchpad.net/~gary/launchpad/bug586466/+merge/27174, lines 110-117, 125 and 151 are the pertinent ones.
<EdwinGrubbs> noodles775, where is the build.duration attribute defined? I don't see it in BinaryPackageBuild or its superclasses.
<noodles775> EdwinGrubbs: lp.buildmaster.model.buildfarmjob (it returns None if either of the values are None).
<noodles775> (it's via delegation rather than inheritance)
<EdwinGrubbs> noodles775, r=me
<noodles775> Thanks EdwinGrubbs
<sinzui> gary_poster, r=me
<gary_poster> thank you sinzui!
<EdwinGrubbs> abentley, maybe, you can't actually get to the sourcepackagerecipe +index page normally, but it seems like you need to disable the link returned by SourcePackageRecipeContextMenu.request_builds
<abentley> EdwinGrubbs, why?
<EdwinGrubbs> abentley, because, I can click on it to get to the form that causes an oops. From your mp, it sounded like you had already removed any way to navigate to that form.
<EdwinGrubbs> abentley, even when build_from_branch.enabled=False, the request_builds link is active.
<abentley> EdwinGrubbs, we have disabled any way to navigate to that form.  The only way you can get there is by URL hacking.
<abentley> EdwinGrubbs, going to the +index page is URL hacking.
<EdwinGrubbs> abentley, ok, r=me
<EdwinGrubbs> abentley, oh, I didn't see that it has already been reviewed and approved.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On Call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<deryck> Hi EdwinGrubbs.  I have a branch for review I'd like to get on the queue.  but I'm EODing soon.
<deryck> EdwinGrubbs, can we start now and finish offline, or would you rather I ping someone else later?
<deryck> EdwinGrubbs, never mind (for when you're available again).  I'll worry about review later.  Thanks, anyway.
<abentley> rockstar, https://code.launchpad.net/~abentley/launchpad/recipe-build-email/+merge/27199
<wgrant> abentley: Why not change BinaryPackageBuild.notify() to return early when it's FULLYBUILT, rather than adding a special _handleStatus_OK? lp.buildmaster should be generically useful, and not require hacks like that.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-06-10
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> thumper or anyone who can rs a testfix: http://pastebin.ubuntu.com/447621/
<noodles775> It was the result of a bad but non-conflicting merge.
<noodles775> to test: `bin/test -vvm test_sourcepackagerecipebuild`
 * noodles775 RS's himself.
<thumper> noodles775: it is fine to rs those yourself I think
<noodles775> thumper: yeah, I thought so too :)
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: - || queue: [noodles(http://bit.ly/a6qLFn)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: deryck || queue: [noodles(http://bit.ly/a6qLFn)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: deryck, noodles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: mars, noodles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks jelmer_ :)
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: mars, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775, jelmer_: got a slightly weird but very simple review for you.
<noodles775> jtv: r=me, but why not just merge it and resolve the conflict once (then you won't have to worry about it?) Are you keeping a pipeline with your feature, or just a single branch?
* bigjools changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: mars, abentley || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> trivial CP branch
<bigjools> well - trivial if you know SQL like stub does :)
 * bigjools -> kunch
<bigjools> lunch even
<noodles775> :)
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: brian-murray, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> wgrant, mainly because _handleStatus_OK scares me, but also because I don't trust that if I break it the tests will catch it, and because another team is the main owner of the code.
<wgrant> abentley: Mmm, true, it is pretty scary.
<wgrant> But large piles of hacks also scare me. :/
<abentley> wgrant, whether or not it's a hack depends on whether you think _handleStatus methods should always notify.  Otherwise, it's just programming-by-difference.
<stub> https://code.edge.launchpad.net/~stub/launchpad/pending-db-changes/+merge/27263 has some random minor DB related updates for production stuff
* stub changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: brian-murray, abentley || queue: [stub] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: brian-murray, stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: brian-murray, stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: brian-murray, stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: brian-murray, stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Ah, jelmer_, had you already started stubs review?
<jelmer_> no, I haven't started yet
<jelmer_> noodles775: did you already start?
<mars> noodles775, jelmer_, have time to review a branch that will restore everyone's confidence in ec2?  :)   https://code.edge.launchpad.net/~mars/launchpad/use-zope.testing-3.9.4-p1/+merge/27265
<jelmer_> I can take mars' mp
<noodles775> mars: I've already done it.
<mars> noodles775, wow, that was fast.  Thanks!
<mars> noodles775, thinking about your comment
<noodles775> jelmer_: ^^, and no, I hadn't started stubs, I just clicked on claim a few seconds before you changed the topi.
<noodles775> topic.
<noodles775> jelmer_: I'll just finish stubs now that I've claimed it then :)
<jelmer_> noodles775: Ok :-)
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> mars: what does your branch fix wrt ec2 ?
<mars> jelmer_, updates to a new version of the zope.testing testrunner with a backported patch fixing a crash.
<mars> jelmer_, the testrunner would crash, forgetting all previous work, then continue to run the suite as if nothing happened: false suite success.
<jelmer_> mars: ah, ok
<jelmer_> mars: I'm still seeing branches disappear, is that a known issue?
<mars> jelmer_, when was the last disappearance?  I landed the fix yesterday
<jelmer_> mars: yesterday evening, after your fix landed afaik
<jelmer_> mars: I'm running the testsuite locally but I'll try again with ec2 as well
<mars> jelmer_, may have been a different bug: same crash, different source
<mars> jelmer_, the subprocess handling in zope.testrunner is screwed.  This will keep happening until I land another branch or two.
<jelmer_> mars: ah, ok
<mars> jelmer_, with what frequency are you seeing ec2 disappearances?
<mars> I was seeing it with 40% frequency
<jelmer_> mars: it varied; it used to be one in 4 but I've seen more recently (40% sounds about right)
<jelmer_> also, some branches seem more prone to disappearance than others
<mars> makes sense depending on what the branch changed.  So far the failures are caused by race conditions and such.
<mars> jelmer_, it is also possible that this branch that fixes the doctest failures will fix the crashes you are seeing
<mars> jelmer_, I'll let you know when it lands.  Hopefully that will make more of them go away.
<jelmer_> mars: thanks!
* noodles775 changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jelmer_: are you landing that branch of mvo's? (or can you?)
<rockstar> jelmer_, you're not still around, are you?
<jelmer_> rockstar: I am
<jelmer_> rockstar: what's up?
* jelmer_ changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> jelmer_, I was gonna change the topic...  :)
* rockstar changed the topic of #launchpad-reviews to: On Call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> ah :-)
<gary_poster> rockstart, anyone, up for reviewing a change to our deps for the python2.6 switch? https://code.edge.launchpad.net/~gary/meta-lp-deps/generic-python/+merge/27292
<gary_poster> rockstar
<rockstar> gary_poster, sure.
<gary_poster> thank you
<rockstar> gary_poster, r=me
<gary_poster> thanks rockstar
<abentley> rockstar, could you please review https://code.launchpad.net/~abentley/launchpad/version-revno/+merge/27308 ?
<rockstar> abentley, sure.
<abentley> rockstar, ta
<rockstar> abentley, is {revno} special to bzr-builder?
<abentley> rockstar, yes.
<rockstar> abentley, ah great, so we don't need the MANIFEST at all.
<abentley> rockstar, huh?
<rockstar> abentley, we were talking about doing this ourselves, but we needed access to the MANIFEST to get to the bzr revno.
<abentley> rockstar, we'll still need manifests to abort duplicate builds, because {revno} is only evaluated once the build has started.
<rockstar> abentley, ah, okay.
<rockstar> abentley, r=me
<abentley> rockstar, thanks.
* rockstar changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-06-11
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313
<rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-branch-requestMirror/+merge/27317
<thumper> rockstar: hey
<thumper> rockstar: looking at the oopses one
<rockstar> thumper, cool.
<rockstar> thumper, ping
<rockstar> thumper, re: "Is it right to hard code the formats?" we talked about this same issue when I first worked on BranchUpgradeJob.  We decided it wasn't a big deal, and especially not since bzr won't be changing its default format until 3.0.
<thumper> rockstar: I just think it would be better to get the formats out of the branch itself
<thumper> rockstar: in that particular situation
<rockstar> thumper, sure.  I can do that.
<thumper> rockstar: thansk
<rockstar> thumper, can you look at the other one as well?
<thumper> rockstar: I did
<thumper> I even commented
<rockstar> thumper, I mean this one: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313
<thumper> ?!?
<thumper> I did
<thumper> damn, wtf?
 * thumper tries again
<thumper> rockstar: look now
<thumper> rockstar: I have one for you if you want
<rockstar> thumper, sure, I can look at it.
<thumper> rockstar: did you see the buildbot failure?
<thumper> rockstar: it is a windmill failure
<rockstar> thumper, no, I haven't seen it.
<rockstar> thumper, how do I take bzr branch format and get the corresponding BranchFormat enum?
 * rockstar forgets...
<thumper> rockstar: take a look in the codehosting ssh server
<thumper> rockstar: I don't remember
<thumper> but that is the other location it is done
<thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/branch-index-slowness/+merge/27318
<thumper> rockstar: I've somehow managed to change my gnome window decoration to something that looks like it is out of the 90s
<thumper> it is like the icon set changed
<rockstar> thumper, log out and log back in.
<rockstar> The gnome config daemon probably died...
<rockstar> (happens to me all the time)
<thumper> eh?
<rockstar> It's hella annoying.
<thumper> ah, yeah
<thumper> that's why I left kubuntu
<thumper> well, left
<thumper> well, trying gnome for a bit
<rockstar> thumper, Lucid is not the best gnome foot forward...  :/
<rockstar> thumper, line 150 of your diff, why not use isinstance?
<thumper> rockstar: because I didn't think of it :)
<rockstar> thumper, okay.  Can you remedy that?
<thumper> yep
<rockstar> thumper, do you have some data to go with this branch that might be good to go to the list?
<thumper> rockstar: what are you asking exactly
<rockstar> thumper, I think query count numbers along with the changes in this branch would be good to post to the list.
<thumper> rockstar: I'll be writing something up, yes
<rockstar> thumper, also, there are new changes to look at on this review: https://code.edge.launchpad.net/~rockstar/launchpad/fix-branch-requestMirror/+merge/27317
<thumper> rockstar: you don't close the branch
<thumper> rockstar: you'd make me very happy if you created a context manager
<thumper> rockstar: and went "with BzrBranch.open_from_transport(source_branch_transport) as branch:" ...
<rockstar> thumper, I can move ForbiddenInstruction to lp.code.errors.  I think hardcoding the instruction isn't that bad, since there are only three actual instructions in the first place.  Thirdly, I can't do the catch of the exception in the validate because the exception isn't raised until we actually try and make the changes.
<rockstar> abentley and I talked about the third option, and we both came to the conclusion that it's the best way to do it.
<thumper> rockstar: isn't there a validator for the recipe?
<thumper> rockstar: there are only three instructions now, what about later?
<thumper> rockstar: if the information is in the exception (and it should be) then it shouldn't be hard to use right?
<rockstar> thumper, I think that would make the code more counterintuitive than it needs to be.
 * thumper has to go move a sofa
<thumper> why?
<abentley> thumper, it follows Easier to Ask Forgiveness Than Permission.
<thumper> which is against the zope way of doing views
<thumper> however...
<thumper> I think we can let this go here for now
<thumper> as we have a precident
<abentley> thumper, if EAFP is against Zope conventions, that's another example of our architecture/conventions not encouraging optimal code, both from a speed and maintenance perspective.
<thumper> abentley: it is what the validate method is for
<abentley> thumper, preventing speed and maintainability?
<thumper> abentley: I _think_ it is to offer a single place of validation for potentially many actions
<abentley> thumper, if that is the single place for validation, then we can't have validation in the model, and that leads to madness.
<thumper> abentley: true
<thumper> We are no longer using views how zope intended I think
<thumper> given our API exposure of the models
<abentley> thumper, well...
<abentley> thumper, I think validation of the input has a place.  e.g. if x is supposed to be an INT, make sure it's an INT.
<abentley> thumper, but I don't think the views should attempt to replicate the model rules.  It means you have to keep two pieces of code in sync.
<thumper> yeah, that makes sense
<abentley> thumper, until rockstar pointed out that actions could call setFieldError, I assumed we had no choice.
<rockstar> thumper, on further inspection, it looks like we can't get to the actual message.  bzr-builder would need a patch before we could.
<thumper> rockstar: what about the command?
<rockstar> thumper, yeah, the command actually doesn't have any information about what the actual command is.
<thumper> rockstar: ok then.  Can you file a bug and XXX the callsite
<thumper> ?
<rockstar> thumper, ack.
<rockstar> thumper, I think we can probably even patch it fairly simply, now that I'm looking at it.
<rockstar> thumper, this one is updated: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313
<rockstar> thumper, I'm not sure what the context manager gives us.
<thumper> closing the branch and nicer code
<thumper> also, this has conflicts now
<rockstar> thumper, no, I resolved those, wait for the diff.
 * thumper waits
<rockstar> thumper, where else are we "closing" a branch?
<thumper> rockstar: don't know, but if you open something, it is good manners to close it :)
<rockstar> There's not even a "close" method.
<thumper> really?
<rockstar> thumper, I don't see one.
<rockstar> thumper, this is why I ask where else we're doing it.
 * thumper asked on #bzr
<rockstar> thumper, I don't see anywhere where we close a branch.  Even BzrSync.syncBranchAndClose just unlocks the branch.
<thumper> rockstar: forget about the context manager then
<rockstar> thumper, I can has approve then?
<thumper> aye
<rockstar> thumper, thanky
<rockstar> thumper, talk to you in week 4.
<thumper> rockstar: have a good break
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* 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.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hello adeuring
<adeuring> hi bac
<bac> adeuring: just letting you know i'm on call now too
<adeuring> bac: thanks; seems to be a quiet day :)
<bac> cool
<bac> adeuring: did you see that sinzui stealthily added branches to the queue?  you want to split them up?
<adeuring> bac: whoops... no
<bac> i'm not sure why he always flies under the radar like that.  very unproductive
<adeuring> OK, I'll take https://code.edge.launchpad.net/~sinzui/launchpad/front-page-footer-0/+merge/27311
<bac> ok
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: sinzui, - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring 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.edge.launchpad.net/launchpad/+activereviews
<bac> hi sinzui, question about your test for PRF changes
<bac> well, actually about the dispay of latest downloads that changed in that branch
<bac> sinzui: you have a new test showing obsolete series are ignored
<bac> before setting s4 to obsolete, the latest with downloads was 3.3, which is a part of s3.  so even if s4 is marked obsolete shouldn't 3.3 still display?
<sinzui> bac: I think I demonstrated that in the test.
 * sinzui looks
<sinzui> bac, waaa..
<sinzui> bac: I am going to run that test. It looks wrong and I think I did say 3.3 was the latest. I may have backed that out of the test
<bac> sinzui: ok.  it just looks funny to the gentle reader.
<sinzui> :(
<sinzui> bac, I did backout my change
<sinzui> well, ctrl-z
<sinzui> ah
<sinzui> Wow
<bac> sinzui: the test passes for me, which makes me confused
<sinzui> I really messed that up. current is 4.3, and after obsolescence it is 3.3
<sinzui> no way,
<sinzui> bac, http://pastebin.ubuntu.com/448268/
<sinzui> I will make the change I intended to make. What I see in the review is the test I wrote before I changed the code and saw I also misread the test
<bac> sinzui: oh you're right, it does fail for me
<bac> sinzui: you didn't include that test in the "test" line of your MP so i hadn't actually run it
<sinzui> It was just after midnight I screwed up.
<sinzui> bac: http://pastebin.ubuntu.com/448271/
<bac> sinzui: see, curtis, turning into a pumpkin is just a metaphor.  you really should stop work after midnight.
<adeuring> sinzui: r=me; two nitpicks
* 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.edge.launchpad.net/launchpad/+activereviews
<sinzui> adeuring, thanks
<bac> sinzui: r=bac, with a couple of changes and one absurd request
* bac changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: -, - || queue: [ -] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> thank bac
<sinzui> bac ping
<bac> hi sinzui
<sinzui> about your points.
<sinzui> 1. I too hate glob
<sinzui> and it is not clear what uses it
<bac> wellispecificallyhatedthelackofseparatorsinthename
<sinzui> We can start by making  good api name. maybe release_finder_url_pattern?
<bac> sinzui: hey that's much better!
<bac> cause, you know what?  it *is* an URL pattern
<bac> sinzui: fwiw i did exercise it via lplib and got validation errors when i tried just using 'cow', etc
<sinzui> yep
<sinzui> I want the name to state what it is and for. One day I will make the release finder public and easy to understand
<bac> nice
<sinzui> bac: may I change the name in the api then?
<bac> sinzui: i think that's a swell idea
<sinzui> I am not interested in adding an lplib test for this. I added it because I personally need this to see the damage that has been done on edge. No one cares about this as a feature until users know it is a feature
<gary_poster> adeuring or bac: up for a review of a very small chance to our meta dependencies?  https://code.edge.launchpad.net/~gary/meta-lp-deps/geoiplite/+merge/27381
<adeuring> gary_poster: I'll look
<gary_poster> Thank you adeuring. Note that there is a pre-existing dependency on geoip-data | geoip-data-city-lite fro launchpad-dependencies.  I'm just trying to say that devs (and people expecting to run tests)really need the lite version
<gary_poster> for
<adeuring> gary_poster: ok, r=me
<gary_poster> thank you adeuring
<maxb> gary_poster: So you're implying that launchpad itself can run with geoip-data, but the tests need -city-lite?
<gary_poster> maxb: yeah.  I just committed it and am pushing it.  Is that OK by you?
<maxb> if that's the case, fine. It just feels a bit perverse that the tests need one thing and production needs another
<gary_poster> I agree
<gary_poster> I think it is about licensing issues, and open-source issues
* adeuring changed the topic of #launchpad-reviews to: On Call: bac || reviewing: - || queue: [ sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> sinzui: you are in the topic queue but i think that is a mistake, right?  i see nothing on activereviews
<sinzui> bac: yes I think I am done
* bac changed the topic of #launchpad-reviews to: On Call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-590840-getMembershipInformation-timeout-2/+merge/27394
<bac> i will
* bac changed the topic of #launchpad-reviews to: On Call: bac || reviewing: edwin || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> EdwinGrubbs: in the MP you talk about "testing both of the unioned queries".  i don't see in your changes where that is addressed.  could you clarify?
<EdwinGrubbs> bac: sorry, that was unclear. registry/doc/mailinglist-subscriptions.txt already tests the team_members query in getSenderAddresses(). I added the tests in message-holds.txt to test the approved_posters query which gets unioned to to team_members in getSenderAddresses(team_names).
<EdwinGrubbs> bac: it's a little confusing that I'm adding tests for getSenderAddresses(team_names), but the branch also modifies the other getSenderAddresses().
<bac> EdwinGrubbs: ok.  that makes sense.  the branch proper looks fine...i was just confused.
<bac> EdwinGrubbs: finally, i assume there was no real benefit to returning a generator instead of the storm resultset as you do now.
<bac> oh, i see they were filtering the results too, which you don't have to do now
<EdwinGrubbs> bac: right, the generator is just overhead
<bac> sinzui: is your other branch going to make it today?
<sinzui> No
<sinzui> I suck
<bac> doh
<bac> sinzui: well i will look at it over the weekend if you want
<sinzui> No rush
<sinzui> I will work on lint over the weekend though
<bac> schweet
<sinzui> bac: I have a 19503 line branch that removes glob imports from our tree.
<bac> really?
<sinzui> I fear the review would kill
<bac> how many 800 line branches would that be?
<sinzui> 4
<sinzui> But this is a mechanical branch
<bac> sinzui: i can work on it this weekend
<sinzui> I have fixed all but one call site
<sinzui> c.l.__init__ has a lot in it still because removing all causes cyclic import problems
<sinzui> bac: I want an rs to get this into a safe state, so that future branches can do the hard thinking work of decoupling the cyclic imports
<bac> why don't you do a rs=sinzui then?  you know what you've done better than i do and that is perfectly legal
<sinzui> You have a good point.
<sinzui> gary talked me off the ledge this week so the scope is safe
#launchpad-reviews 2010-06-13
* sinzui changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac, mars, abentley, rockstar: I have a branch that fixes the main_side layout I broken today: https://code.launchpad.net/~sinzui/launchpad/lp-layout-0/+merge/27444
<sinzui> ^ It would be wonderful to land this for the next edge update.
<rockstar> sinzui, looking
<sinzui> thanks rockstar
