#launchpad-reviews 2010-08-16
<lifeless> hmm, I wants  areviewer
<lifeless>  https://code.launchpad.net/~lifeless/launchpad/cachedproperty/+merge/32728
<lifeless> anyone up for it ?
<lifeless> mwhudson: not that I'm stalking you or anything .... pretty please?
 * mwhudson looks
<mwhudson> lifeless: i guess you grepped for existing __storm_invalidate__ methods so i don't have to? :)
<lifeless> yeah
<lifeless> in devel as of latest commit a couple of hours ago
<lifeless> of course, folk can still add them
<lifeless> so it needs a psg update, mail to the list, various repeats etc etc
<mwhudson> lifeless: approved
<lifeless> \o/
<lifeless> mwhudson: click the other button too
<lifeless> (please)
<mwhudson> lifeless: done
<mwhudson> lifeless: can't you do that though?
<lifeless> mwhudson: I can, but its a pita :)
<mwhudson> ok
<lifeless> so I try to do it for other people when I review their branches
<lifeless> so that when they are told approved, they just need to do 'ec2 land'...
<lifeless> rather than reopen the web page, etc etc
<lifeless> anyhow thanks heaps for the review
<lifeless> sorry if that last bit came across grumpy
<lifeless> its just high friction
<mwhudson> yeah, it was a slip on my part i agree
<mwhudson> thanks for the prod
<lifeless> hah, so the SQLBase super() call is bogus
<lifeless> because the base class doesn't have one, so I'll nuke that :)
<lifeless> (the test suite does blow up, I just hadn't run model class tests ;P)
<lifeless> mwhudson: around? I'm about to pop in a fix or two to that branch (nonobvious in foresight, clear in hindsight)
<lifeless> mwhudson: rev 11352 and 11353 on lp:~lifeless/launchpad/cachedproperty
<mwhudson> lifeless: looking
<lifeless> thanks!
<mwhudson> lifeless: looks fine
<lifeless> thanks
<mwhudson> the need for 11353 is a bit depressing
<lifeless> yes
<mwhudson> hm
<mwhudson> oh nm
<lifeless> OTOH its good that folk are *already* using cachedproperty :P
<mwhudson> was thinking that maybe you could write this as self.cachedprop.cache(value)
<mwhudson> but obviously not
<mwhudson> because of what self.cachedprop does :)
<lifeless> yes
<lifeless> it would be lovely
<lifeless> but sadly, DENIED
<lifeless> it might be an idea to make _cachedproperties a dict
<lifeless> and then clear_properties can just replace the dict
<lifeless> but since its now nicely abstracted ('nicely') we can do that later.
<mwhudson> well, i guess you can do self.__class__.cachedprop
<lifeless> I don't think so
<lifeless> when the decorator is invoked you get a bare function
<lifeless> no im_class attribute
<lifeless> and definitely no instances (instances being what have __class__)
<lifeless> oh, I see
<lifeless> uhm, yes, we could make it into a obj with methods that you access via self.__class__.publicname
<lifeless> self.__class__.publicname.clear(self)
<lifeless> it a tad ugly
<mwhudson> yeah, i think it's a bit ugly
<mwhudson> so let's stick with what you have for now
* 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
<henninge> Bring on your branches!
* StevenK changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> henninge: Hi! :-)
<lifeless> henninge: hi, I've made some changes to a branch sinzui approved, how do you feel about an incremental review ?
<henninge> lifeless: Unless it's urgent I'd like you to take it back to sinzui, actually .. ;-)
<lifeless> henninge: well, given ec2 takes 4 hours to land stuff, I was hoping to throw it in overnight :)
<henninge> ah right, forgot were you are ... ;)
<lifeless> :P
<henninge> lifeless: put it on the queue
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/32067
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* lifeless changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> henninge: there is one commit in particular I want to discuss - http://bazaar.launchpad.net/~lifeless/launchpad/registry/revision/11324
<lifeless> which is not domain specific
<lifeless> when you're done with mr K
<henninge> ok ;)
<StevenK>  Mine is only a widdle branch
<henninge> StevenK: Hi! Looking ;
<henninge> StevenK: I seem to remember that we voted in favor of lifeless' suggestion about on-import-per-line. Unless I am mistaken, could you please restructure the import lines that you touched?
<henninge> StevenK: other than that, r=me ;-)
<lifeless> we did, imports are now like any other list.
<lifeless> well, nearly - still alpha-sorted please :)
<lifeless> henninge: so, my branch looks big, but all the bulky stuff is already reviewed
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * henninge looks
<lifeless> I've explained in the review page the incremental stuff I would love to have more eyeballs on
<henninge> danilos: the branch is ready for review btw.
<danilos> henninge, cool, thanks, I'll take a look at it as soon as I am done with the debugging I am doing right now
<henninge> lifeless: looking at r11324, you should call the result of removeSecurityProxy "naked_instance" or use rSP in place.
<lifeless> sure, I can do that
<henninge> lifeless: why do you (think you) need the rSP calls?
<lifeless> because model objects hold security proxied references to other model objects
<lifeless> and having many little 'clear_method_x' named functions in the public interface would make me want to stab my eyeballs out :)
<lifeless> I think a few more iterations and we'll come up with some good way of managing these things that fits with interfaces and security proxies, but right now its very adhoc: these two branches clean it up a lot (IMNSHO), but also find the limits of the self same cleaned up protocol
<henninge> lifeless: I have to admit I am on shaky ground here ...
<lifeless> ok
<lifeless> this is something new
<lifeless> I don't think that *anyone* can say 'I know all this stuff and its definitely good/bad/whatever'
<lifeless> there are acknowledged risks - and you can see curtis's comment: land it and we'll fix what breaks
<lifeless> these incremental changes are simply the first things that goes boom :)
<henninge> lifeless: actually, my original question about the "why" was if you did see tests failing without the rSP in place. In your comment on the MP you say that you are not sure if they are needed at all.
<lifeless> henninge: oh, I mean sttructurally
<lifeless> like
<lifeless> can we find some (within scope of this branch) way to not need them
<lifeless> but I couldn't see one
<lifeless> ArchiveSet.new
<lifeless> references owner.archive
<lifeless> which populates the cache
<lifeless> and then takes an action that invalidates it IFF the result of the lookup was None
<lifeless> so the change to archive.py to invalidate has to work when the person object used for owner is security proxied
<lifeless> henninge: ?
<henninge> lifeless: how about this:
<henninge> do not do the blanked "removeSecurityProxy" in cachedpropety.py but rather do it in archive.py for the specific use case.
<lifeless> henninge: It looks to me like every use is likely to need it
<lifeless> AIUI the general case is that external references are proxied
<henninge> lifeless: the use of these functions (not the decorator, of course) is quite limited.
<lifeless> henninge: of course, they are added today
<henninge> lifeless: argh, missed that ...
<lifeless> so is_cached clearly has no downside for our code, doing rSP
<lifeless> likewise clear_*
<lifeless> cache_property *could* be misused
<lifeless> but I think it will stand out very clearly where its being used
<lifeless> so its ok
<lifeless> (IMHO)
<henninge> lifeless: well, with these functions being new, we can asses each use of them more easily.
<lifeless> that too
<henninge> we are not in danger to introduce a security hole where there was none before.
<lifeless> so in summary, I'd like to leave rSP in the readonly functions for sure
<lifeless> I'm open to moving rSP out of the cache_property call, but I think we'll end up being more annoyed than benefited :)
<henninge> lifeless: I tink it is fine as long as it is clear that these are low-level functions that do not care about security.
<lifeless> I'll tweak the docstring on cache_property then
<lifeless> for the others, I think its not security related as such *anyhow*
<lifeless> because you can *at most* find out that an attribute exists, not get its value
<henninge> lifeless: can you please create a seperate doctest for these functions instead of putting the tests in the docstrings?
<henninge> We have been moving tests from docstrings into files.
<lifeless> well, doctests are pain anyhow
<henninge> I guess that was one reason I thought this was (really) old code.
<lifeless> I followed the minimal path doing this though
<lifeless> so
<lifeless> how about we treat the tests - including the cachedproperty test - as techdebt, and I'll make a new branch to move it to lp.services, clean it up into proper unit tests and so on at the same time.
<lifeless> I can do that wednesdayish
<henninge> lifeless: that's perfectly fine.
<henninge> lifeless: another idea I had: It would be great if the functions could do the same name mangling as cachedproperty itself.
<lifeless> deal
<lifeless> cachedproperty doesn't name mangle :)
<lifeless> oh, I see the line you mean
<lifeless> uhm
<lifeless> yes, I can pull that out
<lifeless> problem is
<lifeless> we don't have the original fn to examine
<lifeless> so we could *claim* it was called 'foo'
<lifeless> and mangle that
<lifeless> but that will fail for all cases where an explicit cached attribute name was chosen
 * henninge greps
<lifeless> so we'd need another parameter to say 'and don't mangle this' (or we'd probe twice)
<lifeless> line 76
<lifeless> probing twice is unneeded overhead
<henninge> lifeless: how about a named parameter?
<lifeless> and having two parameters for utility functions seems a bit ugh. I'd rather work on being able to do stuff with the actual descriptor
<henninge> clear_property(owner, attr='archive')
<henninge> clear_property(owner, name='_archive_cache')
<henninge> ?
<lifeless> do you think it adds much ?
<lifeless> can always add it later if we decide we want it after all
<henninge> I just think that the name mangling should be black boxed.
<lifeless> so, we couldn't really do this until I added these helper functions anyhow
<lifeless> I think you're right that it would be nicer to not require folk to thing about this
<lifeless> I have two concerns
<henninge> yes, what I mean. Using cachedproperty is quite transparent atm
<lifeless> henninge: well, many users do cachedproperty('_name_to_use') already
<lifeless> anyhow concerns
<lifeless> setattr(thing, 'name', value)
<lifeless> ==
<lifeless> cache_property(thing, 'name', value)
<lifeless> so its easy to translate in your head
<lifeless> cache_property(thing, attrname='name', value) # This is not legal python
<lifeless> so it would have to be
<lifeless> cache_property(think, value, attrname='name')
<lifeless> cache_property(think, value, attr='name')
<lifeless> which is the oppposite order and I worry that that is prone to confusion
<lifeless> I think I'd rather add in mangling versions later, if we need them
<henninge> I understand.
<henninge> Sure, was just my idea.
<lifeless> cache_attribute(thing, unmangled, value)
<lifeless> henninge: its a good idea
<lifeless> needed thought :)
<henninge> Here's another one ;)
<henninge> for a future branch.
<henninge> It would be great to be able to prevent caching in the first place.
<lifeless> what do you think of waiting and seeing ?
<henninge> instead of having to reset it right afterwards.
<henninge> yes, just an idea, again ...
<lifeless> yeah, that gets back to the descriptor idea
<lifeless> 'permit a cache read but don't do a cache write'
* noodles775 changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lifeless || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> henninge: Easy branch testing and fixing a recent JS breakage: https://code.edge.launchpad.net/~michael.nelson/launchpad/561586-qa-fail/+merge/32745 (63 lines)
<henninge> lifeless: r=me ;-)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi noodles775! ;-)
<henninge> Let me see if I can do this before lunch ... ;)
<noodles775> Thanks henninge
<lifeless> henninge: thank you!
<henninge> lifeless: pleasure
<henninge> noodles775: lint should complain about two extra blank lines at the end of the file ...
<noodles775> henninge: yes it did, and I fixed them, and forgot to push...
<henninge> noodles775: np, r=me
<noodles775> Great, thanks henninge.
* 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
* 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
* 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
<jtv> henninge, got a review for ya: https://code.edge.launchpad.net/~jtv/launchpad/recife-karmarecorder-fix/+merge/32756
* 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
<henninge> jtv: thanks ;)
<jtv> henninge: you are ironing, I'm sure :)
<jtv> Or whatever the verb for "being ironic" is
<henninge> Now, I am done with my houshold chores. ;)
<henninge> s/Now/No/
<henninge> jtv: XXX: Module docstring goes here.
<jtv> henninge: whoa, thanks
<jtv> henninge: do I see you're also waiting for a review?
<henninge> jtv: yes, danilo hat offered to take it but if he is not up to it, I'd be happy if you could do it.
<jtv> henninge: ah, that's the one you mentioned in the callâ¦  Yes, I think I can do that.
<henninge> jtv: where is he anyway?
<jtv> Just moving in mysterious ways, I'm sure.
<henninge> jtv: comments on the tests wouldl be nice. ;)
<jtv> henninge: can do
<jtv> Done.
<jtv> henninge: in your branch, in _makeTranslationsDict (diff line 8): if translations can be None, maybe it should just default to Noneâmore conformant with factory practice.
<henninge> jtv: I have copied that behavior from make TranslationMessage.
<jtv> henninge: â¦where it has no business conforming with factory practice.  Here, it does.  :-)
<jtv> henninge: then, I wouldn't document the method as "making sure" the data is in a particular formatâit's a bit ambiguous as to what happens if it isn't.
<henninge> jtv: is it not consistent with factory practice to make up a value of None is passed in?
<jtv> henninge: it is consistent with factory practice to let a parameter default to None if you're going to accept a None and fill in a random value for testing.
<jtv> So (self, translations=None) instead of (self, translations)
<henninge> jtv: r=me btw
<jtv> henninge: great, thanks
* 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
<henninge> let me look at that
<henninge> jtv: ah, now I understand what you want. But this is an internal method, so the outward behavior is consistent as it is "makeSuggestion(translations=None)"
<jtv> henninge: ok
<henninge> But I can add the default paramter, np.
<jtv> henninge: moving on to the second lineâ¦  :-)
<henninge> :-P
<jtv> henninge: I wouldn't lead with "the right format" (IIRC the original code committed the same horror).  What matters is that you want the dict format.  Maybe an example would say it more clearly and in fewer characters: {0: 'foo'}
<gmb> henninge, I have a short branch to add to your queue if that's okay.
<jtv> henninge: I can't help but feel that trying to name all types that are a sequence but not a dict is a losing gameâ¦  do tuples really matter there?  What about ResultSets?  Maybe you could rearrange the test: if translations is None / elif isinstance(translations, dict) / else
<jtv> The "else" part would return dict(enumerate(translations))
<danilos> jtv, btw, are you reviewing henninge's branch?
<henninge> danilos: he is, shouldn't he?
<danilos> sure :)
<jtv> danilos: yesâ¦ to keep him busy while we didn't hear from you :-)
<jtv> It's eod here, but that also means it's too late to do other things.  :)
<henninge> gmb: that is ok
<danilos> henninge, jtv was only supposed to be out by now :)
<henninge> ;-)
<henninge> danilos: he basically begged me to be allowed to review it.
<henninge> I tell him "It's Danilo's!"
<henninge> He says "Oh no, please, please, not him!"
<danilos> henninge, hehe, don't worry, I don't mind too much :)
<jtv> gee, how lucky (!)
<jtv> henninge: there's no more need to do a separate setSequence after a makePOTMsgSetâ¦ just pass the sequence number to makePOTMsgSet.
<jtv> henninge: oh, and when you use assert, there has to be an explanatory string.
<henninge> gmb: bug 618606 ?
<_mup_> Bug #618606: Add bug_subscription_level to the BugSubscriptionModel <story-better-bug-notification> <Launchpad Bugs:In Progress by gmb> <https://launchpad.net/bugs/618606>
<gmb> henninge, Sure. Sorry, missed your first ping.
<henninge> gmb: would it be too much for this branch to move BugNotificationLevel to lp.registry.enum?
<henninge> Why is it in registry in the first place?
<sinzui> henninge, structural subscriptions
<henninge> sinzui: ok
<gmb> henninge, I have no idea. I'd be +1 on moving it to lp.bugs.enum, given that it's bug-specific. Either that or changing it to SubscriptionNotificiationLevel
<gmb> And leaving it in lp.registry.
<sinzui> henninge, SS are some what problematic because when fully implemented, they would know about bugs, specs, questions, and branches
<henninge> sinzui: so it should be lp.coop/inter/shared.enum ... ;-)
<henninge> whatever we decide on ... or have we already?
<sinzui> henninge, registry is exempt because every app depends on it...but that is also why it is problematic
<sinzui> henninge, I think the db scheme is the origin of the problem maybe
<henninge> sinzui: Is there a problem with putting it in lp.registry.enum ?
<sinzui> No
<henninge> cool ;)
<henninge> gmb:  ^
<gmb> Righto.
<gmb> sinzui, Should it be renamed to something more generic so that it can be reused across different SS targets?
<gmb> (e.g. SubscriptionNotificationLevel)
<sinzui> good idea
<henninge> gmb: good idea
<henninge> ;)
<gmb> Cool
<gmb> Will do.
<gmb> I'll file a bug about renaming the attributes, since that's more work.
<gmb> s/attributes/attributes and db fields/
<gmb> Ah.
<gmb> henninge, sinzui: Maybe the rename doesn't make sense after all. There's also a BlueprintNotificationLevel.
<gmb> So the rename makes sense if we're converging on one enum.
<sinzui> :(
<gmb> But I think that's out of scope for this branch.
<gmb> I'll do the move though and update the import sites.
<sinzui> gmb: We can revisit this in a few weeks when I need to look at ACLs for privacy. Maybe I can introduce a generic form for all artefacts
<gmb> Right
<henninge> gmb: r=me ;)
* 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> henninge, Thanks.
<jtv> henninge: did you get my comments about the docstring, and about rearranging the conditions in _makeTranslationsDict?
<jtv> henninge: could you please re-run "make lint"?  There's plenty of stuff in the diff that it would have complained about, so I think your previous run was incomplete somehow.
<henninge> jtv: what was that about rearranging conditions?
<jtv> henninge: Naming all types of sequence that might make up a non-dict translations list is probably a losing game: is it really necessary to accept tuples?  What about ResultSets?  If it's really needed, consider rearranging the conditions: "if translations is None / elif isinstance(translations, dict) / else" (where the "else" part returns dict(enumerate(translations)))
<henninge> jtv: ah, god idea.
<jtv> well I wouldn't go _that_ farâ¦
<jtv> henninge: I've got a ton more, a lot of it clearly not your doing.  I'll paste in the review.
<henninge> jtv: well, I was aware that I would not catch all iterables but that's what the assert was for. But this is just as good.
<henninge> s/but/and/
<jtv> henninge: note that the assert would have needed an explanatory string btw.
<jtv> And speaking of strings, I suppose if we treat everything else as an iterable, then someday we're going to pass a string instead of a list by mistake and get ['f', 'u', 'n', 'n', 'y'] results.
<jtv> So that assertion may still be worthwhile.  :-)
<henninge> jtv: but then I'd be checking isinstance(translations, dict) twice ... :(
<henninge> I think I'll return to the first structure and just add the comment to the assert.
<jtv> henninge: well the assertion on dict type wouldn't be needed, would it?
<henninge> which assertion were you referring to?
<jtv> Sorry: I mean, you can assert on the right typeâbut in the clause for sequences.  So assert isinstance(translations, (list, tuple)).
<jtv> Then if we find that e.g. ResultSets need supporting, all we need to do is add that type to the assert.
<jtv> (I think the error message would be clearer that way than with the current structure)
<henninge> huh, so now we *are* back to the original structure? I am confused.
<jtv> henninge: no, I'm referring to the original structure as the current one.
<jtv> henninge: I can't see what you have locally.
<henninge> you cannot? :-o
<jtv> henninge: so I'm saying it would be good to do something likeâ¦  if translations is None: return {0: makeString()} ; elif isinstance(translations, dict): return translations ; else: assert isinstance(translations, (tuple, list)); return dict(enumerate(translations))
<henninge> Hm, I don't see the big difference/advantage in that, though.
<jtv> henninge: think about what happens if it goes wrong, e.g. because you're getting a ResultSet.
<jtv> Think about the failure mode.
<henninge> ok, we'd be getting something we don't expect.
<danilos> jtv, henninge: is there any reason not to define one single interface (i.e. "only takes a dict")
<henninge> jtv: but OTOH we are not checking that the elements of the list are really strings.
<jtv> In the original code, you'd get a failure "this is not a dict."  Which would be a fairly silly thing to expect it to be.  In the rearranged version, you'd get a failure "this is neither a list or a tuple."  That would instantly make you think: "clearly there are several things this is allowed to be, and clearly a ResultSet is a lot _like_ a list or tuple."
<danilos> (I know I am jumping right into the middle of the discussion, but I'd go with simplest possible APIs)
<henninge> danilos: well, lists are passed in many places and this seemed easier than checking all call sights.
<henninge> site
<henninge> sites
<jtv> s
<henninge> jtv: ok, let's end this here. ;-)
<jtv> uh-ohâ¦ are we talking gunfight in the street?
<henninge> jtv: nah, this is just taking longer then it is worth. Any idea for a good failure message for the assertion?
<henninge> "Translations must be in a dict, list or tuple."
<jtv> henninge: er... "Expecting either a dict or a sequence"?
<jtv> (so as to document intent, not what the code does)
<henninge> ok
<jtv> I pasted the rest of my notes in the MP.
<henninge> jtv: here is the lint output before any changes, btw.
<henninge> http://paste.ubuntu.com/478885/
<jtv> henninge: maybe some messages are being suppressed then.
<henninge> I think the linters expectations on blank lines need to be fixed and I don't know what form to raise exceptions in ...
<henninge> jtv: thanks
<henninge> for the review
<henninge> have not seen it yet, though :-P
<jtv> henninge: it's rather a lot, and mostly probably not about stuff you did so feel free to push back a bit
<sinzui> EdwinGrubbs, I shot myself in the foot with a bad branch and I used my own project for the test.
<sinzui> EdwinGrubbs, Can you read through this bug and then talk to me about whether my proposed fix will help undo future damge: https://bugs.edge.launchpad.net/launchpad-registry/+bug/522599
<EdwinGrubbs> sinzui: I'm looking at it now.
<sinzui> EdwinGrubbs, I am ready to mumble and explain how I screwed up
<sinzui> EdwinGrubbs :Store.of(bugtask).remove(bugtask.conjoined_master)
<lifeless> sinzui: hiya
<sinzui> hi lifeless
<salgado> anybody willing to review a small branch for me? https://code.edge.launchpad.net/~salgado/launchpad/vostok-no-external-redirect/+merge/32623
<sinzui> salgado, yes
<salgado> sinzui, yay, thanks!
<sinzui> salgado, I have a 25 line branch I wanted reviewed. the mp will show up in a few minutes. Can you take a look at it when it does.
<salgado> sinzui, sure
<sinzui> salgado, Is your branch intended to ensure any reuse of lp logic does not take the user to lp.net?
<salgado> sinzui, yes, but it's a nice-to-have safety net anyway.  we only had to remove that in Launchpad because we have multiple hosts we can redirect people to
<sinzui> salgado,  I always think links to attachments/changelogs as redirects to the librarian. Will these links still work?
<salgado> sinzui, in vostok we probably won't have attachments, but we'll certainly store other things in the librarian, so we might have to change that later to allow redirects to the librarian as well
<salgado> but we can worry about that when we need it
<sinzui> Will API calls work? Is API not a part of vostock
<sinzui> That may be moot. We never redirect to API
<salgado> this shouldn't affect the API
<sinzui> salgado, r=me. I have no other questions
<salgado> thanks sinzui.  where's yours?
<sinzui> salgado, https://code.edge.launchpad.net/~sinzui/launchpad/delete-conjoined-bugtask-1/+merge/32796
<sinzui> salgado, I should note that Edwin-lunch fixed by project data. But he too saw a bugtask targeted to trunk with no project bug task.
<salgado> sinzui, r=me
<sinzui> thanks salgado
<rockstar> sinzui, since you just got a review, would you please do a (small) review for me? https://code.edge.launchpad.net/~rockstar/launchpad/no-recipes-on-private-branches/+merge/32799
<sinzui> I can start in a few minutes
<sinzui> rockstar, I had a few questions. Your branch is really fine as it is, but I want to understand if there is something I am missing.
<rockstar> sinzui, cool.
<rockstar> sinzui, so, I haven't used the formatters in the past because we want to use javascript on this feature (_before_ we call the feature done) and the formatters don't yet let you specify an id.
<sinzui> good reason
<rockstar> sinzui, I'll use the with person_logged_in
<sinzui> Thanks for the explanation. I will update the review with my new insight.
<sinzui> rockstar, r=me
<rockstar> sinzui, thanks.
#launchpad-reviews 2010-08-17
<thumper> mwhudson: if you could take a look at http://bazaar.launchpad.net/~thumper/launchpad/new-bzr/revision/11058
<thumper> mwhudson: I've reviewed the rest of the bzr 2.2 upgrade branch as it was done by abentley
<thumper> mwhudson: but that revision is the only non-merge one
<thumper> from me
<thumper> I'm running it throug ec2 now (test only)
<mwhudson> thumper: assuming it makes the tests work, it looks ok to me
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/new-bzr/+merge/32845 is the entire MP
<thumper> the tests all pass with flying colours
<mwhudson> thumper: reviewed, then
<thumper> ta
* jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> henninge: approved
<henninge> jtv: thank you very much! ;)
<jtv> np
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jtv, https://code.edge.launchpad.net/~jtv/launchpad/recife-approveSuggestion/+merge/32848 is it?
<jtv> gmb: yes, that's it
<gmb> Righto
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * gmb should really write an x-chat plugin to deal with the topic in this channel.
<jtv> We could write a chat server for Launchpadâ¦
<jtv> gmb: I do unfortunately have to run off very soon.
<gmb> jtv, No worries, we can do any back-and-forth by email.
<jtv> gmb: thanks
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/milestones/+merge/32855
* lifeless changed the topic of #launchpad-reviews to: On call: gmb || reviewing: jtv || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: still getting used to adding more comments to unit testsâ¦ I'd like them to be shorter and have simpler subject material so I could make the code speak for itself, but doesn't always work.
<gmb> jtv, Yeah; All I'm after is a comment (or docstring; I prefer comments but have no reason for it) at the start of the test telling me the expected behaviour.
<jtv> gmb: doing that now
<gmb> # If the foo is prodded the frobnob goes boing.
<gmb> And so on.
<gmb> jtv, Cool.
<jtv> thanks for the review btw.
<gmb> np
<gmb> jtv, Thanks for the awesome cover letter that saved me a lot of confusion :)
<jtv> Glad to know the effort's not wasted!
<gmb> Indeed.
* gmb changed the topic of #launchpad-reviews to: On call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> gmb: thanks
<gmb> np
<cjwatson> gmb: I'd like to have a pre-impl chat about bug 619152
<gmb> cjwatson, I think you'd be better off chatting to one of { bigjools, jelmer, noodles775 }; I have this much context on Soyuz: ><
<cjwatson> bigjools: ^- would you have a moment?
 * StevenK idly notes he isn't in that list :-P
<gmb> StevenK, I assumed that you were sleeping or otherwise engaged.
<StevenK> Trying to stop working. Failing. :-/
<cjwatson> well, this is *cough* "pre"-impl, I have a branch and would quite like a sanity check that it covers everything I need; especially as running ec2test involves some pile of local setup I haven't managed to do
<gmb> cjwatson, *definitely* one for the Soyuz team, then. Sanity is defined differently once you enter lp.soyuz.
<wgrant> No, it's undefined :/
<cjwatson> lp:~cjwatson/launchpad/dpkg-xz-support-619152
<jelmer> cjwatson: Hi
<cjwatson> hi
<jelmer> cjwatson: I can do a review and run your branch through ec2 if you like.
<cjwatson> that would be awesome
<jelmer> cjwatson: Looks great
<jelmer> cjwatson: I'm wondering though how much policy enforcement we should be doing in nascentuploadfile.
<jelmer> It would be nice if we could do this sort of testing by enforcing a few lintian errors, as dak is doing.
<jelmer> bigjools: Is there a particular reason we aren't doing more checking of control fields in nascentupload?
<jelmer> cjwatson: Before this can land, we also need to get a newer version of dpkg deployed on the Launchpad servers.
<cjwatson> jelmer: the policy enforcement there is just a copy of what we used to do when the other compression formats were new
<cjwatson> jelmer: (not that this really answers your point as such)
<cjwatson> I actually literally resurrected it from old commits
<cjwatson> jelmer: would you like me to take care of getting a new dpkg in place?
<cjwatson> may take a while ...
<jelmer> cjwatson: Ah, sorry. I wasn't aware there had been something like that previously.
<jelmer> cjwatson: I can take care of it if you like; I need to upload some updates to the Launchpad PPA anyway.
<jelmer> cjwatson: If you can propose a merge into lp:launchpad/devel I'll do a proper review as well landing this.
<cjwatson> OK, either way.  Do you know the stuff that's required for hardy-cat backporting?
<jelmer> Hmm, I hadn't considered hardy-cat actually, was just thinking about the launchpad servers (which do unpacking).
<cjwatson> hardy-cat is how dpkg gets to them
<wgrant> Or is Lucid happening soon enough that that's irrelevant?
<cjwatson> lucid doesn't have dpkg 1.15.6, so it would be an issue either way
<jelmer> wgrant: Lucid's dpkg is too old as well if I understand correctly. (1.15.5 vs 1.15.6)
<cjwatson> the backport diff to hardy-cat is http://paste.ubuntu.com/479368/; there was an extra upload by LaMont after that as well, but that was a backport from 1.15.6 so I think it can be discarded
<cjwatson> http://paste.ubuntu.com/479369/ is the complete diff from hardy to hardy-cat currently
<jelmer> do the launchpad servers run hardy-cat as well, not just the buildd's ?
<cjwatson> yes
<cjwatson> gah, dpkg gets harder and harder to backport - now I have to care about gettext
<cjwatson> maybe it would be easier just to cherry-pick the change in question
<jelmer> how intrusive is the patch?
<cjwatson> not hugely
<cjwatson> will still need to get xz-utils into hardy-cat though
<cjwatson> hm, there was some packaging awkwardness there, I recall
<cjwatson> should be OK with maverick's version
* mars changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> hi gmb, mars
<gmb> jelmer, Howdy.
<jelmer> can I add a simple soyuz branch to either of your queues?
<gmb> jelmer, How small is it?
<gmb> I ask because I need to go afk for 1:30 at about 14 UTC
<jelmer> It's 458 lines, but pretty much only adds tests for existing code
<gmb> jelmer, Okay, I'll take a look now. Diff me.
<jelmer> gmb: thanks - mp is @ https://code.edge.launchpad.net/~jelmer/launchpad/nascentuploadfile-tests/+merge/32874
* gmb changed the topic of #launchpad-reviews to: On call: gmb, mars || reviewing: jelmer, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jelmer, Review sent; quite a lot of cosmetic changes needed (callsite formatting and tests that need a comment).
<gmb> But the code is sound; nice work.
<jelmer> gmb: Thanks for the review.
<gmb> np
 * gmb goes off review for a while for travelling.
* gmb changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> mars: I have a couple revisions to an already-approved branch that need review: http://bazaar.launchpad.net/~benji/launchpad/bug-597324/revision/11117 and http://bazaar.launchpad.net/~benji/launchpad/bug-597324/revision/11118
<jelmer> What should the per-line imports look like exactly? The python style guide doesn't appear to've been updated yet.
<mars> jelmer, mister review master wrangler back may know
<mars> 'bac' even
<bac> jelmer: no, it hasn't
<mars> benji, sure, please add yourself the queue
* benji changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jelmer: we agreed that single imports remain on one line, and that more than one require multiple lines, one per
<bac> jelmer: we did not discuss alphabetizing, so i assume that criterion has not been relaxed
<jelmer> should the closing ")" be on a separate line?
<bac> yes
<jelmer> ok, that was the main thing I wasn't sure about - thanks
<bac> with a trailing comma on the last item
<bac> unfortunately, the ) on a separate line irritates our linter
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [benjiÂ²] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> bac: I missed that decision; was it in the email thread [Launchpad-dev] RFC: format lists one-per line ?
<bac> benji: it was in the reviewers meeting and there was an email discussion.  i think you contributed to it.
<bac> benji: i do need to send an email, though, finalizing the decision
<benji> bac: right, I hadn't seen a finalization email; thanks
<bac> benji: that's b/c i am slack
<benji> heh
<mars> Ursinha, merge is ready: https://code.edge.launchpad.net/~mars/qa-tagger/lazr-conversion/+merge/32878
<Ursinha> mars, thanks!
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: benji || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> mars: should I add another MP for those revisions?  I'm not sure how to handle the situation when a branch has been approved and then more changes have to be made to it.
<mars> benji, usually we just ask for another review explicitly for the changed revisions
<mars> and add it to the existing MP
<mars> benji, what is the MP?
<benji> mars: https://code.edge.launchpad.net/~benji/launchpad/bug-597324/+merge/30829
<mars> thanks
<mars> benji, I think you need a new MP.  The old one was not updated with your latest changes
<mars> it looks like you can indeed keep hacking on the same branch
<mars> benji, there is also a big revision gap: the MP ends at 112, and you asked for 117 and 118 to be reviewed.  Where did the others go?
<benji> mars: one was a merge, two were trivial, and one should have been included (and is in the MP I'm writing right now)
<mars> benji, ok, then I think a new proposal should just Do The Right Thing
<benji> mars: here you go: https://code.edge.launchpad.net/~benji/launchpad/bug-597324/+merge/32884
<mars> thanks
<mars> benji, you may want to try 'bzr qannotate' to browse publication.py and figure out what the intent of the 'appears to be a XSRF countermeasure' thing is
<mars> or bzr explorer
<benji> I'll look at that now.
<mars> benji, db-devel is the right target?
<benji> darn; no it's not
<benji> mars: let me delete and re-create the MP so it has the right target
<mars> ok
<benji> mars: https://code.edge.launchpad.net/~benji/launchpad/bug-597324/+merge/32886
<mars> ok
<benji> mars: did you mean "bzr qannontate" or just "bzr annotate"?
<mars> benji, a method has 40 lines of comments to 3 lines of code - I don't know what to say
<benji> mars: ?
<mars> benji, there is a qbzr plugin that gives gtk versions of some bzr commands - qannotate and qlog are really nice versions of the set
<benji> mars: oh, the referer exception  thing
<benji> ok; I'm more of a text kind of guy :)
<mars> benji, no, the text is good
<mars> it's the very fact that a small book must be written to explain the purpose of a single call to 'return' that I find... strange
<mars> sounds like something NASA would do
<benji> heh
<mars> benji, your comments there are fine.  I'm surprised that that one block of code has so many XXXs and so much text
<benji> mars: speaking of NASA, I'm sure you've read this http://www.fastcompany.com/node/28121/print; if not, you'll enjoy it
<benji> yeah, it's a bit of an oddity
<benji> maybe we should start a contest for the most well-documented bare return
<mars> benji, yep, that article is what inspired my NASA comment :)
<benji> cool
<benji> I like to read that every year or so; lots of good stuff in there
<mars> for me that article perfectly demonstrates one of the three faultlines (or continents) in the software world: hackers, enterprise, and academic
<mars> they all hate each other
<mars> and insist that if the others just did it their way, the world would be a better place :)
<benji> heh
<jml> mars, "they"?
<mars> jml, ?
<mars> 'they all' => 'they'
<mars> maybe?
<jml> mars, surely "we" would be more appropriate :)
<mars> :D
<benji> I would hope that the NASA folks realize that very few people can afford to put 260 people on building something about the size of LP.  I'd also hope that the hacker type (which I count my self as) realize that when lives depend on your work product you have to be quite bit more particular about processes.
<jml> mars, unless you're hiding on some uncharted fourth continent :P
<benji> heh
<benji> journey to the center of mars
<mars> benji, then I count thee as enlightened
<benji> ok, mars: I was able to verify that the referer-rejection code is indeed about XSRF; I'll update the comment to be a statement instead of a question.
<mars> cool
<mars> bac, are we transitioning slowly to multi-line imports, or is someone going to nuke the source tree with a macro over the weekend?
<bac> mars: unclear
<bac> mars: the code team seemed to indicate they were going to rush off and do theirs quickly
<bac> mars: no one volunteered for an apocalyptic branch
<mars> bac, I'm reviewing benji's branch, and he worked with the imports in test_publication.py.  Should the ones he worked with be updated to multiline as part of the policy?
<mars> not suggesting the whole file be updated
<bac> mars: i'd suggest not requiring it until we get some tool support.
<mars> perfect
<mars> thanks back
<mars> thanks bac
<mars> twice today, argh
<bac> there is an emacs 'sort-imports' and i think vim has something similar.
<bac> they should be updated
<mars> personally I would like a Python or sed script
<bac> mars: that too
<mars> benji, nice test suite addition
<benji> cool
<mars> benji, done, one errant comment, r=mars with the fix
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> off to lunch!
<benji> mars: thanks
<EdwinGrubbs> mars: can I get a small branch reviewed? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-fillTeamParticipation-timeout/+merge/32895
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> mars: Could you review https://code.edge.launchpad.net/~brian-murray/launchpad/x-launchpad-bug-modifier-follow-on/+merge/32900
<mars> Xorg server crash
* benji changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [Edwin, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> I hate those.
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: lunch || queue: [Edwin, bdmurray, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: Edwin || queue: [bdmurray, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> mine is at https://code.edge.launchpad.net/~benji/launchpad/bug-580035/+merge/32615
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: Edwin || queue: [bdmurray, benji, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> cool, thanks
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: bdmurray || queue: [benji, jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> jcsackett, feel free to put your name in the topic and leave a PM with the merge proposal link
<jcsackett> mars: okay, thanks.
* jcsackett changed the topic of #launchpad-reviews to: On call: mars || reviewing: bdmurray || queue: [benji, jml, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> well, PM isn't right, just a message
<mars> either myself or a passer-by will pick it up
<jcsackett> mars: okay.
<jcsackett> mars: mp is at https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820
<jcsackett> mars: thanks so much!
<mars> no problem
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: benji || queue: [jml, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> benji, ping, looks like bug-580035 has a merge conflict
<mars> benji, merged with db-devel again
<mars> EdwinGrubbs, btw, in your branch, for the routines that the SQL query replaced: was that their only callsite?  If so, can the routines be eliminated?
<EdwinGrubbs> mars: what do you mean eliminated?
<mars> EdwinGrubbs, ah, I guess not.  Lines 97-100 of the diff
<mars> those are probably used elsewhere
<mars> :)
<EdwinGrubbs> mars: oh, you mean whether hasParticipationEntryFor can be eliminated. good question.
<benji> ok, mars: the conflict is fixed and a new MP with the right target is at https://code.edge.launchpad.net/~benji/launchpad/bug-580035/+merge/32917
<EdwinGrubbs> it looks like theat is getting used in asserts and tests, so it would be a pain to remove.
<mars> thanks benji
<mars> EdwinGrubbs, then it still sounds useful to me
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: jcsackett || queue: [jml, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> alright, I'm done for the day.  Take care everyone
<leonardr> gary, can you take a quick look at https://code.edge.launchpad.net/~leonardr/launchpadlib/fix-test-failure/+merge/32925 ?
<gary_poster> on call leonardr
<leonardr> np, i'll wait. i have an ec2 test running and i'll come back tomorrow to see if this solved all the problems or only one
<gary_poster> approved leonardr
#launchpad-reviews 2010-08-18
* 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
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/32953
<lifeless> jtv: ^
<jtv> lifeless: I take it this is a review request?
<lifeless> yes please
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> lifeless: every time someone opens a new communication with the word "So," I'm tempted to replace it with "Therefore."
<jtv> lifeless: I wasn't aware of ValidPersonCache.  We used to have some database views for optimization of translation exports, and we got a tremendous speedup by getting rid of them.
<lifeless> jtv: yeah
<lifeless> I wasn't aware of it either
<lifeless> I think it came with the openid stuff, or something
<jtv> lifeless: what are the "# --" comments?
<jtv> ah, I see now
<lifeless> clarity in a long function which is simple-but-extended
<mwhudson> i think that validpersoncache was actually a cache at some point
<mwhudson> but that changed, maybe during the account/person split
 * mwhudson may very well be wrong, of course
<jtv> lifeless: I can't say I care much for the mixed style of passing arguments: chucking the first one in right after the opening parentheses but then tab-indenting the following ones.  Could you align the first argument with the following ones in line 37 of the diff?  I'd also appreciate a matching change in line 52.
<jtv> conditions = AND(
<jtv>     conditions,
<jtv>     Or(...
<jtv> (I gather the mixing of SQLObject AND and Storm Or is harmless then)
<lifeless> the AND is a mistake
<lifeless> I'll change it
<lifeless> the indenting shows grouping, I think its much less clear without it
<lifeless> aim for readability
<jtv> That's just the thing.  I think this is more readable, not less:
<jtv> conditions = And(
<jtv>     conditions,
<jtv>     Or(
<jtv>         Account.status == None,
<jtv>         Account.status == AccountStatus.ACTIVE))
<lifeless> I can do that
<lifeless> that wasn't what you seemed to be asking for
<jtv> Well I figured you'd find out soon enough that you couldn't take the first argument to the left, and so either had to go to a new line or move the following argument to the right.
<jtv> (I prefer the former, but would also accept the latter)
<lifeless> jtv: I don't see the point of moving conditions to a separate line
<jtv> lifeless: it makes it much easier to see that the line needs continuation.
<lifeless> really?
<jtv> To me, very.  It also makes it easier to see that in terms of grouping, the "conditions" is on par with the "Or."
<lifeless> so
<lifeless> I will do this
<lifeless> but if we do it consistently in the function
<lifeless> it will add 30% or so to a long function
<lifeless> thats unpleasant
<lifeless> I presume you're looking at the diff only atm ?
<jtv> I also looked at the source file; it could do with some cleanup, yes.
<lifeless> I think its definitely cleaner on the valid = (
<lifeless> line
<lifeless> I remain ambivalent at best on the conditions section
<lifeless> but done, for the stuff in the diff
<jtv> Thanks.  I think the problem with the length of the method is just exposing that the method is too long.
<jtv> ISTM prepopulate_person really just wants to be driven by a type â attribute name dict.
<jtv> (Although aliases may get in the way)
<lifeless> rev 11371 pushed for your pleasure
 * jtv shudders with delight
<lifeless> in future work I would like to push the adapters down into the base property definitions
<lifeless> and that way the code is shared
<lifeless> there was just not-quite-enough tuits in doing this orginally
<jtv> The adapters being the bits that figure out where each result column goes?
<jtv> lifeless: r=me.  May you find a nice, round tuit soon.
<lifeless> yes
<lifeless> thank you
<lifeless> I'll finish chatting with stub about this before landing
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> lunchtime!
* jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> 'morning jtv
<jtv> hi jelmer
<jtv> Thank god you're here.  It's been a madhouse.  There was a branch to review.
<jtv> A very small one, but still.
<jelmer> :-)
<jelmer> jtv: I have a branch up for review if you have time.
<jtv> jelmer: that's what ocr is for.
* jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: -, jelmer || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: -, jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> jtv: Just trying to be polite :-)
<jelmer> jtv: The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa/+merge/32908.
* jtv changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: jelmer, jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> jelmer: in line 26 of the diff, I take it self._dict is a dictâ¦ do you really want to iterate over the keys there?  If so, better make it explicit.
<jelmer> jtv: Yep, that's indeed the intention. I'll change it to use iterkeys().
<jtv> thx
<jtv> jelmer: are you using a list of tuples for the user-defined fields?  Wouldn't a dict make more sense there?
<jtv> Although I can see that would upset some ordering in the tests, so you'd want to use assertContentEqual instead of assertEqual there.
<jelmer> jtv: The order in the control field could matter; we discard that information at the moment because we convert it to a dict, but in general order is preserved in Debian control files.
<jtv> OK
<jtv> jelmer: then another question: don't you need any kind of escaping?  For multi-line text attributes for instance?
<jelmer> jtv: That's already done for us, as the input file would have the same escaping.
<jelmer> Perhaps I should add some tests to verify that though.
<jtv> I'd guess you'd want a test to verify that problems with this are caught on input.
<jtv> jelmer: btw kudos for breaking out test_preserves_order and test_files.  I'd also break out the "ignores None" part though, because it's a bit implicit.
<jtv> jelmer_: you blinked
<jtv> don't know if you got this earlier:
<jtv> (16:04:00) jtv: jelmer: btw kudos for breaking out test_preserves_order and test_files.  I'd also break out the "ignores None" part though, because it's a bit implicit.
<jtv> (time is UTC+0700)
<jelmer> jtv: Evolution's memory usage is bringing my laptop down on a regular basis, it's getting a bit ridiculous.
<jelmer> jtv: Thanks, I hadn't seen that.
 * jtv stopped trying to use evolution after a few machines decided spontaneously to wipe his calendars and ensure somehow that they couldn't be set up again
<jtv> When did catastrophic data loss become acceptable outside the windows world?
<jelmer> Much as I'd like to avoid it I'm storing my private calendar in Google calendar these days.
<jml> jelmer, linux needs a good desktop calendar app
<jtv> jelmer: sorry, got distracted and completely forgot to complete your review.  All I had left was some simple layout issues.
<jelmer> jtv: No worries
<jelmer> jml: It also needs a good email client, still.
<jtv> jelmer: in lib/lp/archiveuploader/nascentuploadfile.py line 334 or thereabouts, could you normalise the list comprehension?  Something like:
<jml> jelmer, I guess mutt is still the best.
<jtv> jelmer:
<jtv> return [
<jtv>     (field, contents)
<jtv>     for (field, contents) in control
<jtv>     if field not in self.known_fields]
<jtv> jelmer: also, if it doesn't cause conflicts in other branches, could you reformat the __all__ in lib/lp/archiveuploader/tests/__init__.py to our standard one-item-per-line list notation?
<jelmer> jtv: Sure, will do.
<jtv> thx
<jtv> jelmer: there's also one in lines 166â167 of the diff.  A bit hard to figure out the nesting the way it's currently grouped.
<jelmer> jtv: do we have a style guide for list comprehensions? What you're proposing looks reasonable, but it'd be nice to have something to fall back to.
<jtv> jelmer: I know we used toâ¦  In any case I think it's common sense not to break lines in unexpected places.  Looks a bit like the twins in FC Knudde completing each other's text balloons.
<jelmer> hehe
<jtv> jelmer: I'm so glad I can talk to someone who gets that one.  :-)
<jtv> jelmer: in line 178 of the diff, there's a weird empty line in the docstring for extend.  Did you plan to write something more there?
<jelmer> jtv: Nope, that's a formatting glitch. FIxed.
<jtv> thx
<jtv> jelmer: Another mis-formatted list in lines 229â245 of the diff.  Maybe better put it in a variable of its own.
<jtv> (Did you run "make lint" btw?)
<jtv> Same for 266â291
<jelmer> "make lint" is unfortunately quite chatty about some of the files that I've touched
<jtv> I'm not surprised.  :)  Try to do some drive-bys where you can, and make sure none of your own work is implicated.
<jtv> Meanwhile, branch approved.
* jtv changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> Will do, thanks for the review.
<jtv> no worries, have a good one!
 * jtv is approaching eod
<jelmer> jml: wow, this is one big branch :-)
<jml> jelmer, yeah. sorry.
<jelmer> gmb: hi
<jelmer> gmb: I've updated the branch you reviewed yesterday and was wondering if you could have another look.
<jelmer> jml: is there any reason you use Branch.open_containing vs Branch.open ?
<jml> jelmer, I don't, it's preserved from the original. I reckon we should use Branch.open & would happily change the code.
<jelmer> jml: --not-really doesn't really explain to me what it's not doing
<jelmer> What about --no-tests or --test-setup-only ?
<jtv> jelmer: I have to run now, but was hoping for a review anyway: https://code.edge.launchpad.net/~jtv/launchpad/bug-617431/+merge/32986
<jml> jelmer, I'm going to delete the whole thing. it's buggy anyway
<jelmer> jtv: Sure, no problemo
<jtv> thanks!
<jtv> Still updating diff.  :(
<jtv> jelmer: in case the cover letter doesn't make it clear, we're already using the msgfmt program (in tests as well as in production) so msgunfmt, which my test uses, should be available.
<jelmer> jml: ok, that renders that concern moot then. Those are the only two real points I have. I like the class splitup and testing ftw.
<jelmer> jtv: Ok
<jml> jelmer, cool, thanks.
<jtv> jelmer: argh, the diff is polluted because I branched from devel and proposed for production-devel.  Hang on.
<jtv> jelmer: this one should fare better â https://code.edge.launchpad.net/~jtv/launchpad/pd-bug-617431/+merge/32991
<jelmer> jtv: thanks
<jtv> gargh.  the push got interrupted so the diff is still incomplete
<jtv> re-pushing...
<jtv> Ah no, it was entirely my fault.  A different push got interrupted.
<jelmer> I'm going for lunch now, will have a look afterwards.
<jtv> jelmer: ok
<gmb> jelmer, Sure, I'll take a look in  the next half hour
<jelmer> gmb: Thanks for the re-review.
<gmb> np
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jml || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, - || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<mars> jelmer, thanks for reviewing jml's branch
<jml> mars, did you write the daemonize function in ec2test remote?
<mars> jml, no, I thought gary did
<mars> jml, argh, the file history is broken!  Maybe bzr explorer can jump the gap
<jml> mars, no matter, I think I'm figuring out the reason why something is so without having to consult the original author
<sinzui> EdwinGrubbs, jelmer: can either of you review my https://code.launchpad.net/~sinzui/launchpad/delete-team-2/+merge/32948 branch?
<EdwinGrubbs> sinzui: sure
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: does the test runner no longer require the test_suite to find tests?
<sinzui> EdwinGrubbs, it does not
<bigjools> salgado-lunch: -1 on your last change, I use those policies on the command line for testing
<EdwinGrubbs> sinzui: r=me, login_person and login_celebrity are not in alphabetical order
 * sinzui will fix
<sinzui> and make lint happy
<leonardr> jelmer, edwin: please add https://code.edge.launchpad.net/~leonardr/lazr.restful/integrate-optimized-len/+merge/33023 to the queue
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> bigjools, you'll still be able to use them; they'll just live closer to the tests now.  although I'd have to change that XXX into a comment explaining why we want the policies registered all the time
<bigjools> salgado: ok that's fine if we can still use them from the command line
<jml> jelmer, thanks for that review, btw
<jml> sinzui, actually, you might prefer login_as to login_person
<sinzui> jml, Yes, i do. I think I want to update all the registry tests to use the new login* hlpers
<jml> jelmer, it turns out there was a serious bug preventing it from successfully daemonizing. I've written a fairly hideous test for this and fixed the issue. Once I've verified that it works, could I ask you for a review of the interdiff?
<jml> (or now, if you don't mind reviewing possibly broken code)
<jelmer> jml: yeah, I think so
<jelmer> Edwin: Will you have time to look at leonardrs mp?
<jelmer> (I'm going to EOD soon, don't want to promise to take on too much work before then)
<EdwinGrubbs> jelmer: sure, I can do it.
<jml> jelmer, http://pastebin.ubuntu.com/480006/
<jml> jelmer, the main difference in the code is passing in & using filenames rather than file-like objects.
<jelmer> jml: *nod*
<jelmer> 'morning thumper
<jelmer> thumper: when you have a moment, any chance you can take a look at https://code.edge.launchpad.net/~jelmer/launchpad/always-import-link/+merge/32668 to check whether my approach is the right one for bug 504868?
<jelmer> I'm not looking for a review necessarily, more of a post-pre-imp-call :-)
<EdwinGrubbs> leonardr: buildout couldn't find lazr.batchnavigator=1.2.2
<leonardr> EdwinGrubbs: it's not released yet. check out lp:lazr.batchnavigator
<jcsackett> mars: ping
<sinzui> jelmer, EdwinGrubbs: I have a very small branch for review: https://code.launchpad.net/~sinzui/launchpad/remove-mailman-beta-key/+merge/33042
<leonardr> edwin: actually, can you stop reviewing my branch? i've made a discovery that indicates it will screw up launchpad, so i don't want to land it now
<EdwinGrubbs> leonardr: wow, I'm glad I hadn't approved it, even though I hadn't found anything wrong.
<leonardr> edwin: it's a problem with storm, not with this branch
<EdwinGrubbs> sinzui: I'll look at it
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: oh, Francis already approved it with a comment.
<sinzui> oh
<mars> hi jcsackett
<jcsackett> hi mars. i was wondering if you wanted to continue reviewing the mp i had from yesterday, or if i should hand it off to on-call.
<mars> jcsackett, if you uploaded a set of changes, you can hand just the changes off to the OCR
<mars> there should not be that many of them - a few dozen lines I would think?
<thumper> jelmer: ack, I'll get to it after breakfast
<jcsackett> mars: actually, lifeless introduced some good ideas that made it a larger change set, but i think i need to revise the cover letter and i'll pass it off to on call.
<mars> jcsackett, that sounds good
<jcsackett> cool. thanks mars.
* jcsackett changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: jtv, sinzui || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> Edwin-lunch, jelmer: https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820
<jcsackett> Edwin-lunch, jelmer: the mp is a little muddled b/c some review notes pushed the branch towards a much better solution than initially implemented. i revised the cover letter to reflect this.
<jelmer> jcsackett: Sorry, I EOD'd a couple of hours ago but forgot to remove my name. Hopefully Edwin will have time to do a review.
<jcsackett> jelmer: no problem, sorry to ping you. :-P
<EdwinGrubbs> jcsackett: I can look at it.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: jtv, sinzui || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> EdwinGrubbs: thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: jcsacket || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> jelmer: I wish we had a regenerate diff button
<thumper> jelmer: as that merge proposal you have has a lot of changes that aren't part of it
<lifeless> thumper: why a button
<lifeless> thumper: why not DTRT ?
<thumper> lifeless: because we don't regenerate the diff for all merge proposals on the target moving
<thumper> lifeless: as it is most often pointless
<lifeless> thumper: we could trigger an update if its stale when someone logged in views the page
<james_w> prerequisite moving?
<lifeless> james_w: trunk changing doesn't make the mp diff change
<thumper> lifeless: it may well do
<thumper> lifeless: for certain situations
<james_w> the prerequisite branch changing certainly can
<thumper> james_w: agreed, and we don't do that yet
<jelmer> thumper: Should I try to resubmit ?
<thumper> jelmer: perhaps recreating targetting devel?
<jelmer> thumper, I'll have a look
<EdwinGrubbs> jcsackett|afk: I have a question about the team_memberships attribute.
#launchpad-reviews 2010-08-19
<jelmer> thumper: This one should be ok: https://code.edge.launchpad.net/~jelmer/launchpad/always-import-link/+merge/33070
<thumper> ta
<thumper> jelmer: while this solve the immediate problem, what I really want is a single view to register a new branch whether that be an import or mirror
<thumper> but this is fine for now
<jelmer> thumper: That makes sense. I guess all that's needed is a single URL in most cases?
<thumper> normally
<thumper> we could even have a validator that splits the URL for cvs imports
<thumper> so they go 'pserver...location module'
<thumper> and don't allow creation of remote or hosted branches that way
<thumper> but we'd need push instructions
<jtv> EdwinGrubbs: you still reviewing?
<EdwinGrubbs> jtv: sorry, no
<jtv> :-(
* 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
<jtv> mwhudson, can you review a branch for me?  I was hoping to get it CP'ed.  Yesterday's reviewer left me in the lurch, and now there's no OCR.
<mwhudson> jtv: i can take a look
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/pd-bug-617431/+merge/32991
<jtv> thanks
<thumper> anyone? https://code.edge.launchpad.net/~thumper/launchpad/fix-canonical-url/+merge/33079
<thumper> jtv
<thumper> ?
<jtv> thumper: ah what the hellâ¦ ok
<thumper> jtv: since I have you anyway, did you see my branch that changed a translations test?
<jtv> no?
 * thumper digs
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/fix-TestSharingMigrationPerformance/+merge/32828
<thumper> jtv: the gc.get_objects() was fouling up another translations test using bzrlib and lazy imports
<thumper> jtv: so in consultation with lifeless I rewrote the guts of the test
<thumper> jtv: do you want to check it over to make sure I didn't screw up?
<thumper> jtv: convenitently I'm using POFile to test the canonical_url fix too
<jtv> thumper: I'm looking at it
<thumper> ah fooey, spelling fail
<jtv> where?
<thumper> fingers not obeying brain
<thumper> convenitently
<jtv> sounds very catholic somehow
<thumper> too many Ts
<thumper> jtv: which are you looking at?
<jtv> thumper: the last one you mentioned
<mwhudson> thumper: does removing
<mwhudson>         if rootsite is None:
<mwhudson>             rootsite = 'mainsite'
<mwhudson> not affect anything?
<thumper> no, because by the time we get there, we have already set rootsite if it is None
<thumper> it is done further up the method
<jtv> thumper: your changed test for memory usage in the message-sharing migration script is stricter than the originalâit'll break if we should want to mention those tables in the SQLâor possible even foreign keys to them.  But if it works, I guess it's good enough for now.
<thumper> jtv: yes I know it is more strict
<thumper> jtv: but it passes now
<thumper> jtv: we can change the test if you need to reference the tables later
<thumper> jtv: sound OK?
<jtv> yes
<thumper> good
<jtv> thumper: back to your canonical_url branchâ¦ I really only have one concern: in lines 80â83 of the diff, are you going to take a lot of exceptions to get to LaunchpadLayer?  If so, is there any performance risk to that?
<mwhudson> thumper: obj_urldata.rootsite can't be None?
<mwhudson> i guess tests will find out
<thumper> mwhudson: I'm pretty sure not
<thumper> jtv: I'm not sure I understand
<jtv> thumper: you're catching ComponentLookupError thereâ¦ AIUI exceptions in python can be very very slow (although mostly from a cold cache I hope).  So do you expect these very often?
<thumper> jtv: the overhead isn't particularly big, and we'll get an error for any view for an object on the mainsite
<thumper> jtv: no I don't expect any performance issues here
<thumper> jtv: remember this is only used when someone specifies view_name
<jtv> Remember?  Is this something I knew before?
<jtv> Honestly, *what is the point* in having the little sticking-out plastic lip on a patch cable that makes the satisfying little click when you slot it in if *it doesn't help keep the plug in the socket*?
<jtv> I know, I know.  It gives the satisfying little click.
<thumper> it should keep it in the socket
<thumper> if it doesn't
<thumper> it is broken
<jtv> TIT.  Everything's broken.
<mwhudson> jtv: reviewed
<jtv> mwhudson: thanks!
<thumper> mwhudson: do you remember the process for getting a CP?
<mwhudson> thumper: no
<thumper> hmm...
<jtv> You branch off production-devel
<thumper> jtv: I branched of devel before the rollout
<mwhudson> thumper:  https://wiki.canonical.com/Launchpad/PolicyandProcess/EmergencyChange  maybe?
<jtv> You MP against production-devel, requesting "code" review and a CP one
<mwhudson> might be very out of date for all i know though
 * thumper looks
<jtv> You get approval to land on production-devel
<jtv> You land, then wait for the branch to percolate through to production-stable
<jtv> You register the request on LPS.
<jtv> thumper: the review type for the CP is "production-change."  I think you normally go to the RM for that.
<jtv> thumper: I reviewed your canonical_url branch btw
<thumper> ta
<jtv> thumper: btw if it wasn't clear, what I describe above is the current CP procedure ^^
<thumper> jtv: yeah, I'm just thinking that my change isn't really a CP candidate
<jtv> thumper: it needs a Critical bug and an Incident Report.
<thumper> it is just blocking landing other work which is making our SLA longer
<thumper> :)
<jtv> I don't think that qualifies, no.
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing:  || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* calvino.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
* StevenK changed the topic of #launchpad-reviews to: On call: - || reviewing:  || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: StevenK  || 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: - || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> salgado, can you do a quick review for me of https://code.launchpad.net/~leonardr/lazr.batchnavigator/build-url-without-length/+merge/33045 ?
<salgado> leonardr, sure, but I'm about to jump into a call, so it may take a while
<leonardr> salgado, np. anyone else, feel free to take it
<leonardr> mars, could i maybe get you to look at that branch? -^
 * leonardr just trying random people
<mars> leonardr, awesome doctest :)
<salgado> leonardr, r=me
<leonardr> salgado, thanks. mars, nevermind
<mars> k
<jsackett> EdwinGrubbs: when/if you have a moment, I believe I addressed your notes on my mp.
<EdwinGrubbs> jsackett: I've replied to your changes. BTW, do you preferred to be called Jon or J.C.?
<leonardr> mars, maybe you can review https://code.edge.launchpad.net/~leonardr/launchpad/optimized-length/+merge/33132 for me?
 * leonardr finds it hard to believe he's the only one with branches that need reviewing today
<jml> leonardr, don't worry, you aren't :)
<jml> #launchpad-reviews is only one of many ways to get a review.
<mars> leonardr, still need that review?
<leonardr> mars, yes please
<mars> leonardr, looks great, r=mars
<leonardr> great
<jcsackett> EdwinGrubbs: reviewed your notes and made the changes. and Jon or JC work, I answer to both.
<henninge> bac: Hi! I assume I have your rs on landing the imports reformatting branch? ;-)
<bac> henninge: yes.  i'm pondering how disruptive this will be
<bac> henninge: but week 1 is as good a time as any, right?
<henninge> bac: yes. Directly after roll-out would have been best, but ... oh well ;)
<bac> henninge: rs=bac
<henninge> But I agree that earlier in the cycle is better than later.
<henninge> bac: thanks ;)
<bac> henninge: thanks to you!
<lifeless> if you were to hook up a bzr merge plugin
<lifeless> then it would be painless anytime
<henninge> bac: I messed up the first ec2 run, the one that succeeded. It ran on plain devel. That ought to succeed :(
<bac> henninge: ok.
<henninge> bac: the real one is failing badly.
<henninge> bac: can you make anything out of this: http://paste.ubuntu.com/480557/
<bac> henninge: otp right now
<henninge> ok
<bac> henninge: what have you done?
<bac> henninge: did your process munge a bunch of stuff you didn't expect?
<henninge> bac: what do you mean?
<bac> henninge: that paste.  it's frightening
<henninge> yes, that's truw
<henninge> e
<EdwinGrubbs> jcsackett: your branch looks good. As long as it's passing the full test suite, it can be landed.
<jcsackett> it's passing most of what i've touched, EC2 will tell us the rest.
<jcsackett> thanks, EdwinGrubbs.
<jcsackett> rather, it's passing all of what i've touched.
<jcsackett> EdwinGrubbs: sorry about not posting the incrementals; i had assumed those were easily accessible from the mp, but hadn't really looked.
<EdwinGrubbs> jcsackett: I believe incremental diffs will be automated shortly, so you may end up learning a procedure that is immediately abandoned.
<jcsackett> EdwinGrubbs: won't be the first time, nor the last, i'm sure. :-)
#launchpad-reviews 2010-08-20
<jtv> Anyone up for a review?  https://code.edge.launchpad.net/~jtv/launchpad/recife-review/+merge/33183
<jtv> thumper, you perhaps? ^^
 * thumper isn't here
<thumper> gah
 * thumper waves hand in front of jtv
<thumper> thumper is not the reviewer you are looking for
<jtv> thumper: then who is?
<jtv> I bugged mwhudson last time, poolie isn't hereâ¦
<thumper> jtv: my guess would be stub is the closest to you
<jtv> I don't see him here.
<thumper> jtv: what does get_traits(template) do?
<jtv> It retrieves the TranslationSideTraits for the TranslationSide that template is on.
<jtv> TranslationSideTraits is a traits object to describe e.g. "if I'm translating on the Ubuntu side, what's the flag I should set to make a message the current one for that side?"
<jtv> And "how do I query for the message that's current on this side?"
<jtv> A TranslationMessage has two flags: "this is the current message on the Ubuntu side" and "this is the current message on the upstream side."
<thumper> jtv: do you know about assertSqlAttributeEqualsDate
<thumper> ?
<jtv> No
<thumper> you could use...
<jtv> That's a thought.  I'd also have to inject the right date though.
<thumper> self.assertSqlAttributeEqualsDate(
<thumper>     current, 'date_reviewed', UTC_NOW)
<thumper> which is what I think you really want to check
<jtv> But it doesn't have to be UTC_NOW.
<jtv> It could be the python-side clock from a few ticks ago.
<thumper> but you haven't committed
<thumper> so it will be the UTC_NOW constant
<thumper> see lib/lp/code/model/tests/test_branch.py:152
 * thumper laughs
<thumper> the only tests that use this assertion method are lp.code tests
<thumper> :)
<jtv> It involves changing markReviewed, but UTC_NOW does make more sense.
<thumper> what changes in markReviewed?
<jtv> It currently uses the python clock.
<thumper> ew
<jtv> I didn't think that UTC_NOW got deferred until commit though; doesn't it get set when the object is flushed to the store?
<thumper> it is the transaction start time
<thumper> IIRC
<thumper> but yes
<thumper> the setting happens when flushed
<thumper> but the assertion method checks for UTC_NOW
<thumper> we tend to use timestamp args like
<thumper>  _date_requested=None
<thumper>         if _date_requested is None:
<thumper>             _date_requested = UTC_NOW
<jtv> Yes, it's the transaction start time.  But AIUI it's a special constant when you set it, and then when the object gets flushed or invalidated, it gets replaced with an actual timestamp.
<thumper> that way tests can ovverride
<thumper> jtv: that's my understanding too
<jtv> So have to be careful with unexpected flushing.
<thumper> we have the params start with an underscore to indicate that they are private params
<jtv> What is a private param?
<jtv> It seems a contradiction in terms.
<thumper> :)
<thumper> one whose default value is used almost all of the time
<thumper> but other internal model code may pass something
<thumper> and tests often pass it
<thumper> def requestReview(self, _date_requested=None):
<thumper> no browser or external code should pass the param
<thumper> but internal or tests are OK
<thumper> that is what we mean by private param
<jtv> Of course another thing I could do is just override current.markReviewed and verify that it gets called.  :)
<jtv> current.markReviewed = FakeMethod()
<jtv> self.assertEqual(1, current.markReviewed.call_count)
<thumper> if that is all you are wanting to test, then yeah, sure
<jtv> It may be a bit short on integration-testing.
<jtv> I'm going with UTC_NOW and assertSqlAttributeEqualsDate.
<thumper> ok
<jtv> It really is better that way.  I wonder why I didn't find that methodâisn't it in lib/lp/testing/__init__.py?
<jtv> thumper: pushed that change already btw so you may want to reload the diff
<thumper> jtv: why did you not just replace datetime.now(UTC) with UTC_NOW instead of changing the default param?
<thumper> jtv: it is more idiomatic to have the default be None
<jtv> thumper: I just didn't see the point in having the extra step.
<jtv> I think the None is mainly more idiomatic for cases like [] and {} where things go wrong if you specify the default you really want.
<thumper> and UTC is a constant?
<jtv> Although on the other hand there's the pain of specifying the same default in the interface.
<thumper> true
<thumper> go for None I reckon
<jtv> OK
<jtv> It's pushed, but still no diff update
<thumper> done
<jtv> Thanks.
<jtv> lifeless, you may have some useful thoughts on something I want to do.
<jtv> guten Morgen henninge
<henninge> Goedemorgen jtv! ;)
 * henninge has to remember that w/o looking it up.
<henninge> jtv: à¸ªà¸§à¸±à¸ªà¸à¸µ
<jtv> à¸ªà¸§à¸±à¸ªà¸à¸µà¸à¸£à¸±à¸
<jtv> àºªàº°àºàº²àºàºàºµ
* 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> jtv: shoot
<jtv> lifeless: hi!  It's about the fake librarian.
<jtv> It catches transaction.commit() in order to simulate the librarian's transactional behaviour.
<jtv> (Except of course that it gives something clearer than LookupError if you haven't committed your file yet)
<jtv> however
<jtv> I would like to allow tests to avoid the commit altogether.
<jtv> So many tests commit just to satisfy the librarian.  With the fake librarian, we could avoid those commits.
<jtv> Now, I can't in good faith add a method to the Librarian API.
<jtv> So where do I put this?
<lifeless> why can't you ?
<lifeless> it might be better to let tests commit
<lifeless> and stub out the whole db
<lifeless> bzr tests do many very expensive commits and are still much faster than lp commits
<lifeless> s/lp commits/lp tests/
<lifeless> anyhow; I don't understand why you can't make the librarian api better in tests
<jtv> Well the call I have in mind would basically say "pretend there's been a commit as far as the librarian is concerned."
<jtv> That's not even something we _can_ pretend with the real librarian.
<lifeless> so
<lifeless> I don't understand the problem
<jtv> That's because I haven't really gotten around to formulating it properly yet.
<jtv> The problem at hand is: I want to allow a test,
<jtv> which uses the fake librarian,
<jtv> to tell the fake librarian: "give up the pretense that you don't know about these files just because I haven't committed yet; I don't _want_ to commit."
<jtv> This is something you'd do after creating test data, before the operation you're going to test.
<lifeless> ok
<lifeless> so do that?
<jtv> Quite.
<jtv> As long as it's only a pretense for the librarian's benefit, I feel I can't have a method for that in the librarian APIâbut that makes it hard to invoke on the utility.
<lifeless> again, I don't understand.
<lifeless> perhaps it would help if I describe how I'd do it.
<jtv> ga
<lifeless> I'd create an interface FakedLibrarian
<jtv> (ga the acronym, not the onomatopeia)
<lifeless> extending Librarian
<lifeless> (pseudonyms, I know the names are different)
<jtv> There's an ILibrarianClient.
<lifeless> in FL I'd add 'pretend_commit()'
<lifeless> and in the test suite, the same utility is the fake librarian instance, which implements FL, and so it all works.
<jtv> So I'd unregister the existing ILibrarianClient utility and register an IFakeLibrarianClient in its stead?
<jtv> Would getUtility(ILibrarianClient) then let me access IFakeLibrarianClient methods??
<lifeless> yes
<lifeless> because FLC extends LC it still meets the interface and can be a valid utility
<lifeless> if necessary, you could just do IFLC(getU(LC))
<jtv> There are other alternatives, I suppose: let the test keep the fake librarian in self.fake_librarian and do self.fake_librarian.commit()
<lifeless> sure
<lifeless> either would seem to fit the zope style
<jtv> For now, where a test sets it up manually, self.fake_librarian would be an unproxied object, so there's no need even to extend the interface.
<jtv> Doesn't work so well if we turn it into a test resource or whatever, I suppose.
<lifeless> btw
<lifeless> lp:python-fixtures
<lifeless> have a play, you might like it
<jtv> lifeless: thanks.  When there's timeâ¦ 2011 is a bit full already but 2012 is looking good.
<lifeless> jtv: its a factored out core component from testresources
<jtv> Ah
<lifeless> jtv: its a closer fit in some ways to layers and easier to work with
<lifeless> I suggest it not to give you work, but to liberate you :)
<jtv> :-)
<wgrant> lifeless: What was the motive for splitting it out of testresources?
<wgrant> (ie. why wouldn't one want to use testresources?)
<lifeless> wgrant: separate concepts
<lifeless> many places and projects to use conceptual fixtures exist separate from 'optimise hi cost setup/teardowns'
<lifeless> wgrant: so this aims to be the one-true api for the fixture bit, and optimising layers on top
<lifeless> wgrant: also, write one to throw away
<jelmer> hi Abel
<jelmer> adeuring1: Can I add a branch to the mp queue?
<adeuring1> jelmer: sure, I'll look at it
<jelmer> adeuring1: Thanks. The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/617393-cleanup-tmp/+merge/33194
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jelmer  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> jelmer: in unpack_source(): output, unused = dpkg_source.communicate() -- is stderr really always unused? Or should possible stderr data be added to the data of DpkgSourceError?
<jelmer> adeuring: The useful errors seem to end up on stdout; I'm not sure whether stderr is actually unused.
<adeuring> jelmer: ok; it occured to me meanwhile that you didn't chnage that code anyway -- just moved it (sorry, was distracted from reviewing...)
<adeuring> jelmer: In lines 447..453 of th diff: I think you should use assertRaises() instead of try/except
<adeuring> otherwise, you don't check that the exception is indeed raised
<jelmer> adeuring: I need to check that the exception has the right message in it
<jelmer> adeuring: The self.fail() would be called in that case.
<adeuring> jelmer: ah, right, I missed the fail() call. sorry
<adeuring> jelmer: r=me
* 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
<jml> jelmer, you can use assertRaises for that!
<jml> jelmer, e = self.assertRaises(ThingyException, f, blah, blah); self.assertEqual("Mishandled thingy", str(e))
<jelmer> adeuring: Thanks!
<jelmer> jml: Ah, I wasn't aware assertRaises returned the exception. Thanks
* 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
<bac> hello adeuring
<adeuring> hey bac!
<bac> adeuring: how goes the battle?
<adeuring> bac: I was distracted by a few other issues -- did just in review...
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,allenap  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: mbp,allenap  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: mbp,allenap  || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> adeuring, bac, I've just added a long and boring branch to your queue; hope you'll forgive me
<bac> salgado: np
<adeuring> salgado: any bribe offers ? ;)
* bac changed the topic of #launchpad-reviews to:  On call: adeuring, bac || reviewing: mbp,-  || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado,-  || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> adeuring, a promise that the next one will not be boring as this.  does that count as a bribe?  ;)
<adeuring> salgado: sure :)
<gmb> adeuring, Can I add https://code.edge.launchpad.net/~gmb/launchpad/new-subscriptions-widget-bug-616653/+merge/33215 to your queue (it's the basic subscription widget work)
<adeuring> gmb: go ahead
* gmb changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado,-  || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring, Thanks. Didn't notce that bac was OCR too. Hi bac.
<bac> morning gmb
<bac> gmb i'll take your branch.
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado, gmb  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bac, Thanks. It's a bit of a weird one because it feels like it doesn't add any value, but it's a get-the-ball-rolling branch rather than actually adding any functionality just yet.
<bac> gmb: ok.  using the salgado/sinzui cadence fu?
<gmb> bac, Yes. Well, I'm also using the "I'll kill myself if I don't land *something*" cadence.
<bac> gmb, it's august fer crying out loud!  get on your 2010!
<gmb> bac, Aaaaaaagh
<gmb> CPE.
<bac> :)
<bac> gmb: can't we just get one of the script gurus to fix up all copyrights statements on their xmas break for the coming year?
<gmb> bac, Yes, lets :)
<bac> gmb: I think 'A change is made to the bug (do not notify me about new comments).'  would be better as 'A change is made to the bug, but do not notify me about new comments.'  What do you think?
<bac> gmb: hmm, is that comma extraneous?
<gmb> bac, I had the same problem when trying to work out what that label should read :)
<gmb> A semicolon would replace the ', but' appropriately, I think.
<bac> gmb: classic bikeshedding.  i just think avoiding the parenthetical aside works
<gmb> bac, 'A change is made to the bug; do not notifiy me about new comments' reads better, I think.
<gmb> Although without the spelling error, obviously.
<bac> gmb: ok.  hey, why don't we defer to our wordsmith, since we're supposed to anyway? (conjures mrevell)
 * mrevell reads
<bac> gmb: that is a boatload of scaffolding just for a simple little test!
 * gmb imagines mrevell in harem pants and curly shoes, with a Dali mustache.
<mrevell> hah
<mrevell> semi-colons ftw
<gmb> bac, I know. Actually, I'll try to trim some of it out; it's been a while since I did JS tests, so I've cargo-culted.
<gmb> mrevell, Thanks.
<mrevell> :)
<bac> r=bradley
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado, -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bac, Thanks.
<adeuring> salgado: r=me
<salgado> thanks adeuring!
* adeuring 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
<EdwinGrubbs> adeuring, bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout/+merge/32555
<bac> EdwinGrubbs: i will
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -, Edwin  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> bac: I'm going to drive to a coffee shop, so I'll be incomunicado for a few minutes.
<bac> ok
* adeuring changed the topic of #launchpad-reviews to: On call:  bac || reviewing: Edwin  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> bac: Thank you for reviewing my refactor-get-email-notifications branch earlier :)
<bac> allenap: it's my job.  :)  nice branch
<bac> allenap: what's up with all of these launchpad branches?  you miss us?
<allenap> bac: Yes :)
<bac> well, come home soon
<allenap> bac: Well, my rotation to Landscape is ending (early) in a couple of weeks, so not long.
<bac> allenap: been fun?
<allenap> bac: Yes, lots, but I am a bit home-sick :)
<EdwinGrubbs> bac: hmm, I'm not sure if my last message went through. I'm available for comments now.
<bac> EdwinGrubbs: i've already approved it
<bac> EdwinGrubbs: i did have a question about whether you can use the slave store
<EdwinGrubbs> thanks
 * bac reconsiders the phrase "slave store".  sounds like something you'd see in charleston
<EdwinGrubbs> bac: oh, I answered that in the mp. sorry, I didn't see you asking that in the irc channel.
* 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> hi EdwinGrubbs, could you do a review for me?
<EdwinGrubbs> bac: sure
<bac> thanks EdwinGrubbs  -- https://code.edge.launchpad.net/~bac/launchpad/bug-588773-charm/+merge/33248
<EdwinGrubbs> bac: oops, wrong channel
<bac> hey EdwinGrubbs, over here :)
<bac> though you might get a better answer in #canonical
<bac> autoupdate?  we don't know.
<bac> there is a bug for its removal
<EdwinGrubbs> who added it?
<bac> we looked in the db and only one project has a value for it.
<bac> EdwinGrubbs: it's been around a long time.
<bac> 1ZRR16270358332506
<bac> ergh
<bac> https://bugs.edge.launchpad.net/launchpad-registry/+bug/618926
<_mup_> Bug #618926: Remove autoupdate from product model <tech-debt> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/618926>
<lifeless> autoupdate
<lifeless> we had this requirement to sync project metadata from freshmeat etc
<EdwinGrubbs> I started to get excited when I thought you pasted your cc number, but alas, no free laptop for me.
<lifeless> did 2 runs of the syncer or something and stopped. It was terrible ;)
<lifeless> IIRC salgado knows all about it, but I'm pretty sure you can zap it
<bac> EdwinGrubbs: no, much more boring.  hey look, i got a delivery!
<bac> thanks lifeless
<bac> brb
<EdwinGrubbs> bac: I get an oops if I try to change the person_standing or the personal_standing_reason on a person.
<bac> EdwinGrubbs: ok, let me look
<sinzui> I found a cron job that updates standing every day. It does nothing
<sinzui> jcsackett, did you update your MP to needs review?
<jcsackett> sinzui, i did; i was waiting for the diff to pop up before adding myself to the queue.
* sinzui changed the topic of #launchpad-reviews to: On call:  bac || reviewing: -  || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> jcsackett, undersood
<jcsackett> and sinzui beats me into the queue. :-P
<sinzui> I chose an arcane way to test that was the must less dangerous/evil
<sinzui> jcsackett, look at https://code.edge.launchpad.net/~sinzui/launchpad/dsp-bug-counts-1/+merge/33257
* jcsackett changed the topic of #launchpad-reviews to: On call:  bac || reviewing: -  || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> bac I have a branch for review (^), I think jcsackett should be ahead of me though if his is ready
<jcsackett> bac: https://code.edge.launchpad.net/~jcsackett/launchpad/add-enums-to-models/+merge/33255
<bac> ok, i'll do them in that oder
<bac> order
<bac> odor
<sinzui> jcsackett, I employed delegates and implements to make a test class. Those two helpers allow my class to be the type under test, and let me the code use the real object for non test parts.
<jcsackett> sinzui: very nice.
<jcsackett> that's really cool.
<sinzui> jcsackett, We also use this trick to create helpers that construct data for views
<bac> good catch, EdwinGrubbs.  i failed to change permission in IHasStanding
* bac changed the topic of #launchpad-reviews to: On call:  bac || reviewing: jcs  || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<lifeless> sinzui: hey, FYI
<lifeless> sinzui: that milestone +index fix, I'm down to 46 failures
<lifeless> all fallout from having to convert to storm
<sinzui> wow
<lifeless> sinzui: early next week I should have that conversion done;
<lifeless> then the simple fix can actually land
<lifeless> we could land an even simpler fix sooner if we wantd
<lifeless> by precaching before we access any attributes on the bugs at all
<lifeless> moving the list comprehension down a line
<lifeless> sinzui: what do you think
<sinzui> I have take similar fixes in the last few weeks. Getting some small improvement is still valuable to users
<lifeless> ok
<lifeless> so its the weekend now and we're moving house monday
<lifeless> if I don't have the larger fix's issues actually resolved tuesday, I'll do the trivial thing.
<sinzui> thanks lifeless. I will be about if you need a reviewer
<lifeless> thanks
<lifeless> won't be today
<lifeless> off to buy a desk etc
<EdwinGrubbs> bac: On the $projectgroup/+admin page, if you have launchpad.Moderate, the ProjectEditNavigationMenu shows that you are on the "Administer" page, but you don't see any other links, because you don't have launchpad.Edit. This looks especially bizarro, because there is a yellow edit-icon that you can't click on because you are already on that page.
<bac> EdwinGrubbs: will investigate
<EdwinGrubbs> sinzui: with regards to the ugly ui mentioned above, could the navigationmenu-related-pages.pt have a condition added, so that it doesn't display the menu if there is at least one clickable menu item? If that won't work, maybe we need some clearer indication that the link is disabled because you are already on this page and the edit-icon should appear in gray.
<sinzui> We ran into this problem on the milestone +index page
<sinzui> I thought the nav menu was updated to handle that case
<sinzui> EdwinGrubbs, Your suggest is sound. I thought we had fixed that issue
<EdwinGrubbs> bac: ^^^
<bac> jcsackett: it looks like your tests for distribution and product could be refactored and combined
<jcsackett> bac: i thought about that, but wasn't sure where to stick the combined set.
<bac> jcsackett: name the test for the functionality, not the pillar
<bac> test_service_usage.py
<jcsackett> bac: dig.
<bac> put all of the goop into a base and the product and distribution tests will be teeny
<bac> jcsackett: r=bac, with those suggested fixes
<bac> jcsackett: make sure your base class doesn't execute too.
* bac changed the topic of #launchpad-reviews to: On call:  bac || reviewing: sinzui  || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> bac: thanks. looking into it now.
<bac> hi EdwinGrubbs.  i've fixed the permission problem you found and updated the MP with an incremental diff
<EdwinGrubbs> bac: r=me
<bac> thanks EdwinGrubbs.  have a nice weekend
<bac> EdwinGrubbs: i think that UI fix is probably good and safe.  just not for a friday evening
<bac> sinzui: this does seem to work:  http://pastebin.ubuntu.com/481143/
<sinzui> :(
<sinzui> I think that looks right
<EdwinGrubbs> bac: that's fine. there's no reason to tempt fate.
<sinzui> bac: should we add the show_links to the render rule instead?
<bac> sinzui: perhaps
<sinzui> bac: navigationmenu-related-pages.pt? I think you want to hack global
<sinzui> bac: navigationmenu-actions renders the action menu
<sinzui> bac: related-pages is for that eye-sore that appears on some pages telling you that the page you are one may not have the complete information you are interested it
<bac> sinzui: all good things to consider when we tackle this issue.  mine was just a proof-of-concept since i'm not including it in my de-debacle branch
* 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
<bac> sinzui: did i mentiopn r=bac on your branch?
<sinzui> faboo
#launchpad-reviews 2010-08-22
* lifeless changed the topic of #launchpad-reviews to:  call:  - || reviewing: -  || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/milestones/+merge/32855] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> anyone around to give a quick review on some changes since a branch was approved?
<jcsackett> had to throw in a bit when ec2 found some tests i missed.
<jcsackett> mp is here: https://code.edge.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820
<jcsackett> diff from changes is : https://pastebin.canonical.com/36129/
<jelmer> jcsackett: hi
<jcsackett> jelmer: hi.
<jelmer> jcsackett: what was the test failure exactly? You're including team memberships for which the user was still waiting for approval or memberships that had expired?
<jcsackett> precisely: teams that shouldn't be showing up in participation (b/c they were pending) were showing up.
<jcsackett> also: if you were going +participation for a team, you got an entry for the team itself.
<jcsackett> which is sort of silly. :-P
<jelmer> heh
<jelmer> jcsackett: Are you going to do more work in this area? It seems like it would be nice to have a list of memberships statusses that mean the user should be considered a valid member of the team.
<jelmer> jcsackett: Other than that, r=me
<jcsackett> jelmer: you mean perhaps define "valid_status = (list of statuses); filter on valid_statuses" ?
<jelmer> jcsackett: yeah, as there seems to be another place in the same file where you use that same condition.
<jelmer> Anyway, that's just a thought. Not required for landing this change imo.
<jcsackett> jelmer: it's a good thought, thanks. :-)
