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

RoomAccessRules cleanup #62

Merged
merged 7 commits into from
Sep 10, 2020
Merged

RoomAccessRules cleanup #62

merged 7 commits into from
Sep 10, 2020

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Sep 8, 2020

Various cleanups of the DINUM-specific RoomAccessRules module, including:

  • Type hints
  • Docstring cleanups
  • Some code cleanups

I want to leave the asyncing until we do another round of merging Synapse master into dinsic, as ThirdPartyEventRules is not async yet on the dinsic branch.

Reviewable commit-by-commit, though the bulk of the cleanup is in the first and last commits.

@anoadragon453 anoadragon453 requested a review from a team September 9, 2020 15:21
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm modulo a couple of suggestions.

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved
ACCESS_RULE_DIRECT = "direct"


class AccessRules:
Copy link
Member

Choose a reason for hiding this comment

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

for future reference, https://docs.python.org/3/library/enum.html might be a useful thing for this sort of construction (saves maintaining a separate VALID_ACCESS_RULES)

Copy link
Member Author

Choose a reason for hiding this comment

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

@richvdh I actually spent about an hour trying to make this work with Enum and failed, eventually reverting back to VALID_ACCESS_RULES. Reason being that while one can say isinstance(rule, AccessRule), rule must be an instance of AccessRule. Since we're JSON serialising and deserialising stuff in the tests and the code here, that would've meant having to cast strings to AccessRule all over the place.

Thus VALID_ACCESS_RULES ended up being the less complex option :)

@anoadragon453 anoadragon453 requested review from richvdh and removed request for richvdh September 10, 2020 17:29
@anoadragon453 anoadragon453 merged commit 3d1c941 into dinsic Sep 10, 2020
@anoadragon453 anoadragon453 deleted the anoa/access_rules_cleanup branch September 10, 2020 18:05
anoadragon453 added a commit that referenced this pull request Oct 9, 2020
…oa/freeze_rooms

* 'dinsic' of github.com:matrix-org/synapse-dinsic:
  Only assert valid next_link params when provided (#65)
  Don't push if an user account has expired (#58)
  Swap method calls in RoomAccessTestCase.test_change_rules (#64)
  Make all rooms noisy by default (#60)
  Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change (#63)
  Override the power levels defaults, enforce mod requirement for invites, admin requirements for unknown state events (#61)
  RoomAccessRules cleanup (#62)
  Add a config option for validating 'next_link' parameters against a domain whitelist (#8275)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants