chore(slack): correctly type issue message builder #74876
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/sentry/integrations/slack/message_builder/issues.py
Did you find this useful? React with a 👍 or 👎 |
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #74876 +/- ##
===========================================
+ Coverage 57.04% 78.13% +21.08%
===========================================
Files 6732 6747 +15
Lines 300554 301140 +586
Branches 51704 51791 +87
===========================================
+ Hits 171453 235294 +63841
+ Misses 124314 59484 -64830
- Partials 4787 6362 +1575
|
| @@ -584,7 +545,8 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock: | |||
| # If an event is unspecified, use the tags of the latest event (if one exists). | |||
| event_for_tags = self.event or self.group.get_latest_event() | |||
|
|
|||
| obj = self.event if self.event is not None else self.group | |||
| event_or_group: Group | GroupEvent = self.event if self.event is not None else self.group | |||
There was a problem hiding this comment.
I don't think this should need an annotation? mypy should be able to infer this union I think
There was a problem hiding this comment.
it gets angry if i take it away
There was a problem hiding this comment.
what's the error? -- does it narrow it to object (probably)? :(
| if actor.is_team and isinstance(assigned_actor, Team): | ||
| assignee_text = f"#{assigned_actor.slug}" | ||
| elif actor.is_user: | ||
| elif actor.is_user and isinstance(assigned_actor, RpcUser): |
There was a problem hiding this comment.
could probably just use the isinstance checks here since they should do the same thing
|
|
||
| raise NotImplementedError |
There was a problem hiding this comment.
I think I would probably change the ifs to elifs and else: raise AssertionError('unreachable')
consolidating pure functions into a class is a bad idea -- they are pure functions and there is no reason to bring unnecessary object-orientation into this. classes make it harder to reason about, test, type, and work with. most of the mess we have today is due to excessive OO / inheritance / cuteness with classes and we'd be much better off without them |
|
PR reverted: 0b20403 |
This reverts commit 1d9d37b. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
This reverts commit 1d9d37b. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
Fixing typing for the issues Slack message builder.