-
Notifications
You must be signed in to change notification settings - Fork 637
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
Fix #632 Change the default settings to "useRtmConnect: true" #633
Conversation
Codecov Report
@@ Coverage Diff @@
## main #633 +/- ##
=======================================
Coverage 83.82% 83.82%
=======================================
Files 6 6
Lines 408 408
Branches 91 91
=======================================
Hits 342 342
Misses 38 38
Partials 28 28 Continue to review full report at Codecov.
|
# https://api.slack.com/changelog/2021-10-rtm-start-to-stop | ||
options.rtm.useRtmConnect = true | ||
catch e | ||
console.error(e) |
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 for better developer experience. The code has been silently ignoring the exception raised here.
@@ -9,9 +9,18 @@ exports.use = (robot) -> | |||
# reacts to only messages by insalled workspace users in a shared channel | |||
installedTeamOnly: process.env.INSTALLED_TEAM_ONLY? | |||
try | |||
options.rtm = JSON.parse(process.env.HUBOT_SLACK_RTM_CLIENT_OPTS) | |||
catch | |||
options.rtm = JSON.parse(process.env.HUBOT_SLACK_RTM_CLIENT_OPTS or '{}') |
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 you set the env variable to an empty string, the parsing fails. This change improves the behavior in the scenario.
I've verified the behavior with an example Hubot app. |
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.
LGTM
LGTM |
Summary
This pull request fixes #632 -- refer to the issue for the context
Requirements (place an
x
in each[ ]
)