ref(notifications): Simplify MessageBuilder hierarchy #29721
Conversation
| "style": action.style or "default", | ||
| "type": "button", | ||
| } | ||
| for action in actions |
There was a problem hiding this comment.
@mgaeta IMO, if we aren't going to have a class method as_slack, we should still make a method that replicates the functionality rather than embedding the mapping logic in SlackMessageBuilder
mgaeta
force-pushed
the
feat/api-2202-slack-digest-5
branch
from
8c1ed1f to
1448112
Compare
| } | ||
| ) | ||
| return buttons | ||
| def get_message_builder(klass: str) -> type[SlackNotificationsMessageBuilder]: |
There was a problem hiding this comment.
What is klass? I don't understand the variable name.
There was a problem hiding this comment.
In python, class is a reserved word. By convention when we have a variable that we'd want to call "class" we use "klass".
In /src/sentry/notifications/notifications/activity/note.py we reference the class we want to use by string:
class NoteActivityNotification(GroupActivityNotification):
message_builder = "SlackNotificationsMessageBuilder"
...This is because it's impossible to import the sentry.integrations.slack module in this file without causing a circular dependency. Once we're in the Slack module we can get the class we want:
def get_message_builder(klass: str) -> type[SlackNotificationsMessageBuilder]:
return {
"DigestNotificationMessageBuilder": DigestNotificationMessageBuilder,
"IssueNotificationMessageBuilder": IssueNotificationMessageBuilder,
"SlackNotificationsMessageBuilder": SlackNotificationsMessageBuilder,
}[klass]There was a problem hiding this comment.
Interesting, I knew class was reserved but hadn't seen klass.
There was a problem hiding this comment.
I think the convention in python is usually cls actually, klass is a ruby-ism
See https://www.python.org/dev/peps/pep-0008/#function-and-method-arguments
mgaeta
force-pushed
the
feat/api-2202-slack-digest-5
branch
from
1448112 to
a569203
Compare
| self, | ||
| ) -> Mapping[ExternalProviders, Mapping[User, int]]: | ||
| users_by_provider = NotificationSetting.objects.get_notification_recipients(self.project) | ||
| ) -> Mapping[ExternalProviders, Mapping[Team | User, int]]: |
There was a problem hiding this comment.
I don't think this hurts anything, but I also don't think a team would ever be notified about processing issues.
There was a problem hiding this comment.
I just needed the method signature to match the parent class to satisfy the type checker.
|
|
||
|
|
||
| class SlackOrganizationRequestMessageBuilder(SlackNotificationsMessageBuilder): | ||
| def __init__( |
There was a problem hiding this comment.
Hmm this is gonna need a rebase when Steve's PR is merged since he's adding to this file
This reverts commit c2e2c23. Co-authored-by: scefali via Slack <scefali@sentry.io>
This refactor adds unimplemented methods to the
BaseNotificationclass so that we can use polymorphism instead of a long series of conditionals.