Skip to content

test(rules): Improve type coverage #91849

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −12
Open

test(rules): Improve type coverage #91849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions src/sentry/rules/actions/integrations/create_ticket/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import logging
from collections.abc import Callable, Sequence
from typing import Any, cast

from rest_framework.response import Response

Expand Down Expand Up @@ -109,18 +110,18 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:
organization = event.group.project.organization

for future in futures:
rule_id = future.rule.id
data = future.kwargs.get("data")
provider = future.kwargs.get("provider")
integration_id = future.kwargs.get("integration_id")
generate_footer = future.kwargs.get("generate_footer")
rule_id: int = future.rule.id
data = cast(dict[str, Any], future.kwargs.get("data"))
provider = cast(str, future.kwargs.get("provider"))
integration_id = cast(str, future.kwargs.get("integration_id"))
generate_footer = cast(Callable[[str], str], future.kwargs.get("generate_footer"))
Comment on lines +113 to +117
Copy link
Member

Choose a reason for hiding this comment

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

cast should almost never be used. this indicates you're not properly typing something else somewhere.

in the exceedingly rare case cast should be used to work around a bug in the type checker -- but even then # type: ignore is preferred since it doesn't have runtime overhead like cast

without digging into it this seems like RuleFuture should have a generic type and perhaps a TypedDict for whatever this kwargs is

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 was perhaps not ambitious enough here, just aiming to type the tupes so they weren't Any.
Hadn't thought to prefer type ignore over cast; I'd previously preferred cast since the cost isn't significant in most code and you don't end up with an Any, but I guess if I'm not confident enough to throw in an assert, Any might be more appropriate anyway.

The real issue, as you note, is that I don't know how to properly type RuleFuture. By design, it's a kwargs that corresponds to whatever is expected by whatever function has been wrapped up in it. For most, it's None, for a couple (this is one) cases it has a dict of various values, but conceptually it's designed to be extensible.
I imagine the proper approach, type-wise, if we want to serialize to a dict[str, Any] is to have some extraction and validation rather than assuming?

Copy link
Member

Choose a reason for hiding this comment

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

maybe

class RuleFuture[T](NamedTuple):
    rule: Rule
    kwargs: T

and then callers / users would use an appropriate TypedDict?


# If we invoked this handler from the notification action, we need to replace the rule_id with the legacy_rule_id, so we link notifications correctly
action_id = None
if features.has("organizations:workflow-engine-trigger-actions", organization):
# In the Notification Action, we store the rule_id in the action_id field
action_id = rule_id
rule_id = data.get("legacy_rule_id")
rule_id = cast(int, data.get("legacy_rule_id"))

integration = integration_service.get_integration(
integration_id=integration_id,
Expand Down
9 changes: 6 additions & 3 deletions src/sentry/rules/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

import abc
import logging
from collections import namedtuple
from collections.abc import Callable, MutableMapping, Sequence
from typing import TYPE_CHECKING, Any, ClassVar
from typing import TYPE_CHECKING, Any, ClassVar, NamedTuple

from django import forms

Expand Down Expand Up @@ -48,10 +47,14 @@
- [ACTION:I want to group events when] [RULE:an event matches [FORM]]
"""


# Encapsulates a reference to the callback, including arguments. The `key`
# attribute may be specifically used to key the callbacks when they are
# collated during rule processing.
CallbackFuture = namedtuple("CallbackFuture", ["callback", "kwargs", "key"])
class CallbackFuture(NamedTuple):
callback: Callable[[GroupEvent, Sequence[RuleFuture]], None]
kwargs: dict[str, Any]
key: str | None


class RuleBase(abc.ABC):
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/sentry_apps/tasks/sentry_apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]):

# If the future comes from a rule with a UI component form in the schema, append the issue alert payload
# TODO(ecosystem): We need to change this payload format after alerts create issues
id = f.rule.id
id: str | int = f.rule.id
Copy link
Member

Choose a reason for hiding this comment

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

str | int seems wrong here -- id should always be an int


# if we are using the new workflow engine, we need to use the legacy rule id
# Ignore test notifications
Expand Down
9 changes: 7 additions & 2 deletions src/sentry/types/rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
from collections import namedtuple
from dataclasses import dataclass
from typing import Any, NamedTuple

RuleFuture = namedtuple("RuleFuture", ["rule", "kwargs"])
from sentry.models.rule import Rule


class RuleFuture(NamedTuple):
rule: Rule
kwargs: dict[str, Any]


@dataclass
Expand Down
Loading