-
Notifications
You must be signed in to change notification settings - Fork 838
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
Conversation
There was a problem hiding this 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: |
There was a problem hiding this comment.
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)):
There was a problem hiding this comment.
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], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Codecov ReportAttention: Patch coverage is
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. |
self, | ||
*, | ||
title: Optional[str] = None, | ||
document_content: Dict[str, str], |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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
Co-authored-by: Fil Maj <maj.fil@gmail.com>
Summary
This pull request adds the following new APIs to slack-api-client library:
Category (place an
x
in 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
x
in each[ ]
)python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.