-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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(integrations): Unify get_repositories signature #83921
Conversation
@@ -381,6 +381,7 @@ module = [ | |||
"sentry.integrations.services.*", | |||
"sentry.integrations.slack.threads.*", | |||
"sentry.integrations.slack.views.*", | |||
"sentry.integrations.source_code_management.repository", |
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 the list of modules with stronger typing.
@@ -3,7 +3,7 @@ | |||
import logging | |||
from collections.abc import Mapping, Sequence | |||
from datetime import datetime | |||
from typing import Any, cast |
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.
Using cast
is a bad practice, thus, removing it.
@@ -493,26 +493,6 @@ def _populate_tree( | |||
) | |||
return RepoTree(Repo(full_name, branch), repo_files) | |||
|
|||
def get_repositories(self, fetch_max_pages: bool = False) -> Sequence[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.
I'm integrating this into github/integration.py
.
return [ | ||
{ | ||
"name": i["name"], | ||
"identifier": i["full_name"], | ||
"default_branch": i.get("default_branch"), | ||
} | ||
for i in self.get_client().get_repositories(fetch_max_pages) |
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.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #83921 +/- ##
==========================================
+ Coverage 87.55% 87.66% +0.11%
==========================================
Files 9542 9536 -6
Lines 540927 546135 +5208
Branches 21282 21206 -76
==========================================
+ Hits 473623 478786 +5163
- Misses 66949 66983 +34
- Partials 355 366 +11 |
I need to fix the tests first. |
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 for doing this!
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.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This unifies the signature of
get_repositories
of all SCM integrations.It also consolidates
get_repositories
for GitHub.