ref(incidents): Add type hints to sentry/incidents/logic.py

[RyanSkonnord]

Add type hints to the sentry.incidents.logic module and some related functions.

Remove unused function parameters. Change default collection args from None to () where they are treated interchangeably, to minimize nullable arg types. Add explicit null checks where required by the type checker.

Add underscore prefix to functions used privately. Move functions that were used only by a single module.

[codecov]

Codecov Report

Attention: Patch coverage is 93.37748% with 10 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/incidents/logic.py	95.31%	3 Missing and 3 partials ⚠️
src/sentry/incidents/models/alert_rule.py	66.66%	1 Missing and 1 partial ⚠️
src/sentry/integrations/pagerduty/utils.py	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #75541      +/-   ##
==========================================
- Coverage   78.13%   78.13%   -0.01%     
==========================================
  Files        6931     6931              
  Lines      307646   307695      +49     
  Branches    50358    50364       +6     
==========================================
+ Hits       240386   240424      +38     
- Misses      60864    60870       +6     
- Partials     6396     6401       +5     

[cathteng]

the src/sentry/incidents/logic.py file is huge :/

[cathteng]

is Collection looser than Sequence?

[RyanSkonnord]

Yes. Sequence implies that it has a consistent iteration order and can be indexed with brackets, usually meaning a list or tuple. Collection includes set and frozenset.

[RyanSkonnord]

I looked into it, and apparently nulling out the primary key is the normal and recommended way to copy a model in this way (official docs; Stack Overflow).

I think the reason Mypy doesn't like it is because of our Model class:

sentry/src/sentry/db/models/base.py

Lines 315 to 316 in 0a4fdfd

	class Model(BaseModel):
	id: models.Field[int, int] = BoundedBigAutoField(primary_key=True)

Changing it to

    id: models.Field[int | None, int] = BoundedBigAutoField(primary_key=True)

seems to fix it. I couldn't find any docs that explain the difference between the two type params to Field; I found the above through trial and error. But I'm guessing/hoping it satisfies the meaning of "it can be nullable during a model's lifecycle as a Python object, but is always a non-null int when persisted". Should we go ahead with such a change to the Model class? (If so, I'd lean toward doing it as a follow-up PR and reverting this then.)

[asottile-sentry]

it's actually an intentional django-stubs thing -- you almost never want id = None (and making it nullable causes all sorts of other problems)

[asottile-sentry]

this patch has gotten pretty large -- can we split out parts of those and land them separately to minimize risk and increase reviewability?

[RyanSkonnord]

this patch has gotten pretty large -- can we split out parts of those and land them separately to minimize risk and increase reviewability?

It got large because that's what was necessary to remove "sentry.incidents.logic" from pyproject.toml. You want me to put it back in?

[asottile-sentry]

this patch has gotten pretty large -- can we split out parts of those and land them separately to minimize risk and increase reviewability?

It got large because that's what was necessary to remove "sentry.incidents.logic" from pyproject.toml. You want me to put it back in?

no. there are logical parts of this patch that can be landed separately which will reduce risk and improve reviewability

[RyanSkonnord]

@asottile-sentry Functional changes broken out into:

1. fix(incidents): Fix time window check in update_alert_rule #77197
2. ref(incidents): Inline update_comment and delete_comment #77199
3. ref(incidents): Move set_incident_seen to private function #77200
4. ref(incidents): Refactor get_target_identifier_display_* sigs #77201

[asottile-sentry]

@asottile-sentry Functional changes broken out into:

1. fix(incidents): Fix time window check in update_alert_rule #77197
2. ref(incidents): Inline update_comment and delete_comment #77199
3. ref(incidents): Move set_incident_seen to private function #77200
4. ref(incidents): Refactor get_target_identifier_display_* sigs #77201

thank you! those are so much easier to review

[RyanSkonnord]

@asottile-sentry Now the PR should consist only of type annotation fixes, with the only changes to flow being a few helper functions motivated by type-checking.

[asottile-sentry]

much easier to review now -- thanks!