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

health: move to mypy #1620

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

WilliamBergamin
Copy link
Contributor

Summary

This PR aims to follow the Bolt Pythons #1130

MyPy was found to execute much fast the PyType in this project, these changes adapt the project to use MyPy instead of PyType

From my short experience with mypy:

  • Good documentation 📜
  • Good error surfacing
  • Easy to configure
  • It offers a language server, this helps with development since it integrate with IDEs

Note: removing all the type: ignore comments from the project may cause breaking changes or have a ripple effect through the entire project

Testing

Category

  • 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 (Documents)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)
  • other

Requirements

  • 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.

@WilliamBergamin WilliamBergamin added enhancement M-T: A feature request for new functionality area:async labels Dec 20, 2024
@WilliamBergamin WilliamBergamin self-assigned this Dec 20, 2024
@WilliamBergamin WilliamBergamin changed the title Move to my py health: move to mypy Dec 20, 2024
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 81.87809% with 110 lines in your changes missing coverage. Please review.

Project coverage is 85.06%. Comparing base (241db14) to head (9ba8e07).

Files with missing lines Patch % Lines
slack_sdk/web/async_client.py 80.55% 14 Missing ⚠️
slack_sdk/web/client.py 80.55% 14 Missing ⚠️
slack_sdk/web/legacy_client.py 80.55% 14 Missing ⚠️
slack_sdk/socket_mode/builtin/internals.py 47.36% 10 Missing ⚠️
...sdk/oauth/installation_store/amazon_s3/__init__.py 36.36% 7 Missing ⚠️
slack_sdk/audit_logs/v1/logs.py 33.33% 4 Missing ⚠️
slack_sdk/web/internal_utils.py 20.00% 4 Missing ⚠️
slack_sdk/audit_logs/v1/client.py 25.00% 3 Missing ⚠️
...k_sdk/oauth/installation_store/sqlite3/__init__.py 40.00% 3 Missing ⚠️
slack_sdk/socket_mode/aiohttp/__init__.py 66.66% 3 Missing ⚠️
... and 24 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1620      +/-   ##
==========================================
+ Coverage   85.03%   85.06%   +0.03%     
==========================================
  Files         113      113              
  Lines       12661    12658       -3     
==========================================
+ Hits        10766    10768       +2     
+ Misses       1895     1890       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

LGTM; let one minor comment

placeholder=placeholder,
)
self.min_query_length = min_query_length


class DialogBuilder(JsonObject):
attributes = {} # no attributes because to_dict has unique implementation
attributes = set() # no attributes because to_dict has unique implementation
Copy link
Member

Choose a reason for hiding this comment

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

It seems this property is unused, so changing the type should not affect the behavior. That said, changing from dict to set object may not make sense. This legacy component won't be enhanced anymore but using a set object might be a cause of future bugs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:async enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants