-
Notifications
You must be signed in to change notification settings - Fork 400
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 in blockAction handler to SlackFunction #1657
Conversation
Codecov Report
@@ Coverage Diff @@
## next-gen #1657 +/- ##
===========================================
Coverage ? 80.71%
===========================================
Files ? 20
Lines ? 1779
Branches ? 505
===========================================
Hits ? 1436
Misses ? 221
Partials ? 122 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
👋 Hello! Yay thank you for adding this functionality. I was wondering if you'd considered just reusing the Function's action method within the blockAction method, to make it super explicit that this is an alias?
Something like:
export class SlackFunction {
action(){
}
blockAction(){
return this.action.apply(this, arguments);
}
}
src/SlackFunction.spec.ts
Outdated
@@ -332,6 +370,71 @@ describe('SlackFunction module', () => { | |||
assertNode.rejects(shouldReject); | |||
}); | |||
}); | |||
it('app.function.blockAction() should execute all provided callbacks', async () => { |
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.
Thoughts on locating this test and the one below it within `describe('runInteractivityHandlers', ....) on ln241?
src/SlackFunction.ts
Outdated
* This function is added as an alias for the action() function to create | ||
* consistency with having a blockSuggestion() function. | ||
* | ||
* NOTE: This function is a direct copy of the action() function and |
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.
Is there any way to use the .action() function within blockAction instead of copying the code? If we can do this, it would probably reduce the likelihood of accidental drift between the two interfaces, since we expect them to be the same.
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.
absolutely! I'll try what you suggested above and have this updated by later today! 🙌
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.
👍
Summary
In #1645 we introduced
SlackFunction.blockSuggestion()
as a handler for theblock_suggestions
event. While we already have a.action()
handler forblock_actions
, we should also add in aSlackFunction.blockAction()
handler to be consistent with this naming convention so folks can use either.action()
or.blockAction()
as they wish!Requirements (place an
x
in each[ ]
)