In User.py the method is defined as def getUserById(self, id, default=_marker): try: return self.getUser(id) except: if default is _marker: raise return default I am wondering whether anybody actually depends on the fact that getUser is supposed to raise an exception. I know of no user folder implementation that actually does that. Rather, I'd like to change it to the way it's done in LDAPUserFolder where the method looks (more or less) like: def getUserById(self, id, default=_marker): user = self.getUser(id) if user is None and default is not _marker: return default return user Any objections? Stefan -- The time has come to start talking about whether the emperor is as well dressed as we are supposed to think he is. /Pete McBreen/
In article <F312FBAF-4C54-11D9-9062-000D93325646@epy.co.at> you write:
In User.py the method is defined as
def getUserById(self, id, default=_marker): try: return self.getUser(id) except: if default is _marker: raise return default
I am wondering whether anybody actually depends on the fact that getUser is supposed to raise an exception. I know of no user folder implementation that actually does that.
Rather, I'd like to change it to the way it's done in LDAPUserFolder where the method looks (more or less) like:
def getUserById(self, id, default=_marker): user = self.getUser(id) if user is None and default is not _marker: return default return user
Can we have instead: def getUserById(self, id, default=_marker): """Return the user corresponding to the given id. Raises a KeyError if the user does not exist and no default is provided. """ user = self.getUser(id) if user is not None: return user if default is not _marker: return default raise KeyError(id) Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
On Dec 13, 2004, at 17:15, Florent Guillaume wrote:
Can we have instead:
def getUserById(self, id, default=_marker): """Return the user corresponding to the given id.
Raises a KeyError if the user does not exist and no default is provided. """ user = self.getUser(id) if user is not None: return user if default is not _marker: return default raise KeyError(id)
What is the advantage of throwing a KeyError over simply returning None? I don't see the reason for "getUser" and "getUserById" behaving in different ways here. They should both behave similar. jens
Jens wrote:
On Dec 13, 2004, at 17:15, Florent Guillaume wrote:
Can we have instead:
def getUserById(self, id, default=_marker): """Return the user corresponding to the given id.
Raises a KeyError if the user does not exist and no default is provided. """ user = self.getUser(id) if user is not None: return user if default is not _marker: return default raise KeyError(id)
What is the advantage of throwing a KeyError over simply returning None?
Well it's standard usage all over Zope for all methods that accept an optional default to raise an error if no default is provided. Why accept a default when None is returned anyway if no default is provided, and None is not an acceptable user in itself ? That just doesn't make sense.
I don't see the reason for "getUser" and "getUserById" behaving in different ways here. They should both behave similar.
Chris clarified that getUser is really getUserByName so I'm ok with that. If the method should be never raise and return None if nothing's provided, then let's use simply: def getUserById(self, id, default=None): user = self.getUser(id) if user is not None: return user return default or even: def getUserById(self, id, default=None): return self.getUser(id) or default Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
On Tue, 2004-12-14 at 12:37 +0100, Florent Guillaume wrote:
def getUserById(self, id, default=None): user = self.getUser(id) if user is not None: return user return default
or even:
def getUserById(self, id, default=None): return self.getUser(id) or default
FWIW, I assume it's understood that neither of these will do the right thing for anything except the default Zope user folder (aka "UserFolder"), so it's arguable that these implementations probably shouldn't go into the BasicUserFolder base class (which is a base class for "UserFolder" and maybe a lot of other user folders). That said, I think every other user folder implementation on the planet overrides these methods. FWIW, the user folder API is a complete mess (there really is no sane API). It might be best to just step away slowly from this unless you want to own it. ;-) - C
Chris McDonough wrote:
On Tue, 2004-12-14 at 12:37 +0100, Florent Guillaume wrote:
def getUserById(self, id, default=None): user = self.getUser(id) if user is not None: return user return default
or even:
def getUserById(self, id, default=None): return self.getUser(id) or default
FWIW, I assume it's understood that neither of these will do the right thing for anything except the default Zope user folder (aka "UserFolder"), so it's arguable that these implementations probably shouldn't go into the BasicUserFolder base class (which is a base class for "UserFolder" and maybe a lot of other user folders). That said, I think every other user folder implementation on the planet overrides these methods.
Well if for them id != name, they have to override them.
FWIW, the user folder API is a complete mess (there really is no sane API). It might be best to just step away slowly from this unless you want to own it. ;-)
No argument there :) Stefan wanted some cleanup, why not. Defining the precise semantics is the first step anyway. Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
Hm, I'd rather not raise exceptions. getUserById was introduced in Zope 2.2 and since then has never raised anything. From what I can see it is used as a kind of alias for getUser, expected to return None if the user does not exist. So, my intention is to make 'default' work, not to make it raise. I see that raising may seem like a more consistent idea (think dict.get) but it may also break existing code [1]. My secret agenda is to unify the behavior of getUserById across user folders. As it is now, LDAPUserFolder, GRUF, etc all implement their own little variations because the default is so "strange". Stefan [1] getUserById is not used by Zope, only in a safe way (with default=None) by CMF, and only twice by Plone. But it is used by e.g. CMFMember and, somewhat unfortunately, is all over ZTC-based unit tests. On 13. Dez 2004, at 17:15, Florent Guillaume wrote:
Can we have instead:
def getUserById(self, id, default=_marker): """Return the user corresponding to the given id.
Raises a KeyError if the user does not exist and no default is provided. """ user = self.getUser(id) if user is not None: return user if default is not _marker: return default raise KeyError(id)
-- The time has come to start talking about whether the emperor is as well dressed as we are supposed to think he is. /Pete McBreen/
On Mon, 2004-12-13 at 19:22 +0100, Stefan H. Holek wrote:
Hm, I'd rather not raise exceptions.
getUserById was introduced in Zope 2.2 and since then has never raised anything. From what I can see it is used as a kind of alias for getUser, expected to return None if the user does not exist.
FWIW, it is indeed that when you use a basic user folder. But when you use a user folder that makes the distinction between usernames and userids, getUser operates like it should be named "getUserByName". - C
In article <E6A43D56-4D33-11D9-8ED8-000D93325646@epy.co.at> you write:
Hm, I'd rather not raise exceptions.
getUserById was introduced in Zope 2.2 and since then has never raised anything. From what I can see it is used as a kind of alias for getUser, expected to return None if the user does not exist.
So, my intention is to make 'default' work, not to make it raise. I see that raising may seem like a more consistent idea (think dict.get) but it may also break existing code [1].
Well dict.get is the exception in the python world. Everything else that has an optional default value raises an exception if no result is available and the default value is not provided.
My secret agenda is to unify the behavior of getUserById across user folders. As it is now, LDAPUserFolder, GRUF, etc all implement their own little variations because the default is so "strange".
Indeed, these methods lacks clear interfaces. FWIW CPSUserFolder is at: http://cvs.nuxeo.org/cgi-bin/viewcvs.cgi/CPS3/CPSUserFolder/CPSUserFolder.py... Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
Stefan H. Holek wrote:
In User.py the method is defined as
def getUserById(self, id, default=_marker): try: return self.getUser(id) except: if default is _marker: raise return default
I am wondering whether anybody actually depends on the fact that getUser is supposed to raise an exception. I know of no user folder implementation that actually does that.
Rather, I'd like to change it to the way it's done in LDAPUserFolder where the method looks (more or less) like:
def getUserById(self, id, default=_marker): user = self.getUser(id) if user is None and default is not _marker: return default return user
Any objections?
def getUserById(self, id, default=None): user = self.getUser(id) if user is None: return default return user Seems easier to do the same thing, And in fact, if it shouldn't raise anything, then it shouldn't have a default at all. def getUserById(self, id): return self.getUser(id) But that would break anything that actually calls default... -- Lennart Regebro, Nuxeo http://www.nuxeo.com/ CPS Content Management http://www.cps-project.org/
participants (5)
-
Chris McDonough -
Florent Guillaume -
Jens Vagelpohl -
Lennart Regebro -
Stefan H. Holek