Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to disable commands dynamically #1

Closed
wants to merge 3 commits into from

Conversation

tadzik
Copy link

@tadzik tadzik commented Oct 6, 2021

This allows for hiding them completely depending on e.g. a config setting, or arbitrary sender rules (up to the implementing bridge).

Also accidentally upstreamed already to mautrix#56 – no harm done (though I planned for it to pass here first), but we probably want this for ourselves regardless.

This allows for hiding them completely depending on e.g.
a config setting, or arbitrary sender rules
(up to the implementing bridge).
Copy link

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

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

Were is the configurable part?

I don't see how we could make this return False? (implying one cannot defined a lambda inside a config file.


_help_text: str
_help_args: str
help_section: HelpSection

def __init__(self, handler: CommandHandlerFunc, management_only: bool, name: str,
help_text: str, help_args: str, help_section: HelpSection,
needs_auth: bool, needs_admin: bool, **kwargs) -> None:
needs_auth: bool, needs_admin: bool,
is_enabled_for: IsEnabledForFunc = lambda _: True,
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be True or False? (implying this is the return type declaration of that lamda)

Suggested change
is_enabled_for: IsEnabledForFunc = lambda _: True,
is_enabled_for: IsEnabledForFunc = lambda _: bool,

Copy link
Author

@tadzik tadzik Oct 6, 2021

Choose a reason for hiding this comment

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

That's the default value, not the type signature. This makes the parameter optional, and backwards compatible.

The (return) type declaration is covered by the IsEnabledForFunc ahead, after = we get the concrete implementation of that.

@tadzik
Copy link
Author

tadzik commented Oct 6, 2021

Were is the configurable part?

I don't see how we could make this return False? (implying one cannot defined a lambda inside a config file.

That is up to a bridge implementation. An example usecase is here: element-hq/mautrix-signal#1

@@ -392,6 +397,8 @@ async def handle(self, room_id: RoomID, event_id: EventID, sender: BaseUser,
command = command.lower()
try:
handler = command_handlers[command]
if not handler.is_enabled_for(evt):
raise KeyError()
except KeyError:
try:
handler = command_aliases[command]

Choose a reason for hiding this comment

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

Do we need another check here in case the disabled command has an alias?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. Fixed it in 697e4e9, I think it made the code tidier as well.

@tadzik
Copy link
Author

tadzik commented Oct 12, 2021

This has been merged upstream now (including the suggestions from y'all) so we just need to pull that instead and save ourselves future merge conflicts.

@tadzik tadzik closed this Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants