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

types(python): Slack Integration Types #28236

Merged
merged 4 commits into from
Sep 1, 2021
Merged

Conversation

mgaeta
Copy link
Contributor

@mgaeta mgaeta commented Aug 27, 2021

Adding types to the src/sentry/integrations/slack/ directory. Some tricky things:

  • mypy can't deal with __path__
  • IntegrationMetadata is icky because it extends itself because of classes/namedtuples
  • the parse_link() function should be refactored in a separate PR

@@ -149,7 +151,7 @@ def map_discover_query_args(url: str, args: Mapping[str, str]) -> Mapping[str, A
return dict(**args, query=query)


handler = Handler(
handler: Handler = Handler(
Copy link
Member

@evanpurkhiser evanpurkhiser Aug 27, 2021

Choose a reason for hiding this comment

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

I hate how explicit mypy forces you to be. Like.. can it really not figure out for itself that this variable is a Handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's weird, when I take out the explicit typing in the three handlers I only get two errors:

src/sentry/integrations/slack/unfurl/__init__.py:46: error: Cannot determine type of 'handler'
src/sentry/integrations/slack/unfurl/__init__.py:48: error: Cannot determine type of 'handler'

Copy link
Member

Choose a reason for hiding this comment

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

Nah it's just because mypy isn't very good 😢

@@ -116,8 +119,8 @@ def get_pipeline_views(self):

return [identity_pipeline_view]

def get_team_info(self, access_token):
headers = {"Authorization": "Bearer %s" % access_token}
def get_team_info(self, access_token: str) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

Could use JSONData here too

workspace = forms.ChoiceField(choices=(), widget=forms.Select())
channel = forms.CharField(widget=forms.TextInput())
channel_id = forms.HiddenInput()
tags = forms.CharField(required=False, widget=forms.TextInput())

def __init__(self, *args, **kwargs):
def __init__(self, *args: Any, **kwargs: Any) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Should we type args/kwargs as Seq/Mapping?

value = self._format_value(status, rule_id, error_message)
self.client.set(self._get_redis_key(), f"{value}", ex=60 * 60)

def get_value(self):
def get_value(self) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

We could use sentry.utils.json.JSONData. It's just Any for now, but if we want to change how we define json in the future this will make it easier.

@mgaeta mgaeta force-pushed the feat/api-1710-uninstall-slack-2 branch from 9ea7560 to f5e0cfe Compare September 1, 2021 20:12
@mgaeta mgaeta merged commit ee9f8c4 into master Sep 1, 2021
@mgaeta mgaeta deleted the feat/api-1710-uninstall-slack-2 branch September 1, 2021 20:34
jan-auer added a commit that referenced this pull request Sep 2, 2021
* master: (109 commits)
  ref(js): Add notice to use useApi when applicable (#28365)
  fix(tests): Temporarily disable test for migration 0223 (#28363)
  fix(ui): Use theme border for form footer border (#28359)
  ref(ts): Authenticators have names (#28362)
  feat(cdc_search): Implement cdc search for `is:unresolved` query. (#28006)
  ref(rrweb): Upgrade to latest player (#28333)
  feat(issue-alerts): Add Sentry Apps Alert Rule Action as an Action (#28338)
  feat(notifications): Remove NotificationSettings when Slack is uninstalled (#28216)
  py38(django 2.2): bump django-crispy-forms to 1.8.1 (#28344)
  types(python): Slack Integration Types (#28236)
  ref(jira ac): Rm fk constraints from JiraTenant (#28349)
  chore(js): Upgrade storybook from 6.3.6 -> 6.3.7 (#28323)
  feat(symbol-sources): Redact symbol source secrets from audit logs (#28334)
  feat(notifications): Add `actor_id` to Analytics Events (#28347)
  ref(performance): Separate header from content in transaction events (#28346)
  Adding in new text styles for the codeowners sync CTA in the issues detail. (#28133)
  fix(django 2.2): rename view method to avoid clashing (#28312)
  fix(alerts): Project breadcrumb link to project rule list (#28339)
  ref(performance): Separate header from content in transaction vitals (#28343)
  feat(dev_env): Dev env bootstrap support for two Python versions (#28213)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants