[PATCH 2 of 3 RFC] Remove all uses of hasattr(a, b) in favor of getattr(a, b, None)
Michael Haggerty
mhagger at alum.mit.edu
Thu Mar 3 08:03:20 CST 2011
On 03/02/2011 06:36 AM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <durin42 at gmail.com>
> # Date 1299024193 21600
> # Branch stable
> # Node ID 75e012dfe383900f1932bdb0831eb44a11d35876
> # Parent f202bb8bc5e81a41893144065cc634a786d549b0
> Remove all uses of hasattr(a, b) in favor of getattr(a, b, None)
>
> hasattr() in Python swallows all exceptions rather than only
> AttributeError, making it unsafe for general use. getattr(a, b, None)
> is a mostly-compatible replacement (it fails to distinguish missing
> attributes and None-value attributes) that we can use instead.
I understand that you were doing a largely automatic replacement of
hasattr(), but there are several places where the default value of
getattr() can be set more usefully than None to shorten the code and
avoid a double-lookup. A few examples:
> diff --git a/contrib/setup3k.py b/contrib/setup3k.py
> --- a/contrib/setup3k.py
> +++ b/contrib/setup3k.py
> @@ -8,7 +8,8 @@
> from lib2to3.refactor import get_fixers_from_package as getfixers
>
> import sys
> -if not hasattr(sys, 'version_info') or sys.version_info < (2, 4, 0, 'final'):
> +if (not getattr(sys, 'version_info', None)
> + or sys.version_info < (2, 4, 0, 'final')):
if getattr(sys, 'version_info', (0,)) < (2, 4, 0, 'final'):
> @@ -236,7 +237,7 @@
> try:
> build_ext.build_extension(self, ext)
> except CCompilerError:
> - if not hasattr(ext, 'optional') or not ext.optional:
> + if not getattr(ext, 'optional', None) or not ext.optional:
if not getattr(ext, 'optional', False):
> diff --git a/mercurial/byterange.py b/mercurial/byterange.py
> --- a/mercurial/byterange.py
> +++ b/mercurial/byterange.py
> @@ -103,7 +103,7 @@
> """This effectively allows us to wrap at the instance level.
> Any attribute not found in _this_ object will be searched for
> in self.fo. This includes methods."""
> - if hasattr(self.fo, name):
> + if getattr(self.fo, name, None):
> return getattr(self.fo, name)
> raise AttributeError(name)
Wouldn't it be enough here just to
# If this raises AttributeError, let it escape:
return getattr(self.fo, name)
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -182,7 +182,7 @@
> return -1
>
> def aliasargs(fn):
> - if hasattr(fn, 'args'):
> + if getattr(fn, 'args', None):
> return fn.args
> return []
return getattr(fn, 'args', [])
etc.
For the other cases, maybe it would make sense to define a
def safe_hasattr(o, name):
try:
getattr(o, name)
except AttributeError:
return False
else:
return True
Such a helper function would be safer (no problems with values that
evaluate to False), more obvious, and easier to replace if/when
Mercurial migrates to Python 3.
Michael
--
Michael Haggerty
mhagger at alum.mit.edu
http://softwareswirl.blogspot.com/
More information about the Mercurial-devel
mailing list