/srv/irclogs.ubuntu.com/2010/01/08/#launchpad-reviews.txt

jmlmwhudson, while you're still around, would you like to review my import warnings branch?03:08
mwhudsonjml: definitely03:08
jmlmwhudson, https://code.edge.launchpad.net/~jml/launchpad/fix-import-warnings/+merge/1696503:09
mwhudsonjml: i wonder why those files imported things from _schema_circular_imports03:11
jmlmwhudson, me too.03:11
mwhudsonthat just seems a bit odd03:11
jmlmwhudson, my guess is that the module moving thingy did it03:11
jmlmwhudson, I guess I could 'bzr blame'03:11
mwhudsonjml: "+ # The bug turned to be security-related, subscribe the security"03:11
mwhudsonjml: can you insert the missing "out" ?03:11
mwhudsonit's in lib/lp/bugs/subscribers/bug.py03:12
jmlmwhudson, done.03:12
mwhudson_jml: can you paste the link again03:14
jml<jml> mwhudson, https://code.edge.launchpad.net/~jml/launchpad/fix-import-warnings/+merge/1696503:14
mwhudson_ta03:14
jmllast lines of convo...03:14
jml<mwhudson> it's in lib/lp/bugs/subscribers/bug.py03:14
jml<jml> mwhudson, done.03:14
mwhudson_i actually saw those03:17
mwhudson_jml: i guess i should ask you to file a bug for those XXXs but i can forget to if you like03:17
mwhudson_otherwise, very much r=me03:17
=== mwhudson_ is now known as mwhudson
=== mwhudson_ is now known as mwhudson
jmlmwhudson, can you please change some status on the MP?03:35
mwhudsonah right yes03:35
jmlmwhudson, thanks.03:36
=== abentley1 is now known as abentley
jmlmwhudson, another branch for you04:07
mwhudsonjml: ok04:08
jmlhttps://code.edge.launchpad.net/~jml/launchpad/upcall-testcase/+merge/1700204:08
mwhudsonjml: BRANCH.TODO still has quite a lot in it04:10
jmlmwhudson, oops04:10
jmlmwhudson, fixed04:10
mwhudsonjml: it's almost like you have some automatic way of finding pep8 violations04:12
jmlmwhudson, it's uncanny that you should mention it, because in fact it turns I out I have.04:12
mwhudsonjml: also i guess that doing the super().setUp() style when you pass the user argument is theoretically a bit risky04:13
jmlmwhudson, blame python04:14
mwhudsonyeah04:14
mwhudsonjml: approved04:15
jmlmwhudson, thanks.04:16
=== rockstar changed the topic of #launchpad-reviews to: on-call: || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
stubhttps://code.edge.launchpad.net/~stub/launchpad/replication/+merge/1689804:52
jmlstub, replied.05:00
stubFixing now. Ta.05:03
stubjml: pushing now. Diff at https://pastebin.canonical.com/26350/ for the impatient. 05:13
jmlstub, it's spelled "separate".05:13
jmlstub, other than that, r=jml05:14
stubNot when I spell it is isn't ;)05:14
* stub needs misspelling highlighting in vim05:14
jmlthere was a terrible awful mnemonic on the wall in one of my classes in primary school05:16
jmlsmell "a rat" when you sep_arat_e05:16
=== abentley1 is now known as abentley
=== jtv is now known as jtv-afk
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningeHi adeuring! Bock auf'n review? ;-)10:18
=== henninge changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado-afk is now known as salgado
jtvadeuring: didn't even see that you'd already reviewed my import policy violations fix... thanks!11:00
adeuringjtv: welcome :)11:00
adeuringhenninge: sorry, didnt notice your request... Yes, I can of course do the review11:01
henningeadeuring: np ;)11:01
henningethanks11:01
=== salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: henninge, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara-afk is now known as matsubara
=== mrevell is now known as mrevell-lunch
bacsalgado: i just submitted a MP for a branch to fix bug 419930.  can you review it?13:33
mupBug #419930: non-public email address shown after merge request <email> <merge> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/419930>13:33
salgadobac, sure!13:34
bacsalgado: cool b/c you're the best one to do it.  thanks.13:34
henningeadeuring: Hi, how are you doing with the review? Any questions?13:42
adeuringhenninge: I have one suggestion:13:43
bacsalgado: it just arrived: https://code.edge.launchpad.net/~bac/launchpad/bug-419930/+merge/1701513:44
adeuringhenninge: 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:44
=== bac changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: henninge, - || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
adeuringThe latter is what you need in IPersonRoles13:45
=== mrevell-lunch is now known as mrevell
henningeadeuring: yes, correct. This was the cheapest way to do it because the SQL names were already being collected.13:47
adeuringhenninge: yes. But you can iterate over ILaunchpadCelebrities.names() to find those celebrities that implement IPerson.13:47
henningeadeuring: actually, I'd check for PersonCelebrityDescriptor13:48
adeuringhenninge: OOPS, right13:48
henningeadeuring: but you're right, that shouldn't be too hard.13:48
henningethanks for the suggestion.13:48
adeuringhenninge: Then you could also get rid of the person_names attribute in ILPCelebs13:49
adeuringhenninge: other that that; i found a few typos. I'll send the nitpicks via mail ;)13:49
henningeadeuring: how? I still need a way to get to the list of names - be they sql names or attribute names.13:50
adeuringhenninge: 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 think13:52
henningeadeuring: 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
henningeadeuring: PersonCelebrityDescriptor is not exported and I don't want to do it just for the test.13:53
adeuringhenninge: OK, makes sense. Wat about adding a class attribute is_person_celebrity to CelebrityDescriptor and PersonCelebrityDescriptor?13:56
adeurings/wt/what/13:56
henningeadeuring: I had thought about that but thought I could achieve the same thing just using isinstance (eventually I resorted to just counting).13:58
henningeadeuring: but if I do that *instead* of person_names, it makes more sense.13:58
henningeadeuring: I'll do that.13:58
adeuringhenninge: OK, but there is even an attribute CelebrityDescrptor.interface. COuldn't you use that one?13:59
henningeand check for IPersonSet? possibly13:59
henningehm14:00
adeuringhenninge: right, that's a bit odd14:01
henningeadeuring: no, I think it's odder to add an attribute to a base class that is meant for one specific child class14:03
henningewhich is_person_descriptor would be14:03
adeuringhenninge: right...14:03
=== salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: henninge, bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
henningeadeuring: problem is, ILaunchpadCelebrities has attributes that are no descriptors at all14:07
henningeadeuring: so I'd have to check for existence of the "interface" attribute first, then check its value.14:07
adeuringhenninge: right. But: reading the documentation of __get__() again, I think you can simply use IPerson.providedBy(celebrity)14:08
henningeoh, really? let me try that.14:08
henningeadeuring: 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
henningeagain something learned about Python 14:40
adeuringhenninge: yeah, getattr() was what I was thinking about ;)14:42
henningeadeuring: look http://paste.ubuntu.com/353497/14:43
adeuringhenninge: 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:49
henningeadeuring: np14:51
adeuringhenninge: 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 IPersonRoles14:52
henningeadeuring: that is what I am doing now.14:52
adeuringhenninge: cool!14:52
henningejust the other way round, I am removing the prefix from the IPersonRoles attribute names.14:52
adeuringhenninge: yeah, sure ;)14:52
henningeadeuring: I meant, that is what the patch does.14:53
adeuringok14:53
=== salgado is now known as salgado-lunch
=== matsubara is now known as matsubara-lunch
adeuringhenninge: 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_names15:08
henningeadeuring: working on that atm, using set15:08
adeuringhenninge: great, thanks! otherwise, r=me, BTW15:08
henningeadeuring: cool, thank you!15:09
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: -, bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== beuno is now known as beuno-lunch
=== salgado-lunch is now known as salgado
salgadobac, sorry for the delay on that review16:18
=== salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
bacsalgado: np16:21
bacsalgado: 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:23
salgadobac, no, I was just wondering if it's currently possible to merge a dupe account with hidden email addresses16:24
bacsalgado: you cannot get the person via the picker but you can enter the id in the field16:25
salgadoright16:25
bacthanks for catching the missing test and the suggested improvements16:26
=== beuno-lunch is now known as beuno
=== matsubara-lunch is now known as matsubara
=== deryck is now known as deryck[lunch]
bacsalgado: fancy another?17:42
salgadobac, sure, why not?17:43
bacsalgado: https://code.edge.launchpad.net/~bac/launchpad/bug-407604-proj-desc/+merge/1702817:43
bacsalgado: i am about to leave for lunch and an appt so i won't be around to answer questions on IRC17:44
salgadothat's fine17:45
=== abentley1 is now known as abentley
=== adeuring changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== deryck[lunch] is now known as deryck
gary_posterEdwinGrubbs: 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_posterTo 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:22
gary_posterEdwinGrubbs: huh19:23
gary_posterEdwinGrubbs: did you get everything I just wrote?  It ended with "lines 138-148, written in zcml for each vocabulary definition?"19:23
EdwinGrubbsgary_poster: No, I didn't see that line.19:23
gary_posterEdwinGrubbs: what was the last line you got?19:23
gary_posterbefore I said "huh"19:23
gary_poster(if any)19:24
EdwinGrubbsgary_poster: the last line I got ended with "security proxy is a better approach.)"19:25
EdwinGrubbsgary_poster: can I use canView() instead of the try/except UnauthorizedError?19:25
gary_posterEdwinGrubbs: you could.  Why do you want to?19:26
gary_posterWhen we finish this I guess I'll paste in the next item :-)19:26
EdwinGrubbsgary_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:27
=== salgado changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado is now known as salgado-afk
gary_posterEdwinGrubbs: 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_posterexception 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_posterI'll copy and paste my next point then.19:34
gary_posterStill 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_posterIf 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_posterwell, more than the bit you read, anyway.19:35
EdwinGrubbsgary_poster: bac has taken over the reviewer meetings. I guess you missed the one on Wed.19:36
gary_posterEdwinGrubbs: cool.  Yeah, I was in Lean training with the OEM folks, since I wasn't with LP the first time around.19:36
EdwinGrubbsgary_poster: excuses, excuses19:37
gary_posterheh19:37
EdwinGrubbsgary_poster: __getslice__ is used because HugeVocabularyJSONView batches the results.19:37
gary_posterEdwinGrubbs: but the implementation handles slices in its __getitem__, right?19:38
gary_posterI mean, the vocabulary implementation19:38
EdwinGrubbsgary_poster: right19:39
gary_posterEdwinGribbs: cool.  Then are you OK with me being OK with what you have done there? :-)19:39
EdwinGrubbsgary_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:40
gary_posterEdwinGrubbs: cool, makes sense19:41
EdwinGrubbsgary_poster: yes, I approve of your approval. I guess I'm ready for the pasted text.19:41
gary_posterOK cool19:41
gary_posterFive chunks19:41
gary_posterFinally, 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_posterThe 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:41
gary_posterThe 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_posterI 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_posterThe 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_posterThat's the end of the paste.19:42
gary_posterI'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_posterThat is, I've tried to "step back" and I still don't see anything...19:43
gary_posterI believe the boiler plate is something on the order of this:19:46
gary_poster<allow interface="...IVocabularyFactory" /><allow interface="...IHugeVocabulary" />19:47
EdwinGrubbsgary_poster: exactly19:47
EdwinGrubbsgary_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
EdwinGrubbsgary_poster: I'm not sure what you meant about the first and second phase of zcml processing.19:49
gary_posterEdwinGrubbs: 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 direc19:55
gary_posterIn 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.19:55
gary_posterEdwinGrubbs: 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_posterSolving 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_posterFor now, I don't think we should introduce a surprising kludge or Yet Another ZCML directive.20:00
gary_posterThat's my take.20:00
EdwinGrubbsgary_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:08
EdwinGrubbsgary_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:15
EdwinGrubbsgary_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_posterEdwinGrubbs: So, first, for the "allow" concern, here's a reply: http://pastebin.ubuntu.com/353615/20:20
gary_posterEdwinGrubbs: oh...20:21
gary_posterEdwinGrubbs: you need to treat the IVocabularyFactory as something that the context provides20:21
gary_posterEdwinGrubbs: and you need to treat the resulting class as something that the context implements20:22
gary_posterEdwinGrubbs: that's why my idea doesn't work20:22
gary_posterI mean "you need to treat the IHugeVocabulary as something that the context implements"20:23
EdwinGrubbsgary_poster: yes, in lines 138-148, I had to use a separate ClassDirective instead of piling everything into self.allow()20:25
gary_posterEdwinGrubbs: 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:27
gary_posteradmittedly, you would still have to follow the "recipe" for a secured vocabulary, which is your point.20:28
EdwinGrubbsgary_poster: zcml seems anti-DRY. Maybe I could create a new paradigm, WET (We exemplify tediousness).20:30
EdwinGrubbs;-)20:30
gary_posterlol20:30
EdwinGrubbsgary_poster: despite my negativity, if you think that being explicit in the zcml is the best way to go, I'll do that.20:32
gary_posterEdwinGrubbs: "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_posterI'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_posterI'll summarize my position like this:20:35
gary_poster1) 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:36
gary_poster2) 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_poster3) I'd really rather not see the kludge in securedutility.20:37
gary_posterEdwinGrubbs: fair enough?  I need to run go pick up a son from school20:38
gary_posterEdwinGrubbs: back in a bit20:38
gary_poster'20:38
EdwinGrubbsgary_poster: go ahead, I'm getting distracted in another channel.20:39
=== matsubara is now known as matsubara-afk
gary_posterEdwinGrubbs: back fwiw20:55
gary_posterEdwinGrubbs2: I21:08
gary_posterhave been back for a while.  don't know if you saw that (I see you are having nick fun)21:08
EdwinGrubbs2gary_poster: I saw your 1/2/3 summary. It's ok. sed should make it easy enough.21:12
=== EdwinGrubbs2 is now known as EdwinGrubbs
gary_posterEdwinGrubbs: cool21:25
sinzuiflacoste: 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?22:43

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!