Enable pytype and fix detected issues#1128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1128 +/- ##
==========================================
- Coverage 88.10% 87.99% -0.12%
==========================================
Files 110 110
Lines 10681 10708 +27
==========================================
+ Hits 9410 9422 +12
- Misses 1271 1286 +15
Continue to review full report at Codecov.
|
| self, | ||
| *, | ||
| query_params: Optional[Dict[str, any]] = None, | ||
| query_params: Optional[Dict[str, Any]] = None, |
There was a problem hiding this comment.
Using any works in the Python syntax but pytype recommends using typing.Any instead.
| status_code=e.code, | ||
| raw_body=response_body, | ||
| headers=e.headers, | ||
| headers=dict(e.headers.items()), |
There was a problem hiding this comment.
HTTPError#headers returns an email.Message object, which mostly behaves like a dict. By changing this, the headers works as a plain dict now.
slack_sdk/audit_logs/v1/client.py
Outdated
| if e.code == 429: | ||
| # for backward-compatibility with WebClient (v.2.5.0 or older) | ||
| resp.headers["Retry-After"] = resp.headers["retry-after"] | ||
| if "retry-after" not in resp.headers: |
There was a problem hiding this comment.
As the resp.headers here is consistently a plain dict, these changes are required.
|
|
||
| # NOTE: BAN-B310 is already checked above | ||
| resp: Optional[HTTPResponse] = None | ||
| http_resp: Optional[HTTPResponse] = None |
There was a problem hiding this comment.
Reusing resp for two types (HTTPResponse and AuditLogsResponse) is not a recommended practice in general. pytype detected this potential issue.
| } | ||
|
|
||
| fields: List[AttachmentField] | ||
| actions: List[Action] |
| v = (sec_websocket_key + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11").encode("utf-8") | ||
| expected = encodebytes(hashlib.sha1(v).digest()).decode("utf-8").strip() | ||
| actual = headers.get("sec-websocket-accept").strip() | ||
| actual = headers.get("sec-websocket-accept", "").strip() |
There was a problem hiding this comment.
pytype detected a potential bug here (usually the header value always exists, though)
| return "ping" | ||
| if opcode == FrameHeader.OPCODE_PONG: | ||
| return "pong" | ||
| return "-" |
There was a problem hiding this comment.
as this method return str not Optional[str], the default should be returned this way. "-" instead of None here does not break anything.
| api_url: str, | ||
| req_args: dict, | ||
| # set the default to an empty array for legacy clients | ||
| retry_handlers: List[AsyncRetryHandler] = [], |
There was a problem hiding this comment.
using [] / {} for method arg default value is not recommended (the value is cached and reused for the following method calls)
| token: str = None, | ||
| token: Optional[str] = None, | ||
| url: str, | ||
| query_params: Dict[str, str] = {}, |
There was a problem hiding this comment.
This does not break anything as this is an internal method, which is used only inside this class
| async def _perform_http_request( | ||
| self, *, body: Dict[str, Any], headers: Dict[str, str] | ||
| ) -> WebhookResponse: | ||
| body = json.dumps(body) |
There was a problem hiding this comment.
reusing the same variable name is not a great coding practice here (pytype detected the type error for RetryHandler initialization)
86b5448 to
ac96f77
Compare
Summary
This pull request enables pytype (https://github.com/google/pytype) in this project's CI builds. Also, it resolves the issues detected by pytype. No critical bugs are detected but the internals are much improved.
Category (place an
xin each of the[ ])/docs-src(Documents, have you run./scripts/docs.sh?)/docs-src-v2(Documents, have you run./scripts/docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.