From 1385a827c2dbcb52dc8708d7cfc83a709151dc57 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Fri, 7 Oct 2022 21:10:17 -0700 Subject: [PATCH] python: Clean up getattr, setattr, delattr calls with literal names. These were useful as a transitional workaround to ignore type errors that only show up with django-stubs, while avoiding errors about unused type: ignore comments without django-stubs. Now that the django-stubs transition is complete, switch to type: ignore comments so that mypy will tell us if they become unnecessary. Many already have. Signed-off-by: Anders Kaseorg --- zerver/decorator.py | 9 ++--- zerver/lib/create_user.py | 4 +-- zerver/lib/import_realm.py | 2 +- zerver/lib/logging_util.py | 34 +++++++++---------- zerver/lib/request.py | 4 +-- zerver/lib/test_helpers.py | 2 +- zerver/lib/transfer.py | 6 ++-- zerver/lib/webhooks/common.py | 2 +- zerver/management/commands/makemessages.py | 5 ++- zerver/middleware.py | 4 +-- zerver/models.py | 10 +++--- zerver/tests/test_link_embed.py | 2 +- zerver/tests/test_logging_handlers.py | 2 +- zerver/tests/test_markdown.py | 2 +- zerver/tests/test_openapi.py | 4 +-- zerver/tests/test_signup.py | 11 +++--- .../commands/mark_all_messages_unread.py | 2 +- 17 files changed, 50 insertions(+), 55 deletions(-) diff --git a/zerver/decorator.py b/zerver/decorator.py index 06ff06b6fb..4a8238959a 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -368,7 +368,7 @@ def webhook_view( # Store the event types registered for this webhook as an attribute, which can be access # later conveniently in zerver.lib.test_classes.WebhookTestCase. - setattr(_wrapped_func_arguments, "_all_event_types", all_event_types) + _wrapped_func_arguments._all_event_types = all_event_types # type: ignore[attr-defined] # custom attribute return _wrapped_func_arguments return _wrapped_view_func @@ -793,7 +793,7 @@ def process_as_post( if not request.POST: # Only take action if POST is empty. if request.content_type == "multipart/form-data": - POST, _files = MultiPartParser( + POST, files = MultiPartParser( request.META, BytesIO(request.body), request.upload_handlers, @@ -809,10 +809,7 @@ def process_as_post( # See also: https://github.com/typeddjango/django-stubs/pull/925#issue-1206399444 POST._mutable = False request.POST = cast("_ImmutableQueryDict", POST) - # Note that request._files is just the private attribute that backs the - # FILES property, so we are essentially setting request.FILES here. (In - # Django 3.2 FILES was still a read-only property.) - setattr(request, "_files", _files) + request.FILES.update(files) elif request.content_type == "application/x-www-form-urlencoded": request.POST = QueryDict(request.body, encoding=request.encoding) diff --git a/zerver/lib/create_user.py b/zerver/lib/create_user.py index 9bb28e1acb..c7b841364e 100644 --- a/zerver/lib/create_user.py +++ b/zerver/lib/create_user.py @@ -43,8 +43,8 @@ def copy_default_settings( target_profile.save() return - setattr(target_profile, "full_name", settings_source.full_name) - setattr(target_profile, "timezone", canonicalize_timezone(settings_source.timezone)) + target_profile.full_name = settings_source.full_name + target_profile.timezone = canonicalize_timezone(settings_source.timezone) target_profile.save() if settings_source.avatar_source == UserProfile.AVATAR_FROM_USER: diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 95cc501cde..11d1f333be 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -892,7 +892,7 @@ def import_uploads( process_avatars(record) else: connection.close() - _cache = getattr(cache, "_cache") + _cache = cache._cache # type: ignore[attr-defined] # not in stubs assert isinstance(_cache, bmemcached.Client) _cache.disconnect_all() with ProcessPoolExecutor(max_workers=processes) as executor: diff --git a/zerver/lib/logging_util.py b/zerver/lib/logging_util.py index 030cf93720..1ee547890d 100644 --- a/zerver/lib/logging_util.py +++ b/zerver/lib/logging_util.py @@ -197,12 +197,10 @@ class ZulipFormatter(logging.Formatter): return " ".join(pieces) def format(self, record: logging.LogRecord) -> str: - if not getattr(record, "zulip_decorated", False): - # The `setattr` calls put this logic explicitly outside the bounds of the - # type system; otherwise mypy would complain LogRecord lacks these attributes. - setattr(record, "zulip_level_abbrev", abbrev_log_levelname(record.levelname)) - setattr(record, "zulip_origin", find_log_origin(record)) - setattr(record, "zulip_decorated", True) + if not hasattr(record, "zulip_decorated"): + record.zulip_level_abbrev = abbrev_log_levelname(record.levelname) + record.zulip_origin = find_log_origin(record) + record.zulip_decorated = True return super().format(record) @@ -227,12 +225,12 @@ class ZulipWebhookFormatter(ZulipFormatter): request = get_current_request() if not request: - setattr(record, "user", None) - setattr(record, "client", None) - setattr(record, "url", None) - setattr(record, "content_type", None) - setattr(record, "custom_headers", None) - setattr(record, "payload", None) + record.user = None + record.client = None + record.url = None + record.content_type = None + record.custom_headers = None + record.payload = None return super().format(record) if request.content_type == "application/json": @@ -257,12 +255,12 @@ class ZulipWebhookFormatter(ZulipFormatter): assert client is not None assert request.user.is_authenticated - setattr(record, "user", f"{request.user.delivery_email} ({request.user.realm.string_id})") - setattr(record, "client", client.name) - setattr(record, "url", request.META.get("PATH_INFO", None)) - setattr(record, "content_type", request.content_type) - setattr(record, "custom_headers", header_text or None) - setattr(record, "payload", payload) + record.user = f"{request.user.delivery_email} ({request.user.realm.string_id})" + record.client = client.name + record.url = request.META.get("PATH_INFO", None) + record.content_type = request.content_type + record.custom_headers = header_text or None + record.payload = payload return super().format(record) diff --git a/zerver/lib/request.py b/zerver/lib/request.py index 0da32bd694..ab1188c204 100644 --- a/zerver/lib/request.py +++ b/zerver/lib/request.py @@ -474,9 +474,9 @@ def get_current_request() -> Optional[HttpRequest]: def set_request(req: HttpRequest) -> None: - setattr(local, "request", req) + local.request = req def unset_request() -> None: if hasattr(local, "request"): - delattr(local, "request") + del local.request diff --git a/zerver/lib/test_helpers.py b/zerver/lib/test_helpers.py index 2a2490deb1..75a97bae56 100644 --- a/zerver/lib/test_helpers.py +++ b/zerver/lib/test_helpers.py @@ -394,7 +394,7 @@ def instrument_url(f: UrlFuncT) -> UrlFuncT: info = "" elif isinstance(info, dict): info = { - k: "" if hasattr(v, "read") and callable(getattr(v, "read")) else v + k: "" if hasattr(v, "read") and callable(v.read) else v for k, v in info.items() } diff --git a/zerver/lib/transfer.py b/zerver/lib/transfer.py index 73d67d4459..42d551dd73 100644 --- a/zerver/lib/transfer.py +++ b/zerver/lib/transfer.py @@ -41,7 +41,7 @@ def transfer_avatars_to_s3(processes: int) -> None: _transfer_avatar_to_s3(user) else: # nocoverage connection.close() - _cache = getattr(cache, "_cache") + _cache = cache._cache # type: ignore[attr-defined] # not in stubs assert isinstance(_cache, bmemcached.Client) _cache.disconnect_all() with ProcessPoolExecutor(max_workers=processes) as executor: @@ -76,7 +76,7 @@ def transfer_message_files_to_s3(processes: int) -> None: _transfer_message_files_to_s3(attachment) else: # nocoverage connection.close() - _cache = getattr(cache, "_cache") + _cache = cache._cache # type: ignore[attr-defined] # not in stubs assert isinstance(_cache, bmemcached.Client) _cache.disconnect_all() with ProcessPoolExecutor(max_workers=processes) as executor: @@ -111,7 +111,7 @@ def transfer_emoji_to_s3(processes: int) -> None: _transfer_emoji_to_s3(realm_emoji) else: # nocoverage connection.close() - _cache = getattr(cache, "_cache") + _cache = cache._cache # type: ignore[attr-defined] # not in stubs assert isinstance(_cache, bmemcached.Client) _cache.disconnect_all() with ProcessPoolExecutor(max_workers=processes) as executor: diff --git a/zerver/lib/webhooks/common.py b/zerver/lib/webhooks/common.py index 4e736c7a87..cb711606b4 100644 --- a/zerver/lib/webhooks/common.py +++ b/zerver/lib/webhooks/common.py @@ -192,7 +192,7 @@ def get_fixture_http_headers(integration_name: str, fixture_name: str) -> Dict[" # TODO: We may want to migrate to a more explicit registration # strategy for this behavior rather than a try/except import. view_module = importlib.import_module(view_module_name) - fixture_to_headers = getattr(view_module, "fixture_to_headers") + fixture_to_headers = view_module.fixture_to_headers except (ImportError, AttributeError): return {} return fixture_to_headers(fixture_name) diff --git a/zerver/management/commands/makemessages.py b/zerver/management/commands/makemessages.py index e3c8a6462f..01502ca865 100644 --- a/zerver/management/commands/makemessages.py +++ b/zerver/management/commands/makemessages.py @@ -228,9 +228,8 @@ class Command(makemessages.Command): process_all = self.frontend_all # After calling super().handle(), default_locale_path gets set on self - # so that we can reuse it here. We have to use getattr it to access it without - # getting an attribute error from mypy. - default_locale_path = getattr(self, "default_locale_path") + # so that we can reuse it here. + default_locale_path = self.default_locale_path # type: ignore[attr-defined] # not in stubs paths = glob.glob(f"{default_locale_path}/*") all_locales = [os.path.basename(path) for path in paths if os.path.isdir(path)] diff --git a/zerver/middleware.py b/zerver/middleware.py index ff4e0caaa1..9670f2068b 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -774,7 +774,7 @@ class ZulipSCIMAuthCheckMiddleware(SCIMAuthCheckMiddleware): # so we can assign the corresponding SCIMClient object to request.user - which # will allow this request to pass request.user.is_authenticated checks from now on, # to be served by the relevant views implemented in django-scim2. - # Since request.user must be a UserProfile or AnonymousUser, setattr is a type-unsafe + # Since request.user must be a UserProfile or AnonymousUser, this is a type-unsafe # workaround to make this monkey-patching work. - setattr(request, "user", scim_client) + request.user = scim_client # type: ignore[assignment] # wrong type return None diff --git a/zerver/models.py b/zerver/models.py index cb6c429d22..611a19c1e3 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -3245,22 +3245,20 @@ class AbstractUserMessage(models.Model): @staticmethod def where_unread() -> str: - return AbstractUserMessage.where_flag_is_absent(getattr(AbstractUserMessage.flags, "read")) + return AbstractUserMessage.where_flag_is_absent(AbstractUserMessage.flags.read) @staticmethod def where_read() -> str: - return AbstractUserMessage.where_flag_is_present(getattr(AbstractUserMessage.flags, "read")) + return AbstractUserMessage.where_flag_is_present(AbstractUserMessage.flags.read) @staticmethod def where_starred() -> str: - return AbstractUserMessage.where_flag_is_present( - getattr(AbstractUserMessage.flags, "starred") - ) + return AbstractUserMessage.where_flag_is_present(AbstractUserMessage.flags.starred) @staticmethod def where_active_push_notification() -> str: return AbstractUserMessage.where_flag_is_present( - getattr(AbstractUserMessage.flags, "active_mobile_push_notification") + AbstractUserMessage.flags.active_mobile_push_notification ) def flags_list(self) -> List[str]: diff --git a/zerver/tests/test_link_embed.py b/zerver/tests/test_link_embed.py index 0f34ee7eb3..daa00d3de4 100644 --- a/zerver/tests/test_link_embed.py +++ b/zerver/tests/test_link_embed.py @@ -542,7 +542,7 @@ class PreviewTestCase(ZulipTestCase): self.assertEqual(msg.rendered_content, with_preview) realm = msg.get_realm() - setattr(realm, "inline_url_embed_preview", False) + realm.inline_url_embed_preview = False realm.save() msg = self._send_message_with_test_org_url( diff --git a/zerver/tests/test_logging_handlers.py b/zerver/tests/test_logging_handlers.py index 4a9eb6fc75..c8fc6dba74 100644 --- a/zerver/tests/test_logging_handlers.py +++ b/zerver/tests/test_logging_handlers.py @@ -240,7 +240,7 @@ class AdminNotifyHandlerTest(ZulipTestCase): self.assertEqual(report["stack_trace"], "No stack trace available") # Test arbitrary exceptions from request.user - delattr(record.request, "user") + del record.request.user with patch("zerver.logging_handlers.traceback.print_exc"): report = self.run_handler(record) self.assertIn("host", report) diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index 5f43464c45..2b011303b7 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -623,7 +623,7 @@ class MarkdownTest(ZulipTestCase): self.assertEqual(converted.rendered_content, with_preview) realm = msg.get_realm() - setattr(realm, "inline_image_preview", False) + realm.inline_image_preview = False realm.save() sender_user_profile = self.example_user("othello") diff --git a/zerver/tests/test_openapi.py b/zerver/tests/test_openapi.py index 49818a6bb1..d063244529 100644 --- a/zerver/tests/test_openapi.py +++ b/zerver/tests/test_openapi.py @@ -422,8 +422,8 @@ do not match the types declared in the implementation of {function.__name__}.\n" # Iterate through the decorators to find the original # function, wrapped by has_request_variables, so we can parse # its arguments. - while hasattr(function, "__wrapped__"): - function = getattr(function, "__wrapped__") + while (wrapped := getattr(function, "__wrapped__", None)) is not None: + function = wrapped # Now, we do inference mapping each REQ parameter's # declaration details to the Python/mypy types for the diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index b46a371783..c7a6bfe819 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -12,7 +12,8 @@ from django.contrib.auth.views import PasswordResetConfirmView from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.core.mail.message import EmailMultiAlternatives -from django.http import HttpRequest, HttpResponse +from django.http import HttpRequest, HttpResponse, HttpResponseBase +from django.template.response import TemplateResponse from django.test import Client, override_settings from django.urls import reverse from django.utils import translation @@ -198,7 +199,7 @@ class DeactivationNoticeTestCase(ZulipTestCase): realm.save(update_fields=["deactivated"]) result = self.client_get("/login/", follow=True) - self.assertEqual(getattr(result, "redirect_chain")[-1], ("/accounts/deactivated/", 302)) + self.assertEqual(result.redirect_chain[-1], ("/accounts/deactivated/", 302)) self.assertIn("Zulip Dev, has been deactivated.", result.content.decode()) self.assertNotIn("It has moved to", result.content.decode()) @@ -1080,10 +1081,12 @@ class LoginTest(ZulipTestCase): description = "https://www.google.com/images/srpr/logo4w.png" realm.description = description realm.save(update_fields=["description"]) - response = self.client_get("/login/") + response: HttpResponseBase = self.client_get("/login/") expected_response = """

\ https://www.google.com/images/srpr/logo4w.png

""" - self.assertEqual(getattr(response, "context_data")["realm_description"], expected_response) + assert isinstance(response, TemplateResponse) + assert response.context_data is not None + self.assertEqual(response.context_data["realm_description"], expected_response) self.assertEqual(response.status_code, 200) diff --git a/zilencer/management/commands/mark_all_messages_unread.py b/zilencer/management/commands/mark_all_messages_unread.py index 2b5a5c5d9d..1aa0d5f1c8 100644 --- a/zilencer/management/commands/mark_all_messages_unread.py +++ b/zilencer/management/commands/mark_all_messages_unread.py @@ -15,6 +15,6 @@ class Command(BaseCommand): def handle(self, *args: Any, **options: Any) -> None: assert settings.DEVELOPMENT UserMessage.objects.all().update(flags=F("flags").bitand(~UserMessage.flags.read)) - _cache = getattr(cache, "_cache") + _cache = cache._cache # type: ignore[attr-defined] # not in stubs assert isinstance(_cache, bmemcached.Client) _cache.flush_all()