Skip to content
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.

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

feat(typing): Make monitor_config a TypedDict #2931

Merged
merged 15 commits into from
Apr 10, 2024

Conversation

sentrivana
Copy link
Contributor

Better type hinting for monitor configs.

Closes #2930


General Notes

Thank you for contributing to sentry-python!

Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.

For maintainers

Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.

Before running sensitive test suites, please carefully check the PR. Then, apply the Trigger: tests using secrets label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.

@@ -427,17 +440,17 @@ def _get_humanized_interval(seconds):
for unit, divider in TIME_UNITS:
if seconds >= divider:
interval = int(seconds / divider)
return (interval, unit)
return (interval, cast("MonitorConfigScheduleUnit", unit))
Copy link
Contributor Author

@sentrivana sentrivana Apr 4, 2024

Choose a reason for hiding this comment

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

mypy is not smart enough to realize that unit comes from TIME_UNITS which is an immutable tuple of tuples where the first element is a subset of MonitorConfigScheduleUnit. Trying to add a type comment to line 437 to tell mypy the precise type of TIME_UNITS doesn't work either. So we cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szokeasaurusrex Do you think this cast is okay like this (aliased to lambda _, x: x at runtime) or should the cast itself be hidden behind TYPE_CHECKING here?

Copy link
Member

Choose a reason for hiding this comment

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

cast probably needs to be behind TYPE_CHECKING to maintain compatibility with Python 2.x.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, looks like you handled the compatibility concern by importing as lambda _, x: x when the import fails. Although, worth noting, that I think this lambda only gets activated on Python 2.x. In runtime on Python 3.5+, we still call cast from typing, but that is fine because it is a no-op that returns the second argument at runtime.

"day",
"hour",
"minute",
"second", # not supported in Sentry and will result in a warning
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second-level precision can still be provided by the user so this needs to be part of the type

@sentrivana sentrivana marked this pull request as ready for review April 4, 2024 09:50
@sentrivana sentrivana changed the title ref(typing): Make monitor_config a TypedDict feat(typing): Make monitor_config a TypedDict Apr 4, 2024
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

nice!

@sentrivana sentrivana enabled auto-merge (squash) April 10, 2024 11:24
@sentrivana sentrivana merged commit a584653 into master Apr 10, 2024
123 of 124 checks passed
@sentrivana sentrivana deleted the ivana/monitor-config-typeddict branch April 10, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitor config TypedDict
3 participants