-
Notifications
You must be signed in to change notification settings - Fork 276
WIP: Future function interactivity #713
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
Conversation
moving changes to future branch
| return self.get("authorize_result") | ||
|
|
||
| @property | ||
| def bot_access_token(self) -> Optional[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.
Can you remove this? This is duplicated with bot_token
|
|
||
| def __init__( | ||
| self, | ||
| _register_listener: Callable[..., Optional[Callable[..., Optional[BoltResponse]]]], |
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.
We can move the private function in App and use it here too. Let's remove this argument. This interface is not easy to use for developers.
| def __init__( | ||
| self, | ||
| _register_listener: Callable[..., Optional[Callable[..., Optional[BoltResponse]]]], | ||
| _base_logger: Logger, |
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.
using _ (underscore) prefix for arg names is not generally recommended. base_logger or logger would be fine
|
|
||
|
|
||
| def extract_bot_access_token(payload: Dict[str, Any]) -> Optional[str]: | ||
| if payload.get("bot_access_token") 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.
you can merge this logic to the part extracting bot_token
| function_execution_id = extract_function_execution_id(body) | ||
| if function_execution_id: | ||
| context["function_execution_id"] = function_execution_id | ||
| bot_access_token = extract_bot_access_token(body) |
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.
remove this part
slack_bolt/app/app.py
Outdated
| if self._token is not None: | ||
| # This WebClient instance can be safely singleton | ||
| req.context["client"] = self._client | ||
| req.context["client"] = create_copy(self._client) |
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.
if the singleton client does not work for next-gen functions, you can simplify this part. The extra overhead by this change should be ignorable.
def _init_context(self, req: BoltRequest):
req.context["logger"] = get_bolt_app_logger(app_name=self.name, base_logger=self._base_logger)
req.context["token"] = self._token
client_per_request: WebClient = WebClient(
token=self._token, # this can be None
base_url=self._client.base_url,
timeout=self._client.timeout,
ssl=self._client.ssl,
proxy=self._client.proxy,
headers=self._client.headers,
team_id=req.context.team_id,
)
req.context["client"] = client_per_request|
@seratch Thank you for the feedback 💯 I will add these changes to my branch 🥇 Since this feature still needs a lot of work I will close the PR and reopen it when more work is done |
moving changes to future branch
(Describe the goal of this PR. Mention any related Issue numbers)
Category (place an
xin each of the[ ])slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements (place an
xin each[ ])Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.