Skip to content

Improve typing when using 'get' on an API response#1283

Merged
seratch merged 1 commit intoslackapi:mainfrom
eddyg:improve-type-handling-for-response-get
Oct 27, 2022
Merged

Improve typing when using 'get' on an API response#1283
seratch merged 1 commit intoslackapi:mainfrom
eddyg:improve-type-handling-for-response-get

Conversation

@eddyg
Copy link
Copy Markdown
Contributor

@eddyg eddyg commented Oct 25, 2022

Summary

This is a simple somewhat complicated change that improves type handling when calling .get() on API responses.

For example, this line:

    if api_response and api_response.get("ok") and len(api_response.get("messages", [])):

currently generates this warning in Pyright:

Argument of type "Unknown | None" cannot be assigned to parameter "__obj" of type "Sized" in function "len"
  Type "Unknown | None" cannot be assigned to type "Sized"
    "__len__" is not present

This simple typing change alleviates this issue.

This is the first use of TypeVar in slack_sdk that I see... are there compatibility issues with older Python versions, perhaps? Opening this PR so this fix can at least be considered.

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /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 x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 25, 2022

Codecov Report

Merging #1283 (c47a4e8) into main (c900682) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1283   +/-   ##
=======================================
  Coverage   85.62%   85.62%           
=======================================
  Files          84       84           
  Lines        7827     7827           
=======================================
  Hits         6702     6702           
  Misses       1125     1125           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@misscoded misscoded requested a review from seratch October 25, 2022 22:19
@seratch seratch self-assigned this Oct 25, 2022
@seratch seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Oct 25, 2022
@seratch seratch added this to the 3.19.2 milestone Oct 25, 2022
@seratch
Copy link
Copy Markdown
Contributor

seratch commented Oct 27, 2022

@eddyg Thanks for improving this!

@eddyg
Copy link
Copy Markdown
Contributor Author

eddyg commented Oct 27, 2022

@seratch Thank you for your time in reviewing my changes and pushing me to investigate an even better solution than I came up with originally.

@eddyg eddyg deleted the improve-type-handling-for-response-get branch October 27, 2022 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement M-T: A feature request for new functionality Version: 3x web-client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants