[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