From c7b82d3ece1f28dc9c155e7da5438391c3f7384a Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Wed, 24 Jun 2020 14:38:35 +0000 Subject: [PATCH] mypy: Use tuples for muted_topics. We now have our muted topics use tuples internally, which allows us to tighten up the annotation for get_topic_mutes, as well as our schema checking. We want to deprecate sub_validator=None for check_list, so we also introduce check_tuple here. Now we also want to deprecate check_tuple, but it's at least isolated now. We will use this for data structures that are tuples, but which are sent as lists over the wire. Fortunately, we don't have too many of those. The plan is to convert tuples to dictionaries, but backward compatibility may be tricky in some places. --- zerver/lib/topic_mutes.py | 6 +++--- zerver/lib/validator.py | 18 ++++++++++++++++++ zerver/tests/test_decorators.py | 16 ++++++++++++++++ zerver/tests/test_events.py | 7 ++++++- zerver/tests/test_muting.py | 6 +++--- 5 files changed, 46 insertions(+), 7 deletions(-) diff --git a/zerver/lib/topic_mutes.py b/zerver/lib/topic_mutes.py index f1a30df323..7898c5cdb6 100644 --- a/zerver/lib/topic_mutes.py +++ b/zerver/lib/topic_mutes.py @@ -1,5 +1,5 @@ import datetime -from typing import Any, Callable, Dict, List, Optional, Union +from typing import Any, Callable, Dict, List, Optional, Tuple from django.utils.timezone import now as timezone_now from sqlalchemy.sql import Selectable, and_, column, not_, or_ @@ -9,7 +9,7 @@ from zerver.lib.topic import topic_match_sa from zerver.models import MutedTopic, UserProfile, get_stream -def get_topic_mutes(user_profile: UserProfile) -> List[List[Union[str, float]]]: +def get_topic_mutes(user_profile: UserProfile) -> List[Tuple[str, str, float]]: rows = MutedTopic.objects.filter( user_profile=user_profile, ).values( @@ -18,7 +18,7 @@ def get_topic_mutes(user_profile: UserProfile) -> List[List[Union[str, float]]]: 'date_muted', ) return [ - [row['stream__name'], row['topic_name'], datetime_to_timestamp(row['date_muted'])] + (row['stream__name'], row['topic_name'], datetime_to_timestamp(row['date_muted'])) for row in rows ] diff --git a/zerver/lib/validator.py b/zerver/lib/validator.py index 5595f967dc..f887e69404 100644 --- a/zerver/lib/validator.py +++ b/zerver/lib/validator.py @@ -163,6 +163,24 @@ def check_list(sub_validator: Optional[Validator[ResultT]]=None, length: Optiona return cast(List[ResultT], val) return f +def check_tuple(sub_validators: List[Validator[ResultT]]) -> Validator[Tuple[Any, ...]]: + def f(var_name: str, val: object) -> Tuple[Any, ...]: + if not isinstance(val, tuple): + raise ValidationError(_('{var_name} is not a tuple').format(var_name=var_name)) + + desired_len = len(sub_validators) + if desired_len != len(val): + raise ValidationError(_('{var_name} should have exactly {desired_len} items').format( + var_name=var_name, desired_len=desired_len, + )) + + for i, sub_validator in enumerate(sub_validators): + vname = f'{var_name}[{i}]' + sub_validator(vname, val[i]) + + return val + return f + # https://zulip.readthedocs.io/en/latest/testing/mypy.html#using-overload-to-accurately-describe-variations @overload def check_dict(required_keys: Iterable[Tuple[str, Validator[object]]]=[], diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index adda952e16..0302459ac3 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -68,6 +68,7 @@ from zerver.lib.validator import ( check_string_in, check_string_or_int, check_string_or_int_list, + check_tuple, check_union, check_url, equals, @@ -848,6 +849,21 @@ class ValidatorTestCase(TestCase): with self.assertRaisesRegex(ValidationError, r'color is not a string'): check_color('color', z) + def test_check_tuple(self) -> None: + x: Any = 999 + with self.assertRaisesRegex(ValidationError, r'x is not a tuple'): + check_tuple([check_string])('x', x) + + x = (5, 2) + with self.assertRaisesRegex(ValidationError, r'x\[0\] is not a string'): + check_tuple([check_string, check_string])('x', x) + + x = (1, 2, 3) + with self.assertRaisesRegex(ValidationError, r'x should have exactly 2 items'): + check_tuple([check_int, check_int])('x', x) + + check_tuple([check_string, check_int])('x', ('string', 42)) + def test_check_list(self) -> None: x: Any = 999 with self.assertRaisesRegex(ValidationError, r'x is not a list'): diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 0cb00e825a..b0ae35709b 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -139,6 +139,7 @@ from zerver.lib.validator import ( check_list, check_none_or, check_string, + check_tuple, check_url, equals, ) @@ -1493,7 +1494,11 @@ class EventsRegisterTest(ZulipTestCase): def test_muted_topics_events(self) -> None: muted_topics_checker = self.check_events_dict([ ('type', equals('muted_topics')), - ('muted_topics', check_list(check_list(sub_validator=None, length=3))), + ('muted_topics', check_list(check_tuple([ + check_string, # stream name + check_string, # topic name + check_int, # timestamp + ]))), ]) stream = get_stream('Denmark', self.user_profile.realm) recipient = stream.recipient diff --git a/zerver/tests/test_muting.py b/zerver/tests/test_muting.py index 91d72ff386..818f40b788 100644 --- a/zerver/tests/test_muting.py +++ b/zerver/tests/test_muting.py @@ -73,7 +73,7 @@ class MutedTopicsTests(ZulipTestCase): result = self.api_patch(user, url, data) self.assert_json_success(result) - self.assertIn([stream.name, 'Verona3', mock_date_muted], get_topic_mutes(user)) + self.assertIn((stream.name, 'Verona3', mock_date_muted), get_topic_mutes(user)) self.assertTrue(topic_is_muted(user, stream.id, 'Verona3')) self.assertTrue(topic_is_muted(user, stream.id, 'verona3')) @@ -106,12 +106,12 @@ class MutedTopicsTests(ZulipTestCase): topic_name='Verona3', date_muted=datetime(2020, 1, 1, tzinfo=timezone.utc), ) - self.assertIn([stream.name, 'Verona3', mock_date_muted], get_topic_mutes(user)) + self.assertIn((stream.name, 'Verona3', mock_date_muted), get_topic_mutes(user)) result = self.api_patch(user, url, data) self.assert_json_success(result) - self.assertNotIn([stream.name, 'Verona3', mock_date_muted], get_topic_mutes(user)) + self.assertNotIn((stream.name, 'Verona3', mock_date_muted), get_topic_mutes(user)) self.assertFalse(topic_is_muted(user, stream.id, 'verona3')) def test_muted_topic_add_invalid(self) -> None: