Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix some comments and types in service notices #7996

Merged
merged 5 commits into from
Jul 31, 2020
Merged

Fix some comments and types in service notices #7996

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd1fbb2
Stop references Deferreds in comments.
clokep Jul 30, 2020
4a3f15d
Add to tox and fix a few more issues.
clokep Jul 30, 2020
a9d97d8
Add changelog.
clokep Jul 30, 2020
699eb37
Add more typing info.
clokep Jul 31, 2020
052a474
Ignore mypy on a line.
clokep Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions synapse/server_notices/resource_limits_server_notices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import logging
from typing import List, Tuple
from typing import List, Optional, Tuple

from synapse.api.constants import (
EventTypes,
Expand Down Expand Up @@ -135,7 +135,7 @@ async def _remove_limit_block_notification(
)

async def _apply_limit_block_notification(
self, user_id: str, event_body: str, event_limit_type: str
self, user_id: str, event_body: str, event_limit_type: Optional[str]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I see much evidence of event_limit_type ever being set to None by the calling function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synapse/synapse/server_notices/resource_limits_server_notices.py

Lines 88 to 97 in 4a3f15d

limit_msg = None
limit_type = None
try:
# Normally should always pass in user_id to check_auth_blocking
# if you have it, but in this case are checking what would happen
# to other users if they were to arrive.
await self._auth.check_auth_blocking()
except ResourceLimitError as e:
limit_msg = e.msg
limit_type = e.limit_type

limit_type only gets set on error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I should also note that mypy found this.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like in the linked code block that _apply_limit_block_notification will only be called if limit_msg is not None, and as limit_msg and limit_type are both set together, _apply_limit_block_notification should only be called when limit_type is not None?

I suppose that may just be circumstantial of this one calling function though, and _apply_limit_block_notification can handle limit_type=None just fine, and thus that's what the signature should be. I found it a bit difficult to verify that last bit though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you're saying. Let me see if I can convince mypy here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so mypy doesn't seem to recognize that these are both set or neither is set. I switched it around a bit in 052a474, but ended up ignoring that function call. I believe the correct signature is str, not Optional[str], so I'd rather the signature be correct.

Let me know if you agree!

) -> None:
"""Utility method to apply limit block notifications in the server
notices room.
Expand Down Expand Up @@ -206,7 +206,7 @@ async def _is_room_currently_blocked(self, room_id: str) -> Tuple[bool, List[str
# The user has yet to join the server notices room
pass

referenced_events = []
referenced_events = [] # type: List[str]
if pinned_state_event is not None:
referenced_events = list(pinned_state_event.content.get("pinned", []))

Expand Down
4 changes: 3 additions & 1 deletion synapse/server_notices/server_notices_sender.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Iterable, Union

from synapse.server_notices.consent_server_notices import ConsentServerNotices
from synapse.server_notices.resource_limits_server_notices import (
ResourceLimitsServerNotices,
Expand All @@ -32,7 +34,7 @@ def __init__(self, hs):
self._server_notices = (
ConsentServerNotices(hs),
ResourceLimitsServerNotices(hs),
)
) # type: Iterable[Union[ConsentServerNotices, ResourceLimitsServerNotices]]

async def on_user_syncing(self, user_id: str) -> None:
"""Called when the user performs a sync operation.
Expand Down
1 change: 1 addition & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ commands = mypy \
synapse/push/push_rule_evaluator.py \
synapse/replication \
synapse/rest \
synapse/server_notices \
synapse/spam_checker_api \
synapse/storage/data_stores/main/ui_auth.py \
synapse/storage/database.py \
Expand Down