chore(github): fix typing for github client #75480
Conversation
cathteng
commented
Aug 1, 2024
•
cathteng
commented
github-actions
bot
added
the
Scope: Backend
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/sentry/shared_integrations/client/base.py
Did you find this useful? React with a 👍 or 👎 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #75480 +/- ##
========================================
Coverage 78.18% 78.19%
========================================
Files 6809 6816 +7
Lines 302915 303040 +125
Branches 52132 52159 +27
========================================
+ Hits 236844 236958 +114
+ Misses 59686 59684 -2
- Partials 6385 6398 +13
|
| self.limit = info.get("limit") | ||
| self.remaining = info.get("remaining", 0) | ||
| self.reset = info.get("reset") | ||
| self.used = info.get("used") |
There was a problem hiding this comment.
would rather not weaken these -- the brackets are preferrable since we know the structure (whereas .get would allow a programming error to slip through unnoticed on a typo)
| if resp.status_code == 204: | ||
| return {} | ||
| return BaseApiResponse(status_code=204) |
There was a problem hiding this comment.
I've been meaning to make this change for a while -- but it's not so simple
some things test if not response: ... and assume that's a 204 so it needs quite a lot of work to safely unwind this (probably needs some sort of opt-in dont_special_case_204s=True which can then be targetted with @overload and not return the empty dict here -- but it's quite a bit of work!)
this probably needs some more involved tracing to nuke properly
also callers really shouldn't need BaseApiResponseX since it should be one of the two types specifically (based on the raw_response=...) -- so I suspect the github client doesn't need this union either and will always get specifically one or the other response type
There was a problem hiding this comment.
i see... i put the {} back then. is it possible to define __bool__ to determine if an object is truthy? for 204s we could return False
There was a problem hiding this comment.
I'm a little torn because I think this is an improvement -- but I also think that the whole api client stuff needs to be torn out and rethought (like I think ideally there would be some response schemas with typeddicts for the structure -- and not have to guess whether it's a requests.Response, the weird dict subclass, or empty dict from 204s)
it'd be a larger chunk of work but it would make everything way easier to work with and have actual proper types
| class GithubProxyClient(IntegrationProxyClient): | ||
| integration: RpcIntegration | Integration |
There was a problem hiding this comment.
I think this is due to it being expected from a subclass -- but it turns out there's only one subclass! so let's merge GithubProxyClient and the class below and then I think this annotation may be able to be removed?
| super().__init__( | ||
| org_integration_id=org_integration_id, | ||
| verify_ssl=verify_ssl, | ||
| logging_context=logging_context, | ||
| **kwargs, | ||
| integration_id=getattr(self.integration, "id", None), |
There was a problem hiding this comment.
this getattr shouldn't be necessary if the integration: Integration type is correct -- since that always has an id: int
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
