[PATCH 2 of 3 RFC] Remove all uses of hasattr(a, b) in favor of getattr(a, b, None)

[Augie Fackler]

On Mar 3, 2011, at 8:03 AM, Michael Haggerty wrote:
> 
> 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:

Good catch. I'll re-roll the series as a larger batch of patches and make sure to get this optimization in.

> 
>> 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

Does anyone in the wider mercurial-devel have a feeling on this one? It seems reasonable as a hedge against the day in about 2050 that we can be Python3 only...

> 
> 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