| jtv | Do we still require the backquotes for """See IThisClass.""" docstrings? | 12:02 |
|---|---|---|
| noodles775 | It's still listed here: https://dev.launchpad.net/PythonStyleGuide#Docstrings | 12:13 |
| jtv | noodles775: Thanks! Then that settles it for me, and I'll continue to be a prick about it. :-) | 12:22 |
| noodles775 | heh... I meant that if there's no reason to use it, update the wiki (and bring it up for the review meeting?). I assume it's used for pydoctor, but have never run our code through pydoctor myself. | 12:28 |
| === matsubara-afk is now known as matsubara | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| leonardr | abentley, can i trouble you to review https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/server-takes-precedence/+merge/28059 ? | 15:32 |
| abentley | leonardr, sure. | 15:32 |
| abentley | leonardr, why are 'team' and 'person' different types? | 15:33 |
| === matsubara is now known as matsubara-afk | ||
| leonardr | abentley: 'team' has some features like membership_policy that 'person' doesn't have | 15:35 |
| leonardr | same reason ITeam and IPerson are different interfaces | 15:35 |
| abentley | leonardr, it seems like this behaviour should require that the returned type is derived from the expected type, don't you think? | 15:43 |
| leonardr | abentley: i don't think so--i think if the client and the server disagree for any reason, the client is just wrong. in real life, the only reason they'll disagree is because of an inheritance situation | 15:55 |
| leonardr | but there's no way to represent inheritance in wadl, so we can't be much more sophisticated | 15:56 |
| abentley | leonardr, I don't think it matters who's wrong. If the returned object's interface isn't a superset of the expected interface, chaos will result. | 15:57 |
| abentley | leonardr, I am having trouble finding out where the 'people' collection is defined, but PersonSet always returns IPerson, not ITeam, which seems to contradict your documentation. | 16:00 |
| abentley | leonardr, even findTeam returns IPerson. | 16:00 |
| leonardr | abentley: let's say we had a magic way of knowing. if we detected a non-inheritance scenario, we'd throw an exception. that's exactly what'll happen now: an exception saying 'a bar-type object doesn't have a property "onlypresentinfoo"' | 16:00 |
| leonardr | the 'people' collection i refer to is the client-side collection in launchpadlib | 16:01 |
| leonardr | on the server side, we declare the most general interface (though findTeam really should return ITeam). on the client side, we expect the most specific interface | 16:01 |
| leonardr | that is, it's guaranteed that everything coming out of PersonSet implements at least IPerson | 16:02 |
| leonardr | but if you grab an object from PersonSet, and you know *nothing* about it, the most convenient thing to do for your end-user is to let them treat it as ITeam if they think it's an ITeam, and raise an exception if that assumption turns out to be invalid | 16:03 |
| leonardr | that decision could go the other way, and this branch would still be needed, but launchpadlib wouldn't be as useful | 16:04 |
| leonardr | you couldn't do launchpad.people['team'].membership_policy | 16:05 |
| === salgado is now known as salgado-lunch | ||
| abentley | leonardr, don't you need to load the object to evaluate launchpad.people['team'].membership_policy ? | 16:22 |
| leonardr | only if 'membership_policy' is something that could conceivably be a property of launchpad.people['team'] | 16:23 |
| leonardr | if you do launchpad.people['team'].nosuchproperty it won't load the object, you'll just get an error | 16:23 |
| leonardr | the question is whether or not membership_policy (or anything held by teams but not persons) should look like nosuchproperty | 16:24 |
| abentley | leonardr, it seems like it would be a programming error to try to access launchpad.people['team'].nosuchproperty and therefore making avoid retrieving the object is not a great advantage. | 16:26 |
| leonardr | i agree it's not a great advantage, but that's how it works now. a request will only go over the wire if it conforms with what the client knows about the application based on the wadl | 16:27 |
| leonardr | by contrast, launchpad.people['person'].membership_policy will go over the wire and _then_ raise an exception | 16:28 |
| leonardr | the important thing is that both cases raise exceptions | 16:28 |
| abentley | Can you give me an example of a case where there wouldn't be an exception? | 16:29 |
| leonardr | launchpad.people['person'].name or launchpad.people['team'].membership_policy | 16:30 |
| leonardr | in the first case, the property is present in both team and person | 16:30 |
| leonardr | in the second case, the property is only present for team, but fortunately the object happens to be a team (according to the server) | 16:30 |
| abentley | leonardr, can you give me an example where the object is a person, but appears to be a team? The first one I can think of is dir(person) | 16:34 |
| leonardr | abentley, i'm about to push a launchpadlib branch that will hopefully make it clearer | 16:35 |
| leonardr | but, to answer your question, launchpad.people['person'] will appear to be a team until the request is made. so, yes, dir(person) and .lp_attributes will show team attributes | 16:36 |
| leonardr | once the request is made, dir(person) will change to be more accurate | 16:36 |
| leonardr | abentley: https://code.edge.launchpad.net/~leonardr/launchpadlib/shim-users/+merge/28067 | 16:37 |
| krkhan | gmb: thanks for the review. i'll post on the list tonight for suggestions and see how it goes | 16:55 |
| gmb | krkhan, Cool. | 16:55 |
| abentley | leonardr, why doesn't setting "httplib2.debuglevel = 0" make the test always pass? | 17:02 |
| leonardr | abentley: going to go out on a limb here and suppose that you have the wrong idea about what httplib2.debuglevel does? | 17:03 |
| abentley | leonardr, could be. | 17:04 |
| leonardr | all it does is show the requests as they're being made, so that i can demonstrate that requests are made later rather than sooner | 17:04 |
| leonardr | it doesn't affect the test environment itself | 17:04 |
| abentley | leonardr, I think that's what I expected. | 17:07 |
| abentley | leonardr, so why does line 56 show a GET? | 17:07 |
| leonardr | ah | 17:07 |
| leonardr | when you create a client object, it inherits the current value of httplib2.debuglevel | 17:07 |
| leonardr | it would be better to set httplib2.debuglevel later, for readability, but technically it doesn't matter--you can unset it anytime after you create the client | 17:08 |
| leonardr | i'll make the change | 17:08 |
| abentley | leonardr, okay, cool. | 17:08 |
| abentley | leonardr, it would be nice if you could create it with "debug_service = CookbookWebServiceClient(_http_debuglevel=1)" to reflect this. | 17:09 |
| leonardr | that's a good idea but i'd rather do that in a separate branch, since that would affect a bunch of tests | 17:12 |
| abentley | leonardr, that's fine. I don't require it, just an idea. | 17:13 |
| leonardr | ok | 17:13 |
| abentley | Could you update the documentation to mention that WADL doesn't provide a way to find out inheritance relationships? | 17:15 |
| abentley | leonardr, also, did you consider allowing collections claim that they provide multiple types? | 17:16 |
| abentley | leonardr, that would allow a shim to be an IPersonOrITeam shim, and presumably it could find out which it is when it needs to. | 17:17 |
| leonardr | abentley: yes, that was my original (much more complicated plan), until flacoste said that team is and can only be a superset of person | 17:17 |
| leonardr | and the way this works in practice is very similar to that | 17:17 |
| leonardr | since team is a superset of person, IPersonOrTeam looks just like ITeam | 17:17 |
| leonardr | and any attempt to explicitly find out what kind of object a specific object is will collapse the wavefunction by making an http request | 17:18 |
| abentley | leonardr, I thought IPersonOrTeam would make http requests when dir(IPersonOrTeam) was invoked, etc. | 17:19 |
| leonardr | i see | 17:19 |
| james_w | salgado-lunch: https://code.edge.launchpad.net/~james-w/launchpad/copy-archive-test-refactor/+merge/28073 is the branch I was talking about if you could review please. | 17:20 |
| leonardr | i don't think that's worth the work, and if it is, we can do that work just as easily with this branch as a base | 17:20 |
| abentley | leonardr, approved, but I'd like to see an explanation of why you didn't consider inheritance (WADL doesn't provide that) in the docs. | 17:24 |
| leonardr | ok | 17:24 |
| leonardr | abentley, want to look at the launchpadlib branch too? (if not, gary has expressed an interest) | 17:25 |
| gary_poster | (a willingness :-) ) | 17:25 |
| abentley | gary_poster, if you don't mind, there are some issues I'd like to chase up. | 17:26 |
| gary_poster | abentley, leonardr, will do | 17:26 |
| === salgado-lunch is now known as salgado | ||
| salgado | james_w, I'll get to it in a minute | 17:41 |
| james_w | thanks | 17:41 |
| === deryck is now known as deryck[lunch] | ||
| gary_poster | leonardr: launchpadlib branch approved, no comments | 17:48 |
| leonardr | gary: cool. lazr.restfulclient release is out, launchpadlib release will follow, then i'm going to go to bed | 17:49 |
| gary_poster | ok, leonardr | 17:49 |
| leonardr | gary: done, see you tomorrow | 17:52 |
| === deryck[lunch] is now known as deryck | ||
| === sinzui changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| sinzui | is anyone available for a 47 line fix: https://code.edge.launchpad.net/~sinzui/launchpad/unique-openid-identifiers-0/+merge/28081 | 18:59 |
| sinzui | abentley, are you available for a 47 line fix: https://code.edge.launchpad.net/~sinzui/launchpad/unique-openid-identifiers-0/+merge/28081 | 20:08 |
| abentley | sinzui, sure. | 20:08 |
| === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | sinzui, openid identifiers are derived from account names? | 20:13 |
| sinzui | no random stringd | 20:13 |
| sinzui | sample data sucks and uses | 20:13 |
| abentley | sinzui, so the random strings aren't random enough? | 20:13 |
| sinzui | the randoms get reused apparently | 20:14 |
| sinzui | I saw something like merge-Z3dgy45 and Z3dgy45 in staging. the latter cannot be merged | 20:15 |
| abentley | sinzui, that doesn't sound like a good idea... | 20:15 |
| sinzui | We do not control openid identifiers | 20:15 |
| abentley | sinzui, Ah, that's canonical-foundations? | 20:15 |
| EdwinGrubbs | abentley: will you have time to review this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-35728-bugtracker-overlay/+merge/28100 | 20:16 |
| abentley | EdwinGrubbs, I expect so. | 20:16 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: sinzui || queue: [sinzui, Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | sinzui, "dash which does not occur in SSO identifier." should probably be "dash, which does not occur in SSO identifiers." | 20:18 |
| sinzui | abentley, ISD, canonical-identity-provider reuses identifier tokens. I was surprised. In my original fix I wrote that since we know the identifier is unique and cannot contain a dash, we could prepend 'merge-' and there was no chance of future conflict... | 20:18 |
| sinzui | I was proven wrong in 10 days | 20:19 |
| abentley | sinzui, if you are replying to my last statement, I was just nitpicking grammar. | 20:20 |
| sinzui | abentley, no, I was trying to provide backgroind to "that's canonical-foundations?" | 20:21 |
| * sinzui is being pedantic, sorry | 20:21 | |
| abentley | sinzui, ah, I missed that you mentioned ISD in there. | 20:22 |
| abentley | sinzui, I think it would be good to add a unit test to ensure that merging accounts works when you have duplicate identifiers. | 20:23 |
| sinzui | yes, I should do that | 20:23 |
| abentley | sinzui, the change itself looks fine. | 20:23 |
| abentley | sinzui, ping me when you have that test. | 20:24 |
| sinzui | okay | 20:24 |
| abentley | sinzui, also, this sinzui in the queue: is that the same thing? | 20:24 |
| sinzui | yep | 20:24 |
| === abentley changed the topic of #launchpad-reviews to: abentley || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | EdwinGrubbs, could you explain 395-397, please? | 20:38 |
| === abentley changed the topic of #launchpad-reviews to: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | abentley: it's not really important now that we normally run windmill tests through the test runner so we always have a clean database. I copied it from test_add_milestone. I can remove it if you want. | 20:57 |
| abentley | EdwinGrubbs, please do, then. | 20:58 |
| === abentley changed the topic of #launchpad-reviews to: is On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| sinzui | abentley, check your email for my reply and incremental diff | 22:05 |
| abentley | sinzui, I am EOD. | 22:06 |
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!