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

Add canvases APIs and users.discoverableContacts.lookup API #1508

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

seratch
Copy link
Member

@seratch seratch commented Jun 13, 2024

Summary

This pull request adds the following new APIs to slack-api-client library:

  • canvases.access.delete
  • canvases.access.set
  • canvases.create
  • canvases.delete
  • canvases.edit
  • canvases.sections.lookup
  • conversations.canvases.create
  • users.discoverableContacts.lookup

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.

@seratch seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Jun 13, 2024
@seratch seratch added this to the 2.29.0 milestone Jun 13, 2024
@seratch seratch self-assigned this Jun 13, 2024
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Left a few comments but thanks for tackling this!

kwargs.update({"channel_ids": ",".join(channel_ids)})
else:
kwargs.update({"channel_ids": channel_ids})
if user_ids is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note as a possible enhancement in the future, I see that both client and async client handle this list serialization into comma-separated strings at the method level. Perhaps an enhancement could be to factor that out into a utility method? I see the same pattern is repeated a lot:

➜ ack "list, Tuple"
client.py
289:            if isinstance(team_ids, (list, Tuple)):
344:        if isinstance(app_ids, (list, Tuple)):
403:        if isinstance(entity_ids, (list, Tuple)):
422:        if isinstance(entity_ids, (list, Tuple)):
442:        if isinstance(barriered_from_usergroup_ids, (list, Tuple)):
446:        if isinstance(restricted_subjects, (list, Tuple)):
477:        if isinstance(barriered_from_usergroup_ids, (list, Tuple)):

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah it should be good; we can do it in the future

self,
*,
title: Optional[str] = None,
document_content: Dict[str, str],
Copy link
Contributor

Choose a reason for hiding this comment

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

The API mentions document_content is optional - should this be wrapped w/ Optional, then?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, thanks but I don't want to encourage skipping the parameter because the current set of public APIs does not allow you to append sections if the document does not have anything. Your canvas document should have at least one section (like h1 title) for the following operations.

slack_sdk/web/async_client.py Show resolved Hide resolved
slack_sdk/web/async_client.py Outdated Show resolved Hide resolved
slack_sdk/web/async_client.py Show resolved Hide resolved
slack_sdk/web/client.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 85.05%. Comparing base (862796c) to head (3f059ef).

Files Patch % Lines
slack_sdk/web/async_client.py 70.00% 12 Missing ⚠️
slack_sdk/web/client.py 70.00% 12 Missing ⚠️
slack_sdk/web/legacy_client.py 70.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1508      +/-   ##
==========================================
- Coverage   85.09%   85.05%   -0.05%     
==========================================
  Files         112      112              
  Lines       12320    12440     +120     
==========================================
+ Hits        10484    10581      +97     
- Misses       1836     1859      +23     

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

self,
*,
title: Optional[str] = None,
document_content: Dict[str, str],
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, thanks but I don't want to encourage skipping the parameter because the current set of public APIs does not allow you to append sections if the document does not have anything. Your canvas document should have at least one section (like h1 title) for the following operations.

kwargs.update({"channel_ids": ",".join(channel_ids)})
else:
kwargs.update({"channel_ids": channel_ids})
if user_ids is not None:
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah it should be good; we can do it in the future

slack_sdk/web/async_client.py Show resolved Hide resolved
slack_sdk/web/client.py Show resolved Hide resolved
Co-authored-by: Fil Maj <maj.fil@gmail.com>
@seratch seratch merged commit 67028d6 into slackapi:main Jun 13, 2024
12 checks passed
@seratch seratch deleted the canvas-apis branch June 14, 2024 00:46
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.

None yet

2 participants