Ref #51 -- Resolve sentry_monitor_config type ambigutity#56
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
- Coverage 98.85% 98.82% -0.03%
==========================================
Files 6 6
Lines 174 170 -4
==========================================
- Hits 172 168 -4
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR (Ref #51) updates how sentry_monitor_config is represented and forwarded in the cron() decorator, aiming to eliminate ambiguity between “unset/default”, “custom config”, and “disabled” Sentry monitoring.
Changes:
- Introduces an
UNSETsentinel to distinguish “argument not provided” from “provided but falsy”. - Adjusts
cron()and Sentry integration typing/behavior aroundsentry_monitor_config. - Updates docs and tests to reflect the new “falsy disables” semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crontask/__init__.py |
Adds UNSET sentinel and changes cron()’s sentry_monitor_config typing and forwarding behavior. |
crontask/contrib/sentry.py |
Changes monitor_cron_task() signature for sentry_monitor_config. |
README.md |
Updates usage guidance/examples for disabling Sentry monitoring. |
tests/test_tasks.py |
Relaxes assertion around default forwarded sentry_monitor_config. |
Comments suppressed due to low confidence (1)
crontask/contrib/sentry.py:84
monitor_cron_task()now requiressentry_monitor_configto be a dict, but the function still treats it as optional viasentry_monitor_config or trigger_to_monitor_config(trigger), and existing tests call it withNone. This should either keep the| Nonein the signature (and document thatNonetriggers auto-detection) or update callers/tests and the internal logic to removeNonesupport consistently.
def monitor_cron_task(
task: Task,
trigger: BaseTrigger,
sentry_monitor_config: dict[str, dict[str, str | int] | str],
) -> Task:
"""
Wrap the task function in a Sentry monitor for a suitable trigger.
You don't need to create the monitor in advance since
the Sentry monitor configuration is upserted on each task execution.
If the trigger is not supported, the task is returned unchanged.
"""
try:
from sentry_sdk import monitor
except ImportError:
return task
else:
if monitor_config := sentry_monitor_config or trigger_to_monitor_config(
trigger
):
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.