-
Notifications
You must be signed in to change notification settings - Fork 399
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
Fixes #1241 Replacing instances of console with logger #1242
Fixes #1241 Replacing instances of console with logger #1242
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1242 +/- ##
=======================================
Coverage 73.22% 73.22%
=======================================
Files 17 17
Lines 1438 1438
Branches 431 431
=======================================
Hits 1053 1053
Misses 300 300
Partials 85 85 Continue to review full report at Codecov.
|
try { | ||
const result = await client.chat.postMessage({ | ||
channel: welcomeChannelId, | ||
text: `Welcome to the team, <@${event.user.id}>! 🎉 You can introduce yourself in this channel.` |
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.
I don't think this change is correct.
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.
Whoops, I think you're right, I was testing with the wrong event! I was going to check the doc but there isn't a full example there either: https://api.slack.com/events/team_join.
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.
Indeed, the document should be comprehensive but you can rely on the outputs by the Java SDK project at this moment:
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.
Thanks! I just tested it again to be sure and reverted the change now.
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
@srajiang If this PR looks good to you too, you can "squash and merge" this tomorrow! |
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.
Nice job with these @wongjas!
Summary
#1241 Replace instances of
console.log
where you have access to the built-in Bolt loggerRequirements (place an
x
in each[ ]
)