launchpad-reviewers team mailing list archive

[Colin Watson <mp+413587@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:no-safe-hasattr into launchpad:master.

Commit message:
Stop using lazr.restful.utils.safe_hasattr

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/413587

Python's builtin `hasattr` is fine as of Python 3.2.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:no-safe-hasattr into launchpad:master.

diff --git a/doc/guides/python.rst b/doc/guides/python.rst
index be09dfe..f2ede1a 100644
--- a/doc/guides/python.rst
+++ b/doc/guides/python.rst
@@ -326,12 +326,6 @@ Prefer `operator.attrgetter
 ``lambda``.  Remember that giving functions names makes the code that calls,
 passes and returns them easier to debug.
 
-Use of hasattr
-==============
-
-Use ``safe_hasattr`` from ``lazr.restful.utils`` instead of the built-in
-``hasattr`` function because the latter swallows exceptions.
-
 Database-related
 ================
 
diff --git a/lib/lp/app/browser/lazrjs.py b/lib/lp/app/browser/lazrjs.py
index 1d4a85f..c48cc09 100644
--- a/lib/lp/app/browser/lazrjs.py
+++ b/lib/lp/app/browser/lazrjs.py
@@ -17,10 +17,7 @@ __all__ = [
 
 from lazr.enum import IEnumeratedType
 from lazr.restful.declarations import LAZR_WEBSERVICE_EXPORTED
-from lazr.restful.utils import (
-    get_current_browser_request,
-    safe_hasattr,
-    )
+from lazr.restful.utils import get_current_browser_request
 import simplejson
 from zope.browserpage import ViewPageTemplateFile
 from zope.component import getUtility
@@ -325,8 +322,8 @@ class InlineEditPickerWidget(WidgetBase):
         if self.context is None:
             return None
         val = getattr(self.context, self.exported_field.__name__)
-        if val is not None and safe_hasattr(val, 'name'):
-            return getattr(val, 'name')
+        if val is not None:
+            return getattr(val, 'name', None)
         return None
 
     @property
@@ -508,7 +505,7 @@ class InlineMultiCheckboxWidget(WidgetBase):
         items = []
         style = ';'.join(['font-weight: normal', items_style])
         for item in vocabulary:
-            item_value = item.value if safe_hasattr(item, 'value') else item
+            item_value = getattr(item, 'value', item)
             checked = item_value in selected_items
             if linkify_items:
                 save_value = canonical_url(item_value, force_local_path=True)
@@ -561,7 +558,7 @@ def vocabulary_to_choice_edit_items(
     for item in vocab:
         # Introspect to make sure we're dealing with the object itself.
         # SimpleTerm objects have the object itself at item.value.
-        if safe_hasattr(item, 'value'):
+        if hasattr(item, 'value'):
             item = item.value
         if excluded_items and item in excluded_items:
             continue
diff --git a/lib/lp/app/doc/menus.txt b/lib/lp/app/doc/menus.txt
index b1e0203..c76ac01 100644
--- a/lib/lp/app/doc/menus.txt
+++ b/lib/lp/app/doc/menus.txt
@@ -284,7 +284,6 @@ can access `self.context`.
     >>> defineChecker(MenuLink, InterfaceChecker(ILink))
     >>> defineChecker(URI, NamesChecker(dir(URI)))
 
-    >>> from lazr.restful.utils import safe_hasattr
     >>> def summarise_links(menu, url=None, facet=None):
     ...     """List the links and their attributes."""
     ...     if url is not None:
@@ -296,7 +295,7 @@ can access `self.context`.
     ...         print('link %s' % link.name)
     ...         attributes = ('url', 'enabled', 'menu', 'selected', 'linked')
     ...         for attrname in attributes:
-    ...             if not safe_hasattr(link, attrname):
+    ...             if not hasattr(link, attrname):
     ...                 continue
     ...             print('    %s: %s' % (attrname, getattr(link, attrname)))
 
diff --git a/lib/lp/app/widgets/popup.py b/lib/lp/app/widgets/popup.py
index 34264ae..c6c869f 100644
--- a/lib/lp/app/widgets/popup.py
+++ b/lib/lp/app/widgets/popup.py
@@ -13,7 +13,6 @@ __all__ = [
     "VocabularyPickerWidget",
     ]
 
-from lazr.restful.utils import safe_hasattr
 import simplejson
 from zope.browserpage import ViewPageTemplateFile
 from zope.component import getUtility
@@ -130,8 +129,8 @@ class VocabularyPickerWidget(SingleDataHelper, ItemsWidgetBase):
         Default implementation is to return the 'name' attribute.
         """
         val = self._getFormValue()
-        if val is not None and safe_hasattr(val, 'name'):
-            return getattr(val, 'name')
+        if val is not None:
+            return getattr(val, 'name', None)
         return None
 
     @property
diff --git a/lib/lp/blueprints/browser/specificationtarget.py b/lib/lp/blueprints/browser/specificationtarget.py
index 0de51ca..e6b4e29 100644
--- a/lib/lp/blueprints/browser/specificationtarget.py
+++ b/lib/lp/blueprints/browser/specificationtarget.py
@@ -13,10 +13,7 @@ __all__ = [
 
 from operator import itemgetter
 
-from lazr.restful.utils import (
-    safe_hasattr,
-    smartquote,
-    )
+from lazr.restful.utils import smartquote
 from zope.browserpage import ViewPageTemplateFile
 from zope.component import (
     getMultiAdapter,
@@ -161,7 +158,7 @@ class HasSpecificationsView(LaunchpadView):
         # zope.browserpage.simpleviewclass.simple class that is magically
         # mixed in by the browser:page zcml directive the template defined in
         # the directive should be used.
-        if safe_hasattr(self, 'index'):
+        if hasattr(self, 'index'):
             return super().template
 
         # Sprints and Persons don't have a usage enum for blueprints, so we
diff --git a/lib/lp/registry/browser/milestone.py b/lib/lp/registry/browser/milestone.py
index 375f5e4..fb645c2 100644
--- a/lib/lp/registry/browser/milestone.py
+++ b/lib/lp/registry/browser/milestone.py
@@ -22,7 +22,6 @@ __all__ = [
     ]
 
 
-from lazr.restful.utils import safe_hasattr
 from zope.component import getUtility
 from zope.formlib import form
 from zope.interface import (
@@ -106,7 +105,7 @@ class MilestoneBreadcrumb(Breadcrumb):
     @property
     def text(self):
         milestone = IMilestoneData(self.context)
-        if safe_hasattr(milestone, 'code_name') and milestone.code_name:
+        if hasattr(milestone, 'code_name') and milestone.code_name:
             return '%s "%s"' % (milestone.name, milestone.code_name)
         else:
             return milestone.name
diff --git a/lib/lp/registry/model/product.py b/lib/lp/registry/model/product.py
index c3aa29a..f213c1d 100644
--- a/lib/lp/registry/model/product.py
+++ b/lib/lp/registry/model/product.py
@@ -18,7 +18,6 @@ import operator
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.restful.declarations import error_status
-from lazr.restful.utils import safe_hasattr
 import pytz
 from storm.expr import (
     And,
@@ -1531,13 +1530,13 @@ def get_precached_products(products, need_licences=False,
     caches = {product.id: get_property_cache(product)
         for product in products}
     for cache in caches.values():
-        if not safe_hasattr(cache, 'commercial_subscription'):
+        if not hasattr(cache, 'commercial_subscription'):
             cache.commercial_subscription = None
-        if need_licences and  not safe_hasattr(cache, '_cached_licenses'):
+        if need_licences and  not hasattr(cache, '_cached_licenses'):
             cache._cached_licenses = []
-        if need_packages and not safe_hasattr(cache, 'distrosourcepackages'):
+        if need_packages and not hasattr(cache, 'distrosourcepackages'):
             cache.distrosourcepackages = []
-        if need_series and not safe_hasattr(cache, 'series'):
+        if need_series and not hasattr(cache, 'series'):
             cache.series = []
 
     from lp.registry.model.distributionsourcepackage import (
@@ -1559,8 +1558,7 @@ def get_precached_products(products, need_licences=False,
             ProductSeries,
             ProductSeries.productID.is_in(product_ids)):
             series_cache = get_property_cache(series)
-            if (need_releases and
-                not safe_hasattr(series_cache, '_cached_releases')):
+            if need_releases and not hasattr(series_cache, '_cached_releases'):
                 series_cache._cached_releases = []
 
             series_caches[series.id] = series_cache
@@ -1573,7 +1571,7 @@ def get_precached_products(products, need_licences=False,
             for milestone, release, product_id in milestones_and_releases:
                 release_cache = get_property_cache(release)
                 release_caches[release.id] = release_cache
-                if not safe_hasattr(release_cache, 'files'):
+                if not hasattr(release_cache, 'files'):
                     release_cache.files = []
                 all_releases.append(release)
                 series_cache = series_caches[milestone.productseries.id]
diff --git a/lib/lp/services/apachelogparser/script.py b/lib/lp/services/apachelogparser/script.py
index 0c92c00..c5faf6e 100644
--- a/lib/lp/services/apachelogparser/script.py
+++ b/lib/lp/services/apachelogparser/script.py
@@ -6,7 +6,6 @@ __all__ = ['ParseApacheLogs']
 import glob
 import os
 
-from lazr.restful.utils import safe_hasattr
 from zope.component import getUtility
 
 from lp.app.errors import NotFoundError
@@ -115,10 +114,7 @@ class ParseApacheLogs(LaunchpadCronScript):
             fd.close()
             create_or_update_parsedlog_entry(first_line, parsed_bytes)
             self.txn.commit()
-            if safe_hasattr(fd, 'name'):
-                name = fd.name
-            else:
-                name = fd
+            name = getattr(fd, 'name', fd)
             self.logger.info('Finished parsing %s' % name)
 
         self.logger.info('Done parsing apache log files')
diff --git a/lib/lp/services/features/__init__.py b/lib/lp/services/features/__init__.py
index e58a026..85d9949 100644
--- a/lib/lp/services/features/__init__.py
+++ b/lib/lp/services/features/__init__.py
@@ -181,8 +181,6 @@ other environments that have no explicit setup and teardown::
 
 import threading
 
-from lazr.restful.utils import safe_hasattr
-
 
 __all__ = [
     'currentScope',
@@ -212,7 +210,7 @@ def uninstall_feature_controller():
 
     This function is used to create a pristine environment in tests.
     """
-    if safe_hasattr(per_thread, 'features'):
+    if hasattr(per_thread, 'features'):
         del per_thread.features
 
 
diff --git a/lib/lp/services/webapp/adapter.py b/lib/lp/services/webapp/adapter.py
index 653983d..c4b8f72 100644
--- a/lib/lp/services/webapp/adapter.py
+++ b/lib/lp/services/webapp/adapter.py
@@ -12,10 +12,7 @@ from time import time
 import traceback
 import warnings
 
-from lazr.restful.utils import (
-    get_current_browser_request,
-    safe_hasattr,
-    )
+from lazr.restful.utils import get_current_browser_request
 from psycopg2.extensions import (
     ISOLATION_LEVEL_AUTOCOMMIT,
     ISOLATION_LEVEL_READ_COMMITTED,
@@ -536,7 +533,7 @@ class LaunchpadSessionDatabase(Postgres):
 
         flags = _get_dirty_commit_flags()
         raw_connection = super(LaunchpadSessionDatabase, self).raw_connect()
-        if safe_hasattr(raw_connection, 'auto_close'):
+        if hasattr(raw_connection, 'auto_close'):
             raw_connection.auto_close = False
         raw_connection.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
         _reset_dirty_commit_flags(*flags)
@@ -724,7 +721,7 @@ class StoreSelector:
     @staticmethod
     def push(db_policy):
         """See `IStoreSelector`."""
-        if not safe_hasattr(_local, 'db_policies'):
+        if not hasattr(_local, 'db_policies'):
             _local.db_policies = []
         db_policy.install()
         _local.db_policies.append(db_policy)
diff --git a/lib/lp/services/webapp/errorlog.py b/lib/lp/services/webapp/errorlog.py
index f7a83fe..8c66532 100644
--- a/lib/lp/services/webapp/errorlog.py
+++ b/lib/lp/services/webapp/errorlog.py
@@ -8,10 +8,7 @@ from itertools import repeat
 import operator
 import re
 
-from lazr.restful.utils import (
-    get_current_browser_request,
-    safe_hasattr,
-    )
+from lazr.restful.utils import get_current_browser_request
 import oops.createhooks
 import oops_amqp
 from oops_datedir_repo import DateDirRepo
@@ -159,7 +156,7 @@ def attach_http_request(report, context):
         return
     # XXX jamesh 2005-11-22: Temporary fix, which Steve should
     #      undo. URL is just too HTTPRequest-specific.
-    if safe_hasattr(request, 'URL'):
+    if hasattr(request, 'URL'):
         # URL's are byte strings, but possibly str() will fail - safe_unicode
         # handles all those cases.  This is strictly double handling as a
         # URL should never have unicode characters in it anyway (though it
@@ -180,7 +177,7 @@ def attach_http_request(report, context):
     missing = object()
     principal = getattr(request, 'principal', missing)
 
-    if safe_hasattr(principal, 'getLogin'):
+    if hasattr(principal, 'getLogin'):
         login = principal.getLogin()
     elif principal is missing or principal is None:
         # Request has no principal (e.g. scriptrequest)
diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
index 27c29d4..fafe9b5 100644
--- a/lib/lp/services/webapp/publication.py
+++ b/lib/lp/services/webapp/publication.py
@@ -12,7 +12,6 @@ import threading
 import time
 import traceback
 
-from lazr.restful.utils import safe_hasattr
 from lazr.uri import (
     InvalidURIError,
     URI,
@@ -410,7 +409,7 @@ class LaunchpadBrowserPublication(
             # is accessible in the instance __name__ attribute. We use
             # that if it's available, otherwise fall back to the class
             # name.
-            if safe_hasattr(view, '__name__'):
+            if hasattr(view, '__name__'):
                 view_name = view.__name__
             else:
                 view_name = view.__class__.__name__
@@ -419,7 +418,7 @@ class LaunchpadBrowserPublication(
             context_name = context.__class__.__name__
             # Is this a view of a generated view class,
             # such as ++model++ view of Product:+bugs. Recurse!
-            if ' ' in context_name and safe_hasattr(context, 'context'):
+            if ' ' in context_name and hasattr(context, 'context'):
                 return self.constructPageID(context, context.context, names)
             view_names = ':'.join(names)
             pageid = '%s:%s' % (context_name, view_names)
diff --git a/lib/lp/services/webapp/vocabulary.py b/lib/lp/services/webapp/vocabulary.py
index d663e85..cffb141 100644
--- a/lib/lp/services/webapp/vocabulary.py
+++ b/lib/lp/services/webapp/vocabulary.py
@@ -23,7 +23,6 @@ __all__ = [
 
 from collections import namedtuple
 
-from lazr.restful.utils import safe_hasattr
 import six
 from storm.base import Storm
 from storm.store import EmptyResultSet
@@ -253,7 +252,7 @@ class FilteredVocabularyBase:
     # parameter to a VocabularyFilter instance.
     def __getattribute__(self, name):
         func = object.__getattribute__(self, name)
-        if (safe_hasattr(func, '__call__')
+        if (hasattr(func, '__call__')
                 and (
                     func.__name__ == 'searchForTerms'
                     or func.__name__ == 'search')):
diff --git a/lib/lp/soyuz/browser/build.py b/lib/lp/soyuz/browser/build.py
index ab519b7..e997277 100644
--- a/lib/lp/soyuz/browser/build.py
+++ b/lib/lp/soyuz/browser/build.py
@@ -21,7 +21,6 @@ from itertools import groupby
 from operator import attrgetter
 
 from lazr.batchnavigator import ListRangeFactory
-from lazr.restful.utils import safe_hasattr
 from zope.component import getUtility
 from zope.interface import (
     implementer,
@@ -525,7 +524,7 @@ class BuildRecordsView(LaunchpadView):
         """Return the architecture options for the context."""
         # Guard against contexts that cannot tell us the available
         # distroarchseries.
-        if safe_hasattr(self.context, 'architectures') is False:
+        if hasattr(self.context, 'architectures') is False:
             return []
 
         # Grab all the architecture tags for the context.
diff --git a/lib/lp/testing/menu.py b/lib/lp/testing/menu.py
index c8328b3..bce4988 100644
--- a/lib/lp/testing/menu.py
+++ b/lib/lp/testing/menu.py
@@ -8,7 +8,6 @@ __all__ = [
     'make_fake_request',
     ]
 
-from lazr.restful.utils import safe_hasattr
 from zope.security.management import (
     endInteraction,
     newInteraction,
@@ -64,7 +63,7 @@ def summarise_tal_links(links):
             print('link %s' % link.name)
             attributes = ('url', 'enabled', 'menu', 'selected', 'linked')
             for attrname in attributes:
-                if not safe_hasattr(link, attrname):
+                if not hasattr(link, attrname):
                     continue
                 print('    %s:' % attrname, getattr(link, attrname))
         else: