feat(typing): add possibly-undefined-check to issues team / error ingestion critical code #68283
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| # begin: possibly-undefined checks for issue team critical code | ||
| [[tool.mypy.overrides]] | ||
| module = [ | ||
| "sentry.issues.*", | ||
| "sentry.tasks.post_process", | ||
| "sentry.event_manager", | ||
| "sentry.grouping.*", | ||
| "sentry.tasks.store", | ||
| "sentry.ingest.consumer.processors" | ||
| ] | ||
| enable_error_code = ["possibly-undefined"] | ||
| # end: possibly-undefined |
There was a problem hiding this comment.
if we're going to do this (I still don't think this is the right way to land this change and doesn't prevent this issue from happening in general) let's just add this to the stronger list above
There was a problem hiding this comment.
#65589 (comment) -- on the previous PR, you asked me to make changes, i made the changes, and you still did not approve it, and when i made follow up comments, you did not address them or respond to them.
Here, can you please inform me that if i make these changes, you will approve the PR?
There was a problem hiding this comment.
Yes, let's do that.
It may also be easier to reduce the number of modules you want to adjust.
There was a problem hiding this comment.
We will be unable to cover all of our code here without a lot of effort -- 267 errors would need to be fixed.
Unless we get dedicated time to try and address all of this, we will have to add a much smaller subset of these files, if any of them at the moment.
Why are we so against preventing incidents with mypy checks?
There was a problem hiding this comment.
Was possibly undefined a case missed which caused an incident?
There was a problem hiding this comment.
I actually think it would be less work to enable this check for the whole codebase, than to try and add all of the issues / error ingestion code to the strict typing check set.
| return | ||
|
|
||
| publisher.publish( | ||
| "ingest-replay-events", |
There was a problem hiding this comment.
slightly more correct / less error prone would be to use try: else:
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
We continually face incidents/issues where this type check configuration would have caught the error.
I attempted to add this for all of sentry in #65589 but this was deemed unsatisfactory because there was a push to try and fix all of the errors in sentry before merging, a large task which i did not have time for.
Here, instead, we add an allowlist, which right now contains critical issues team code, which we want to have a higher safety bar for.
This PR will prevent incidents, and make our ingestion safer.
I fix one instance of this error in post_process.