launchpad-reviewers team mailing list archive

[Colin Watson <mp+454020@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:bugbear-getattr-setattr into launchpad:master.

Commit message:
Remove useless uses of getattr/setattr

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

`flake8-bugbear` points out:

  B009: Do not call getattr(x, 'attr'), instead use normal property access: x.attr. Missing a default to getattr will cause an AttributeError to be raised for non-existent properties. There is no additional safety in using getattr if you know the attribute name ahead of time.

  B010: Do not call setattr(x, 'attr', val), instead use normal property access: x.attr = val. There is no additional safety in using setattr if you know the attribute name ahead of time.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:bugbear-getattr-setattr into launchpad:master.

diff --git a/lib/lp/answers/tests/test_question_workflow.py b/lib/lp/answers/tests/test_question_workflow.py
index c0e61ce..2569042 100644
--- a/lib/lp/answers/tests/test_question_workflow.py
+++ b/lib/lp/answers/tests/test_question_workflow.py
@@ -426,7 +426,7 @@ class RequestInfoTestCase(BaseAnswerTrackerWorkflowTestCase):
         self.assertRaises(Unauthorized, getattr, self.question, "requestInfo")
 
         login_person(self.answerer)
-        getattr(self.question, "requestInfo")
+        self.question.requestInfo
 
 
 class GiveInfoTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -473,7 +473,7 @@ class GiveInfoTestCase(BaseAnswerTrackerWorkflowTestCase):
         self.assertRaises(Unauthorized, getattr, self.question, "giveInfo")
 
         login_person(self.owner)
-        getattr(self.question, "giveInfo")
+        self.question.giveInfo
 
 
 class GiveAnswerTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -588,7 +588,7 @@ class GiveAnswerTestCase(BaseAnswerTrackerWorkflowTestCase):
         self.assertRaises(Unauthorized, getattr, self.question, "giveAnswer")
 
         login_person(self.answerer)
-        getattr(self.question, "giveAnswer")
+        self.question.giveAnswer
 
 
 class LinkFAQTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -680,7 +680,7 @@ class LinkFAQTestCase(BaseAnswerTrackerWorkflowTestCase):
         self.assertRaises(Unauthorized, getattr, self.question, "linkFAQ")
 
         login_person(self.answerer)
-        getattr(self.question, "linkFAQ")
+        self.question.linkFAQ
 
 
 class ConfirmAnswerTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -856,7 +856,7 @@ class ConfirmAnswerTestCase(BaseAnswerTrackerWorkflowTestCase):
         )
 
         login_person(self.owner)
-        getattr(self.question, "confirmAnswer")
+        self.question.confirmAnswer
 
 
 class ReopenTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -971,7 +971,7 @@ class ReopenTestCase(BaseAnswerTrackerWorkflowTestCase):
         self.assertRaises(Unauthorized, getattr, self.question, "reopen")
 
         login_person(self.owner)
-        getattr(self.question, "reopen")
+        self.question.reopen
 
 
 class ExpireQuestionTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -1014,7 +1014,7 @@ class ExpireQuestionTestCase(BaseAnswerTrackerWorkflowTestCase):
         )
 
         login_person(self.answerer)
-        getattr(self.question, "expireQuestion")
+        self.question.expireQuestion
 
 
 class RejectTestCase(BaseAnswerTrackerWorkflowTestCase):
@@ -1103,13 +1103,10 @@ class RejectTestCase(BaseAnswerTrackerWorkflowTestCase):
         # clear authorization cache for check_permission
         clear_cache()
         self.assertTrue(
-            getattr(self.question, "reject"),
-            "Answer contact cannot reject question.",
+            self.question.reject, "Answer contact cannot reject question."
         )
         login_person(self.admin)
-        self.assertTrue(
-            getattr(self.question, "reject"), "Admin cannot reject question."
-        )
+        self.assertTrue(self.question.reject, "Admin cannot reject question.")
 
     def testRejectPermission_indirect_answer_contact(self):
         # Indirect answer contacts (for a distribution) can reject
@@ -1121,6 +1118,5 @@ class RejectTestCase(BaseAnswerTrackerWorkflowTestCase):
         self.answerer.addLanguage(getUtility(ILanguageSet)["en"])
         self.ubuntu.addAnswerContact(self.answerer, self.answerer)
         self.assertTrue(
-            getattr(self.question, "reject"),
-            "Answer contact cannot reject question.",
+            self.question.reject, "Answer contact cannot reject question."
         )
diff --git a/lib/lp/blueprints/model/sprint.py b/lib/lp/blueprints/model/sprint.py
index 15f20ea..7b5fc16 100644
--- a/lib/lp/blueprints/model/sprint.py
+++ b/lib/lp/blueprints/model/sprint.py
@@ -425,7 +425,7 @@ class HasSprintsMixin:
 
         Subclasses must overwrite this method if it doesn't suit them.
         """
-        table = getattr(self, "__storm_table__")
+        table = self.__storm_table__
         return [
             getattr(Specification, table.lower()) == self,
             Specification.id == SprintSpecification.specification_id,
diff --git a/lib/lp/code/model/branchtarget.py b/lib/lp/code/model/branchtarget.py
index 998c391..ceb60fa 100644
--- a/lib/lp/code/model/branchtarget.py
+++ b/lib/lp/code/model/branchtarget.py
@@ -184,10 +184,7 @@ class PackageBranchTarget(_BaseBranchTarget):
 
         result = sorted_version_numbers(
             result,
-            key=lambda branch_info: (
-                getattr(branch_info[1], "name"),
-                branch_info[0].id,
-            ),
+            key=lambda branch_info: (branch_info[1].name, branch_info[0].id),
         )
 
         if limit_results is not None:
@@ -438,10 +435,7 @@ class ProductBranchTarget(_BaseBranchTarget):
 
         result = sorted_version_numbers(
             result,
-            key=lambda branch_info: (
-                getattr(branch_info[1], "name"),
-                branch_info[0].id,
-            ),
+            key=lambda branch_info: (branch_info[1].name, branch_info[0].id),
         )
 
         if limit_results is not None:
diff --git a/lib/lp/codehosting/vfs/branchfs.py b/lib/lp/codehosting/vfs/branchfs.py
index 74461c9..3010e69 100644
--- a/lib/lp/codehosting/vfs/branchfs.py
+++ b/lib/lp/codehosting/vfs/branchfs.py
@@ -502,7 +502,7 @@ class AsyncLaunchpadTransport(AsyncVirtualTransport):
         @no_traceback_failures
         def real_mkdir(result):
             transport, path = result
-            return getattr(transport, "mkdir")(path, mode)
+            return transport.mkdir(path, mode)
 
         deferred.addCallback(real_mkdir)
         deferred.addErrback(maybe_make_branch_in_db)
diff --git a/lib/lp/codehosting/vfs/transport.py b/lib/lp/codehosting/vfs/transport.py
index bcdb9d3..b896c65 100644
--- a/lib/lp/codehosting/vfs/transport.py
+++ b/lib/lp/codehosting/vfs/transport.py
@@ -247,7 +247,7 @@ class AsyncVirtualTransport(Transport):
                         "cannot move between underlying transports"
                     )
                 )
-            return getattr(from_transport, "rename")(from_path, to_path)
+            return from_transport.rename(from_path, to_path)
 
         deferred.addCallback(check_transports_and_rename)
         return deferred
diff --git a/lib/lp/registry/tests/test_product.py b/lib/lp/registry/tests/test_product.py
index 4b22c33..1ef5135 100644
--- a/lib/lp/registry/tests/test_product.py
+++ b/lib/lp/registry/tests/test_product.py
@@ -1443,9 +1443,9 @@ class TestProduct(TestCaseWithFactory):
             )
         ordinary_user = self.factory.makePerson()
         with person_logged_in(ordinary_user):
-            setattr(product, "date_next_suggest_packaging", "foo")
+            product.date_next_suggest_packaging = "foo"
         with person_logged_in(product.owner):
-            setattr(product, "date_next_suggest_packaging", "foo")
+            product.date_next_suggest_packaging = "foo"
 
     def test_set_launchpad_AnyAllowedPerson_proprietary_product(self):
         # Only people with grants for a private product can set
@@ -1473,7 +1473,7 @@ class TestProduct(TestCaseWithFactory):
                 "foo",
             )
         with person_logged_in(owner):
-            setattr(product, "date_next_suggest_packaging", "foo")
+            product.date_next_suggest_packaging = "foo"
         # A user with a policy grant for the product can access attributes
         # of a private product.
         with person_logged_in(owner):
@@ -1484,7 +1484,7 @@ class TestProduct(TestCaseWithFactory):
                 {InformationType.PROPRIETARY: SharingPermission.ALL},
             )
         with person_logged_in(ordinary_user):
-            setattr(product, "date_next_suggest_packaging", "foo")
+            product.date_next_suggest_packaging = "foo"
 
     def test_userCanView_caches_known_users(self):
         # userCanView() maintains a cache of users known to have the
diff --git a/lib/lp/services/webapp/error.py b/lib/lp/services/webapp/error.py
index 4c451f9..b0f7f3f 100644
--- a/lib/lp/services/webapp/error.py
+++ b/lib/lp/services/webapp/error.py
@@ -67,7 +67,7 @@ class SystemErrorView(LaunchpadView):
         self.request.response.removeAllNotifications()
         if self.response_code is not None:
             self.request.response.setStatus(self.response_code)
-        if getattr(self.request, "oopsid") is not None:
+        if self.request.oopsid is not None:
             self.request.response.addHeader(
                 "X-Lazr-OopsId", self.request.oopsid
             )
diff --git a/lib/lp/services/webapp/publisher.py b/lib/lp/services/webapp/publisher.py
index 4702b67..bf64b0d 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.py
@@ -185,7 +185,7 @@ class redirection:
         redirections = getattr(fn, "__redirections__", None)
         if redirections is None:
             redirections = {}
-            setattr(fn, "__redirections__", redirections)
+            fn.__redirections__ = redirections
         redirections[self.name] = self.status
         return fn
 
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 7605ed7..71fac63 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -1552,8 +1552,7 @@ class LaunchpadObjectFactory(ObjectFactory):
 
             # Sort them
             related_series_branch_info = sorted_version_numbers(
-                series_branch_info,
-                key=lambda branch_info: (getattr(branch_info[1], "name")),
+                series_branch_info, key=lambda branch_info: branch_info[1].name
             )
 
             # Add a development branch at the start of the list.
@@ -1641,7 +1640,7 @@ class LaunchpadObjectFactory(ObjectFactory):
 
             related_package_branch_info = sorted_version_numbers(
                 related_package_branch_info,
-                key=lambda branch_info: (getattr(branch_info[1], "name")),
+                key=lambda branch_info: branch_info[1].name,
             )
 
         return (