types(incidents): Stronglist sentry.incidents.metric_issue_detector#112638
types(incidents): Stronglist sentry.incidents.metric_issue_detector#112638
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| def validate_comparison(self, value: dict | float | int | str) -> float | dict: | ||
| def validate_comparison( | ||
| self, value: dict[str, Any] | float | int | str |
There was a problem hiding this comment.
should we have stronger types around the values here? Like have BaseJSONConfig or something export this as a named type; JSONPrimitive or something? (although, this wouldn't include arrays 🤔)
| conditions = serializers.ListField(required=True) | ||
|
|
||
| def validate_conditions(self, value): | ||
| def validate_conditions(self, value: list[dict[str, Any]]) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
similar thought about giving this dict[str, Any] a name; ValidatedCondition or something for the result?
There was a problem hiding this comment.
in these cases (ie validate_* methods) I'm not sure we can actually rely on the type being anything more specific than what the named field verifies, so we may be being too specific already. I didn't dig deep to verify though; the aim here is mostly to get type validation elsewhere where I'd been assuming it and surprised that clearly invalid stuff wasn't flagged, so the annotations to get into the stronglist were just trying to be basically correct and not missing.
3 participants
This not being strongly typed allowed a bug to sneak into another PR.
Doing this first and merging it forst allows us to prevent that bug in the future without complicating that other PR.