-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(typing): Type pagerduty module #98075
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
Conversation
src/sentry/utils/forms.py
Outdated
field.widget.choices = choices | ||
|
||
|
||
def get_field_choices(field: forms.Field) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks anthony for the workaround 🙏
The Any
here isn't ideal but the alternative would be grabbing _ types from django-stubs (which I didn't see as an existing pattern?) and this is a workaround anyway.
service_id = _validate_int_field("service", cleaned_data) | ||
|
||
if service_id: | ||
if service_id and integration_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a logical change, but the current logic would error anyway if there wasn't an integration_id
given 🤷♀️
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## hackweek/typing-25 #98075 +/- ##
=====================================================
Coverage ? 80.59%
=====================================================
Files ? 8598
Lines ? 378518
Branches ? 24710
=====================================================
Hits ? 305078
Misses ? 73062
Partials ? 378 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just one question!
|
||
self.fields["account"].choices = integrations | ||
self.fields["account"].widget.choices = self.fields["account"].choices | ||
set_field_choices(self.fields["account"], integrations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the reason these changes were needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah they're needed because mypy doesn't see .choices
as an accessible attr on ChoiceField
after initialization see typeddjango/django-stubs#1858 & typeddjango/django-stubs#1514.
"account": dict(get_field_choices(self.fields["account"])).get(integration_id), | ||
"service": dict(get_field_choices(self.fields["service"])).get(service_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than introduce get_field_choices
(sorta abusing choices as a data smuggling mechanism) I would recommend doing it like I did here: 865de37
No description provided.