-
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
RFC: Python Slack Client v2.0 #384
Comments
Sounds great !
If that is not in your plan, it would be great for non-official client to be able to import some logic from this new client. Such as message parsing, method definition, token validation, etc |
Some initial feedback (mostly small stuff):
Sounds like there's a typo or missing word around "Developers are encouraged and"
I think this should be "Whether you're using Slack's RTM API or Events API, the"
simply link your application's callbacks
This sounds awesome 🙌
This is interesting and implies that we should consider adding support to vend this data via API for clients to ingest. The other thing that sticks out here is that my understanding is that we report a pretty clear error in cases where you use an unsupported token type from our API but it seems like we're not surfacing that well to developers. I haven't finished reading the spec yet but are we doing something to remedy that issue? |
@ovv Thanks for reading and being the first to comment on this issue. The current plan is to continue supporting Python 2.7 as you've read in #368. So this v2 client ideally would work for both Python 2.7 and 3. If there's enough demand however, we may consider creating a Python 3.6+ version that takes advantage of |
@bredman Thanks for the feedback. I'll fix those typo/grammar errors now!
We do provide this API response:
|
@bredman @RodneyU215 token type is also something we should add to the API spec (cc @episod) |
Any chance we could ask for pep484 annotations on methods like chat_postMessage, or is it going to remain generally kwargs-based? |
Looks great!
|
@wittekm I appreciate the question. It's something we could do for sure. I imagine the implementation would be different though if we wanted to ensure it was Python 2.7 compatible. Regardless I believe this is a great idea. |
@joshzana Thanks for the feedback and the really insightful questions.
|
That would be great
To expend on this. In my opinion a second packages handling everything non I/O related would be the best. Other client could rely on it without too much dependency and it would make maintenance and upgrade easier for everyone involved. |
I had the same question about asynchronous requests while reading the new design. We might consider powering this with a Requests add-on, like requests-futures. Apparently Futures are natively available in Python 3.2 but have been backported to Python 2. |
Have you considered using our newly available JSON schema to generate Python bindings for the web API? This seems to be the most widely used generator: Response validation appears to be typically done with jsonschema. |
Think this reads better as
To make a Slack API call, there are 4 nested objects created.
Can we include the defaults for these attributes? |
I think it's probably best to get rid of this for now but one thing to be aware of is that token rotation is coming back in the future for our regular tokens. See the top of the page about WTA apps and token rotation. |
This is probably only important if this is a reference document in the future but it would be nice to mention how to access this stuff? Also should we maybe have separate variables for users to interact with for headers, status code info, and payload? These are related but very separate items and it seems a bit weird to shove them all into a single dictionary. |
For
|
From "Costs and Tradeoffs"
Is this just if they want to interact with the API? Or is there some other reason they need their own WebClient? |
Thanks for the perspective and feedback @ajmacd!
I’d like to move forward with creating a branch for Python 3.6+ with Async support and other goodies built-in. That being said this will come as a lower priority when compared to other tasks such as bug fixing critical client 1.x and getting client 2.x shipped asap.
I think it’s a great idea to to use our JSON schema to generate our Web Client API. I’ve not yet personally used I’ve also been looking into swagger-codegen, but it doesn’t seem simple to add. I believe it may be best to work on this post-release of client 2.x. 🤔 |
Thanks for the feedback @bredman!
Really great points here. I’ll update our readme to ensure it’s clear how to interact with the response data. I’ll also disentangle headers and status code from the response body.
The SlackResponse class will implement both a
No! The
This is only if they’d like to interact with the Slack Web API. The RTM API only supports posting simple messages formatted using our default message formatting mode. It does not support attachments, Blocks or other message formatting modes. So I’d like to encourage them to use the WebClient if they’d like to send messages. |
Thanks to everyone who has shared feedback with us so far! I really appreciate every single comment and reaction added to this issue. New changes: Based on the feedback that I’ve received on this RFC I’ve made a few notable changes in the implementation.
Client v1 Support Plan:
Provisional timeline: I’ve created the v2 branch (#394) and have begun development on this. As of today, March 29th, we’re in “Pre-Alpha”. My goal is to be in Beta by April 24th. This ultimately depends on the feedback I receive from the community here as well as my QA and security team’s review at Slack. Thank you all again! 🙇 |
I have been invited by Slack support to send API commentary to this issue. (Please accept my apologies if this feedback is better addressed elsewhere; I would be happy to move it there.) As I use the slack web client API and not RTM, I look forward to a simpler Slack client. However, one of my primary concerns is validating that I am using the API correctly. While it is convenient to send unstructured key-value data to Slack, it is very hard to validate that this code is being used correctly, and it is not clear that this is an area of interest in the new designs. I think this will be of particular interest to people who are going to have to maintain additional client state themselves. I've handrolled a simple substitute of this for use in my app, and reproduce some portions here to give an idea of how to help API consumers access the API correctly. For instance, the following code excerpt facilitates the construction of message payloads. (You will note that it targets the older Slack attachment format, not blocks; please also forgive the use of Payload = Dict[str, Any]
class SlackString(str):
def for_slack(self) -> str:
return text_for_slack(self)
class TrustedSlackString(SlackString):
def for_slack(self) -> str:
return self
def text_for_slack(text: str) -> str:
"""
Slack requires these escapes and does not support escapes other than these three.
"""
return text.replace("&", "&").replace("<", "<").replace(">", ">")
class SlackAttachment(NamedTuple):
# scalar fields (in Slack documentation order)
fallback: Optional[SlackString] = None
pretext: Optional[SlackString] = None
author_name: Optional[SlackString] = None
author_link: Optional[SlackString] = None
author_icon: Optional[SlackString] = None
title: Optional[SlackString] = None
title_link: Optional[SlackString] = None
text: Optional[SlackString] = None
image_url: Optional[SlackString] = None
thumb_url: Optional[SlackString] = None
footer: Optional[SlackString] = None
footer_icon: Optional[SlackString] = None
ts: Optional[int] = None
# list fields
fields: Optional[List[SlackField]] = None
actions: Optional[List[SlackAction]] = None
# rendering control fields
mrkdwn_in: Optional[List[SlackPayloadFieldName]] = None
def for_slack(self) -> Payload:
d = named_tuple_to_payload(self)
if not d.get("fallback") and self.actions:
d["fallback"] = "\n".join(
action.fallback() for action in self.actions or []
)
return d
def named_tuple_to_payload(self: Any) -> Payload:
return dict_to_payload(self._asdict())
def dict_to_payload(dictionary: Dict[str, Any]) -> Payload:
values = {}
for k, v in dictionary.items():
if v and hasattr(v.__class__, "for_slack"):
values[k] = v.for_slack()
elif isinstance(v, list):
values[k] = list(item.for_slack() for item in v)
elif v is not None:
values[k] = v
return values The specific implementation here is less important than the idea that it's really nice to use type checking to validate that a generated SlackMessage is in fact a valid SlackMessage. This permits us to check messages against a known good template as we run unit tests, linters, and other such tools — instead of waiting until runtime to validate this against a live API. I understand if Slack, organizationally, is less than thrilled at the idea of maintaining these definitions across all sorts of different clients in different languages — but the alternative is that you make integrators do the same work, without the same organizational knowledge. |
^ They've approved using PEP 484 (type annotations) for the new client :) (Also, you may want to look into TypedDict, a dict equivalent of NamedTuple - just removes that conversion step) |
Hi @twhaples, Thanks for providing feedback on this. Like @wittekm mentioned I'm working on add type annotations for all of the API methods. Initially only for the required arguments, but in a subsequent PR I'll add types for all of the optional parameters as well. Like you mentioned this will only solve the problem for simple payloads. For structures such as Blocks these can be fairly complex. I believe we'll explore creating a builtIn Message class for this, but it may take some time and experimentation to get it right. I'm happy to review any PR's 😉for what this could look like. |
We've officially uploaded the v2 version of the SDK as a pre-release beta. Those who'd like to test it out can install it like this:
I'll now be closing this RFC. Please feel free to open a new issue with the label v2 to provide any additional feedback. Thanks again to everyone who's help guide the development of this release. ❤️ |
This PR implements the changes specified in RFC slackapi#384. However, the most notable deviation is that we've decided that v2 will target Python 3.6+ only! Python 2.7+ will continue to be supported in v1 until Dec 31st, 2019. See additional details in slackapi#384!
Abstract
This issue proposes a redesign of the Slack Python SDK/Client to address several design flaws that surfaced as the scope and complexity of the platform grew. Currently every app built on RTM is required to implement a loop over the stream of events coming in. Web HTTP requests could be simpler to write and more performant when scaled.
This issue proposes that we split up the client into an RTM client and a Web client. We'd like to remove any state not explicitly used for the client's function. As well as make it simpler for developers to interact with the Web API and respond to RTM events.
Motivation
The goal of our Python SDK is to make it simple for Python developers to create Slack apps. We believe that developers should be able to go from Readme to a running app in less than 10 minutes.
Currently there are a number of limitations in our existing project that prevent this goal from being realized. From a high level, the complexity in our SDK has led to a number of tough to triage bugs. It also makes it harder for new app developers or possible first time contributors to get up and running quickly.
Working with the Slack Web API:
Client#api_call→Server#api_call→SlackRequest#do→requests#post
Server.py
.Working with the Slack RTM API:
Other challenges:
Existing Slack Client Architecture:
Example App in v1:
Here's a simple example app that replies "Hi <@userid>!" in a thread if you send it a message containing "Hello".
Proposal
Primary Changes
v2 Slack Client Architecture
Example App in v2:
Here's that same simple example app that replies "Hi <@userid>!" in a thread if you send it a message containing "Hello".
Additional Features and enhancements
WebClient
xoxb
vsxoxp
: We validate the token used is supported for that method. This solves issues like #326.requests
library's json feature. This ensures all data sent will be automatically properly encoded. This solves issues like #337.next_cursor
attribute. See #343.RTMClient
Issues resolved
Unresolved discussions:
Detailed design
slack.WebClient
A WebClient allows apps to communicate with the Slack Platform's Web API. The Slack Web API is an interface for querying information from and enacting change in a Slack workspace. This client handles constructing and sending HTTP requests to Slack as well as parsing any responses received into a
SlackResponse
.Attributes:
#token
(str): A string specifying an xoxp or xoxb token.#use_session
(bool): An boolean specifying if the client should take advantage of urllib3's connection pooling. Default isTrue
.#base_url
(str): A string representing the Slack API base URL. Default ishttps://www.slack.com/api/
#proxies
(dict): If you need to use a proxy, you can pass a dict of proxy configs. e.g. {'https': "https://127.0.0.1:8080"} Default is None.#timeout
(int): The maximum number of seconds the client will wait to connect and receive a response from Slack. Default is30
seconds.Methods:
#api_call()
: Constructs a request and executes the API call to Slack.Recommended usage:
Example manually creating an API request: (Mostly for backwards compatibility)
Note:
slack.web.SlackResponse
An iterable container of response data.
Attributes:
#data
(dict): The json-encoded content of the response. Along with the headers and status code information.Methods:
#validate()
: Check if the response from Slack was successful.#get()
: Retrieves any key from the response data.#next()
: Retrieves the next portion of results, if 'next_cursor' is present.Example:
Note:
slack.RTMClient
An RTMClient allows apps to communicate with the Slack Platform's RTM API. The event-driven architecture of this client allows you to simply link callbacks to their corresponding events. When an event occurs this client executes your callback while passing along any information it receives.
Attributes:
#token
(str): A string specifying anxoxp
orxoxb
token.#connect_method
(str): An string specifying if the client will connect withrtm.connect
orrtm.start
. Default isrtm.connect
.#auto_reconnect
(bool): When true the client will automatically reconnect when (not manually) disconnected. Default is True.#ping_interval
(int): automatically send "ping" command every specified period of seconds. If set to0
, do not send automatically. Default is 30.#ping_timeout
(int): The amount of seconds the ping should timeout. If the pong message is not received. Default is 10.#proxies
(dict): If you need to use a proxy, you can pass a dict of proxy configs. e.g. {'https': "https://user:pass@127.0.0.1:8080"} Default is None.#base_url
(str): A string representing the Slack API base URL. Default is https://www.slack.com/api/Methods:
#ping()
: Sends a ping message over the websocket to Slack.#typing()
: Sends a typing indicator to the specified channel.#on()
: Stores and links callbacks to websocket and Slack events.#start()
: Starts an RTM Session with Slack.#stop()
: Closes the websocket connection and ensures it won't reconnect.Example:
Note:
Migrating to 2.x
Import changes: The goal of this project is to provide a set of tools that ease the creation of Python Slack apps. To better align with this goal we’re renaming the main module to
slack
. Fromslack
developers can import various tools.RTM Client API Changes:
The RTM client has been completely redesigned please refer above for the new API.
We no longer store any team data.: In the current 1.x version of the client we store some channel and user information internally on
Server.py
inclient
. This data will now be available in the open event for consumption. Developers are then free to store any information they choose. Here's an example:Web Client API Changes:
Token refresh removed: This feature originally shipped as a part of Workspace Tokens. Since we're heading in a new direction it's safe to remove this, along with any related attributes stored on the client.
refresh_tokentoken_update_callbackclient_idclient_secret#api_call()
:timeout
param has been removed. Timeout is passed at the client level now.kwargs
param has been removed. You must specify where the data you pass belongs in the request. e.g. 'data' vs 'params' vs 'files'...etcSlackAPIMixin: The Slack API mixin provides built-in methods for Slack's Web API. These methods act as helpers enabling you to focus less on how the request is constructed. Here are a few things that this mixin provides:
Costs and Tradeoffs
Separating Web from RTM:
WebClient
.WebClient SlackAPIMixin:
.
separated methods we’d have to implement nested classes for each method family. However valuing maintainability we’ve opted to use a partial snake_case naming convention replacing every.
with an_
.RTMClient redesign:
Server.py
as a convenience for apps to reuse the initial state without having to query for additional data. The challenge with this approach is that this data can become stale/inaccurate very quickly depending on the team. We also didn't store all of the information available.Rejected Alternatives
Do Nothing
If we avoid a redesign we’d still have to address existing bugs in v1. New features will be slower to add and first-time contributors would face an increasingly steep ramp-up period. Debugging will also remain difficult as the existing classes today are tightly coupled.
Requirements
The text was updated successfully, but these errors were encountered: