[03:08] <jml> mwhudson, while you're still around, would you like to review my import warnings branch?
[03:08] <mwhudson> jml: definitely
[03:09] <jml> mwhudson, https://code.edge.launchpad.net/~jml/launchpad/fix-import-warnings/+merge/16965
[03:11] <mwhudson> jml: i wonder why those files imported things from _schema_circular_imports
[03:11] <jml> mwhudson, me too.
[03:11] <mwhudson> that just seems a bit odd
[03:11] <jml> mwhudson, my guess is that the module moving thingy did it
[03:11] <jml> mwhudson, I guess I could 'bzr blame'
[03:11] <mwhudson> jml: "+ # The bug turned to be security-related, subscribe the security"
[03:11] <mwhudson> jml: can you insert the missing "out" ?
[03:12] <mwhudson> it's in lib/lp/bugs/subscribers/bug.py
[03:12] <jml> mwhudson, done.
[03:14] <mwhudson_> jml: can you paste the link again
 mwhudson, https://code.edge.launchpad.net/~jml/launchpad/fix-import-warnings/+merge/16965
[03:14] <mwhudson_> ta
[03:14] <jml> last lines of convo...
 it's in lib/lp/bugs/subscribers/bug.py
 mwhudson, done.
[03:17] <mwhudson_> i actually saw those
[03:17] <mwhudson_> jml: i guess i should ask you to file a bug for those XXXs but i can forget to if you like
[03:17] <mwhudson_> otherwise, very much r=me
[03:35] <jml> mwhudson, can you please change some status on the MP?
[03:35] <mwhudson> ah right yes
[03:36] <jml> mwhudson, thanks.
[04:07] <jml> mwhudson, another branch for you
[04:08] <mwhudson> jml: ok
[04:08] <jml> https://code.edge.launchpad.net/~jml/launchpad/upcall-testcase/+merge/17002
[04:10] <mwhudson> jml: BRANCH.TODO still has quite a lot in it
[04:10] <jml> mwhudson, oops
[04:10] <jml> mwhudson, fixed
[04:12] <mwhudson> jml: it's almost like you have some automatic way of finding pep8 violations
[04:12] <jml> mwhudson, it's uncanny that you should mention it, because in fact it turns I out I have.
[04:13] <mwhudson> jml: also i guess that doing the super().setUp() style when you pass the user argument is theoretically a bit risky
[04:14] <jml> mwhudson, blame python
[04:14] <mwhudson> yeah
[04:15] <mwhudson> jml: approved
[04:16] <jml> mwhudson, thanks.
[04:52] <stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/16898
[05:00] <jml> stub, replied.
[05:03] <stub> Fixing now. Ta.
[05:13] <stub> jml: pushing now. Diff at https://pastebin.canonical.com/26350/ for the impatient. 
[05:13] <jml> stub, it's spelled "separate".
[05:14] <jml> stub, other than that, r=jml
[05:14] <stub> Not when I spell it is isn't ;)
[05:14]  * stub needs misspelling highlighting in vim
[05:16] <jml> there was a terrible awful mnemonic on the wall in one of my classes in primary school
[05:16] <jml> smell "a rat" when you sep_arat_e
[10:18] <henninge> Hi adeuring! Bock auf'n review? ;-)
[11:00] <jtv> adeuring: didn't even see that you'd already reviewed my import policy violations fix... thanks!
[11:00] <adeuring> jtv: welcome :)
[11:01] <adeuring> henninge: sorry, didnt notice your request... Yes, I can of course do the review
[11:01] <henninge> adeuring: np ;)
[11:01] <henninge> thanks
[13:33] <bac> salgado: i just submitted a MP for a branch to fix bug 419930.  can you review it?
[13:33] <mup> Bug #419930: non-public email address shown after merge request <email> <merge> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/419930>
[13:34] <salgado> bac, sure!
[13:34] <bac> salgado: cool b/c you're the best one to do it.  thanks.
[13:42] <henninge> adeuring: Hi, how are you doing with the review? Any questions?
[13:43] <adeuring> henninge: I have one suggestion:
[13:44] <bac> salgado: it just arrived: https://code.edge.launchpad.net/~bac/launchpad/bug-419930/+merge/17015
[13:44] <adeuring> henninge: you list the the "SQL names" o the person objects in ILaunchpadCelebrities in l/c/l/doc/celebrities.txt. What do you think of listing instead the _attribute_ names of IPerson objects in ILauchpadCelebrities()?
[13:45] <adeuring> The latter is what you need in IPersonRoles
[13:47] <henninge> adeuring: yes, correct. This was the cheapest way to do it because the SQL names were already being collected.
[13:47] <adeuring> henninge: yes. But you can iterate over ILaunchpadCelebrities.names() to find those celebrities that implement IPerson.
[13:48] <henninge> adeuring: actually, I'd check for PersonCelebrityDescriptor
[13:48] <adeuring> henninge: OOPS, right
[13:48] <henninge> adeuring: but you're right, that shouldn't be too hard.
[13:48] <henninge> thanks for the suggestion.
[13:49] <adeuring> henninge: Then you could also get rid of the person_names attribute in ILPCelebs
[13:49] <adeuring> henninge: other that that; i found a few typos. I'll send the nitpicks via mail ;)
[13:50] <henninge> adeuring: how? I still need a way to get to the list of names - be they sql names or attribute names.
[13:52] <adeuring> henninge: right. You already use IPersonRoles.names() in a test; ILPCelebrities.names() should give you all celebrity names; and with IPersonCelebrity.providedBy(getattr(all_celebrities, attribute_name) you can check if you have a "relevant" celeb object, I think
[13:53] <henninge> adeuring: no doubt, but I want to do that in a test and to get to that in a test, it needs to be in an interface somewhere.
[13:53] <henninge> adeuring: PersonCelebrityDescriptor is not exported and I don't want to do it just for the test.
[13:56] <adeuring> henninge: OK, makes sense. Wat about adding a class attribute is_person_celebrity to CelebrityDescriptor and PersonCelebrityDescriptor?
[13:56] <adeuring> s/wt/what/
[13:58] <henninge> adeuring: I had thought about that but thought I could achieve the same thing just using isinstance (eventually I resorted to just counting).
[13:58] <henninge> adeuring: but if I do that *instead* of person_names, it makes more sense.
[13:58] <henninge> adeuring: I'll do that.
[13:59] <adeuring> henninge: OK, but there is even an attribute CelebrityDescrptor.interface. COuldn't you use that one?
[13:59] <henninge> and check for IPersonSet? possibly
[14:00] <henninge> hm
[14:01] <adeuring> henninge: right, that's a bit odd
[14:03] <henninge> adeuring: no, I think it's odder to add an attribute to a base class that is meant for one specific child class
[14:03] <henninge> which is_person_descriptor would be
[14:03] <adeuring> henninge: right...
[14:07] <henninge> adeuring: problem is, ILaunchpadCelebrities has attributes that are no descriptors at all
[14:07] <henninge> adeuring: so I'd have to check for existence of the "interface" attribute first, then check its value.
[14:08] <adeuring> henninge: right. But: reading the documentation of __get__() again, I think you can simply use IPerson.providedBy(celebrity)
[14:08] <henninge> oh, really? let me try that.
[14:40] <henninge> adeuring: ok, that works but I had to go through "getattr" for the Descritor to be invoked. I tried to loop through __dict__ first but that does not invoke __get__. And it looks ugly.
[14:40] <henninge> again something learned about Python 
[14:42] <adeuring> henninge: yeah, getattr() was what I was thinking about ;)
[14:43] <henninge> adeuring: look http://paste.ubuntu.com/353497/
[14:49] <adeuring> henninge: looks good. But I think it you should also explicitly print the names of the "person celebrities", so that you see more clearly that you should change IPersonRoles when you add/remove/change an IPerson celebrity. (sorry to be such PITA...)
[14:51] <henninge> adeuring: np
[14:52] <adeuring> henninge: Or perhaps you could even create a sorted list of the person celeb names, prefixed with "in_" and compare that list with the corresoding list of attributes from IPersonRoles
[14:52] <henninge> adeuring: that is what I am doing now.
[14:52] <adeuring> henninge: cool!
[14:52] <henninge> just the other way round, I am removing the prefix from the IPersonRoles attribute names.
[14:52] <adeuring> henninge: yeah, sure ;)
[14:53] <henninge> adeuring: I meant, that is what the patch does.
[14:53] <adeuring> ok
[15:08] <adeuring> henninge: OK, now I get it; what I meant is: The comparison in line 64 is http://paste.ubuntu.com/353497/ is fine. But when something goes wrong, you need to figure out what goes wrong. It helps a little bit if you print also the content of person_celebrity_names
[15:08] <henninge> adeuring: working on that atm, using set
[15:08] <adeuring> henninge: great, thanks! otherwise, r=me, BTW
[15:09] <henninge> adeuring: cool, thank you!
[16:18] <salgado> bac, sorry for the delay on that review
[16:21] <bac> salgado: np
[16:23] <bac> salgado: to answer your question, if a duplicate person has hidden email addresses then you can merge them if you know the LP id.  is that problematic?
[16:24] <salgado> bac, no, I was just wondering if it's currently possible to merge a dupe account with hidden email addresses
[16:25] <bac> salgado: you cannot get the person via the picker but you can enter the id in the field
[16:25] <salgado> right
[16:26] <bac> thanks for catching the missing test and the suggested improvements
[17:42] <bac> salgado: fancy another?
[17:43] <salgado> bac, sure, why not?
[17:43] <bac> salgado: https://code.edge.launchpad.net/~bac/launchpad/bug-407604-proj-desc/+merge/17028
[17:44] <bac> salgado: i am about to leave for lunch and an appt so i won't be around to answer questions on IRC
[17:45] <salgado> that's fine
[19:22] <gary_poster> EdwinGrubbs: hey.  So, I'm still thinking about the vocabulary thing you pinged me about.  Even though I don't feel "done," I figure I'd share my thoughts so you could steer me or maybe provide an idea I would have missed.
[19:22] <gary_poster> To start trivially and less problematically (for me at least), I think lines 177-180 should be a "try: ... except UnauthorizedError: ..." rather than looking for launchpad.View.  IMO, you should never have to ask for a given permission.  
[19:22] <gary_poster> (FWIW, zope.security.checker.canView would be better because it does what you want without having to specify a permission, and would incidentally Just Work without the secured vocabulary work you have done, but I agree with Francis that a security proxy is a better approach.)
[19:23] <gary_poster> EdwinGrubbs: huh
[19:23] <gary_poster> EdwinGrubbs: did you get everything I just wrote?  It ended with "lines 138-148, written in zcml for each vocabulary definition?"
[19:23] <EdwinGrubbs> gary_poster: No, I didn't see that line.
[19:23] <gary_poster> EdwinGrubbs: what was the last line you got?
[19:23] <gary_poster> before I said "huh"
[19:24] <gary_poster> (if any)
[19:25] <EdwinGrubbs> gary_poster: the last line I got ended with "security proxy is a better approach.)"
[19:25] <EdwinGrubbs> gary_poster: can I use canView() instead of the try/except UnauthorizedError?
[19:26] <gary_poster> EdwinGrubbs: you could.  Why do you want to?
[19:26] <gary_poster> When we finish this I guess I'll paste in the next item :-)
[19:27] <EdwinGrubbs> gary_poster: well, because exception handling is slow, and because I have seen canView() used elsewhere in launchpad but exception handling for permissions not so much.
[19:34] <gary_poster> EdwinGrubbs: slow: I'm skeptical.  I know the advice of which you speak, but I doubt exception handling is slow compared to calling another Python function.  I'd want to see a timeit test before that was a valid argument.  I think exception handling is slow compared to other single-Python-operation tasks.  
[19:34] <gary_poster> exception handling for permissions: huh.  Of course, that's the principle for Zope's basic operation.  I think try: except is more appropriate here.  I'm OK with you using canView for now, and letting me bring this up in the reviewers meetings (are we even having those now that barry is gone?) if that's what you want.
[19:34] <gary_poster> I'll copy and paste my next point then.
[19:35] <gary_poster> Still staying easy, my next topic is the way you handled __getslice__ for ICountableIterator.  I prefer what you have done rather than modifying ICountableIterator, if I understand the situation properly.  Am I correct that this is necessary because Python will try __getslice__ before trying __getitem__?  If so, this is clearly a security issue, and the scribbling belongs where you have it, IMO.
[19:35] <gary_poster> If you are OK with that, I'll then copy and paste my last bits, which was where I wrote quite a lot.
[19:35] <gary_poster> well, more than the bit you read, anyway.
[19:36] <EdwinGrubbs> gary_poster: bac has taken over the reviewer meetings. I guess you missed the one on Wed.
[19:36] <gary_poster> EdwinGrubbs: cool.  Yeah, I was in Lean training with the OEM folks, since I wasn't with LP the first time around.
[19:37] <EdwinGrubbs> gary_poster: excuses, excuses
[19:37] <gary_poster> heh
[19:37] <EdwinGrubbs> gary_poster: __getslice__ is used because HugeVocabularyJSONView batches the results.
[19:38] <gary_poster> EdwinGrubbs: but the implementation handles slices in its __getitem__, right?
[19:38] <gary_poster> I mean, the vocabulary implementation
[19:39] <EdwinGrubbs> gary_poster: right
[19:39] <gary_poster> EdwinGribbs: cool.  Then are you OK with me being OK with what you have done there? :-)
[19:40] <EdwinGrubbs> gary_poster: as far as try/except for security exceptions. I have no problem with it, if that is preferred, but I can imagine getting questioned on it in the code review, so it would be good to bring it up at the next meeting.
[19:41] <gary_poster> EdwinGrubbs: cool, makes sense
[19:41] <EdwinGrubbs> gary_poster: yes, I approve of your approval. I guess I'm ready for the pasted text.
[19:41] <gary_poster> OK cool
[19:41] <gary_poster> Five chunks
[19:41] <gary_poster> Finally, the bigger and harder question is the first one you raised, about creating a new vocabularyutility directive rather than hacking securedutility. Honestly, I don't love either approach. 
[19:41] <gary_poster> The securedutility approach is a kludge, as you said.  If we were going to pursue it, I'd like to try doing it a slightly different way, but we can talk about that later if necessary.  (Summary: register another directive in first phase of zcml processing rather than just doing it in the second stage.)
[19:42] <gary_poster> The vocabularyutility directive introduces yet another zcml directive, when I think that in practice one of zcml's failings is that it was never actually designed as a language but as a collection of automations.  (If I'm wrong, and the original intent was a language, then I think the intent was lost.)  Therefore, I don't want that one either.
[19:42] <gary_poster> I tend to think that what we need is to look at the problem you are trying to solve and step back.  We can introduce a new zcml directive, but if we do, I'd rather it be of more general usefulness.  It might not be possible, or possible within a short timeframe, but I still think we need to talk about it.
[19:42] <gary_poster> The problem you were trying to solve was "a ton of extra boiler plate".  I think I know what that boiler plate was, but do you have a full example, or can you easily construct it?  Am I right that it is lines 138-148, written in zcml for each vocabulary definition?
[19:42] <gary_poster> That's the end of the paste.
[19:43] <gary_poster> I'm pretty sure that the last para is right about lines 138-148.  I still don't have an answer I like. :-(
[19:43] <gary_poster> That is, I've tried to "step back" and I still don't see anything...
[19:46] <gary_poster> I believe the boiler plate is something on the order of this:
[19:47] <gary_poster> <allow interface="...IVocabularyFactory" /><allow interface="...IHugeVocabulary" />
[19:47] <EdwinGrubbs> gary_poster: exactly
[19:49] <EdwinGrubbs> gary_poster: If we don't want the zcml directive doing the work. One alternative that may or may not be considered even more kludgy would be to post process the registered utilities. Of course, I have absolutely no idea how to go about doing that.
[19:49] <EdwinGrubbs> gary_poster: I'm not sure what you meant about the first and second phase of zcml processing.
[19:55] <gary_poster> EdwinGrubbs: zcml processing has two rounds.  First, it parses the XML and registers directive classes with unique identifiers.  If it encounters a directive with the same identifier already registered, it pukes, and announces a conflict.  This is how zcml makes sure that two directives don't fight (or even duplicate) one another.  If the zcml processing gets through this phase, then it actually *performs* the zcml direc
[19:55] <gary_poster> In the case of your code, registering directives for the classes would make sure that you are not stomping on anything else.  It also would have caught the problem that made you use the _vocabulary_cache...though you might still have needed that solution, in order to avoid complaints.
[20:00] <gary_poster> EdwinGrubbs: OK after thinking about it maybe more than I should have, here's my opinion, which you might not like :-) .  I think you should make an interface that combines IVocabularyFactory and IHugeFactory into a single interface for convenience, and do <allow interface="...combined interface" /> for now.
[20:00] <gary_poster> Solving this is a matter for thought--and in fact, this is the kind of thought that the grok community has been pursuing.  That project is about reducing Zope boiler plate (and making it easier to start with Zope 3 technologies) and so I think we will want to use their approach for simplifying this kind of thing in the future.
[20:00] <gary_poster> For now, I don't think we should introduce a surprising kludge or Yet Another ZCML directive.
[20:00] <gary_poster> That's my take.
[20:08] <EdwinGrubbs> gary_poster: I just realized that I was wrong when I said that the boilerplate I was eliminating were just the two <allow> directives. I can't combine those two directives with a new interface, since one <allow> will be in <securedutility> and the other will be in <class>. See http://pastebin.ubuntu.com/353609/
[20:15] <EdwinGrubbs> gary_poster: I'm not sure how grok would make this situation better. Wouldn't require just as many extra configurations, or is it considered less tabu to create custom configuration directives, since it doesn't have the difficulty of trying to figure out where a zcml directive is actually defined or what it does?
[20:20] <EdwinGrubbs> gary_poster: it really seems like we need something like the <pages><page/></pages> hierarchical directives, so we can fill in redundant attributes. However, it wouldn't be quite as simple since there is both a <securedutility> and <class> directive to fill in.
[20:20] <gary_poster> EdwinGrubbs: So, first, for the "allow" concern, here's a reply: http://pastebin.ubuntu.com/353615/
[20:21] <gary_poster> EdwinGrubbs: oh...
[20:21] <gary_poster> EdwinGrubbs: you need to treat the IVocabularyFactory as something that the context provides
[20:22] <gary_poster> EdwinGrubbs: and you need to treat the resulting class as something that the context implements
[20:22] <gary_poster> EdwinGrubbs: that's why my idea doesn't work
[20:23] <gary_poster> I mean "you need to treat the IHugeVocabulary as something that the context implements"
[20:25] <EdwinGrubbs> gary_poster: yes, in lines 138-148, I had to use a separate ClassDirective instead of piling everything into self.allow()
[20:27] <gary_poster> EdwinGrubbs: grok better: I think you would be able to define the utility in a single line, and define the permission for the class in a single line.  It would not compress the two behaviors the way you have it here, but I suspect it would be both more readable/explicit and shorter.  (Moreover, if grok does not have support for secured utilities, we could add it.)
[20:28] <gary_poster> admittedly, you would still have to follow the "recipe" for a secured vocabulary, which is your point.
[20:30] <EdwinGrubbs> gary_poster: zcml seems anti-DRY. Maybe I could create a new paradigm, WET (We exemplify tediousness).
[20:30] <EdwinGrubbs> ;-)
[20:30] <gary_poster> lol
[20:32] <EdwinGrubbs> gary_poster: despite my negativity, if you think that being explicit in the zcml is the best way to go, I'll do that.
[20:35] <gary_poster> EdwinGrubbs: "convention over configuration": "convention" usually is not explicit.  configuration typically does not conform to DRY.  grok is about trying to get some of the zope benefits while bringing in some DRY ideas.  
[20:35] <gary_poster> I'm not against your concerns, I'm just against trying to address them in a way that introduces another issue IMO (makes the zcml "language" bigger with more to learn) without some significant pushback.  I reconciled my position in my own mind by thinking "grok is the better way to reduce duplication".
[20:35] <gary_poster> I'll summarize my position like this:
[20:36] <gary_poster> 1) My primary vote is to just grin and bear it, and wait for grok and/or some deeper thought on how to reduce DRY in our code.
[20:37] <gary_poster> 2) If you feel strongly that you don't want to wait for some unspecified time in the future for this, I'm uncomfortably OK with you making a securedvocabulary (or <vocabulary secure="true" />?) directive.
[20:37] <gary_poster> 3) I'd really rather not see the kludge in securedutility.
[20:38] <gary_poster> EdwinGrubbs: fair enough?  I need to run go pick up a son from school
[20:38] <gary_poster> EdwinGrubbs: back in a bit
[20:38] <gary_poster> '
[20:39] <EdwinGrubbs> gary_poster: go ahead, I'm getting distracted in another channel.
[20:55] <gary_poster> EdwinGrubbs: back fwiw
[21:08] <gary_poster> EdwinGrubbs2: I
[21:08] <gary_poster> have been back for a while.  don't know if you saw that (I see you are having nick fun)
[21:12] <EdwinGrubbs2> gary_poster: I saw your 1/2/3 summary. It's ok. sed should make it easy enough.
[21:25] <gary_poster> EdwinGrubbs: cool
[22:43] <sinzui> flacoste: I replied with a two remarks. Do you want to change the security on featured projects? Do you want to run the doc tests on the DatabaseFunctionalLayer?