Skip to content
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

Allow command handlers to match regexes #846

Merged
merged 3 commits into from
Mar 23, 2021

Conversation

itowlson
Copy link
Contributor

Summary

The App#command method currently only subscribes to a specific slash command identified by exact name. In a few cases it is useful to have more flexibility - for example, a gateway that transforms commands into messages for another system might need to be agnostic about which specific commands the user wants to use it with. This PR adds a RegExp option to the App#command signature, which enables this and related behaviours.

Fixes #841.

Notes to reviewers

  • I was not sure how the tests fitted together. I would welcome feedback or guidance on what I've done.
  • There is an issue of what happens if the user supplies multiple overlapping regexes, e.g. app.command(/h*/) and app.command(/he*/). I followed the behaviour of shortcut here, where if it runs the listener for all matching regexes. This may be surprising/unwelcome but it is at least consistent. I documented the shortcut behaviour as part of this; but it may be that you prefer to leave this undefined or say it's unsupported so people don't take a dependency on the current behaviour. Note this is distinct from Issue 281 message array support #761 where the regexes could be passed in an array in a single message call (and therefore were looped over within a single piece of middleware). Again I'd welcome feedback or guidance on this!

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label Mar 23, 2021
@CLAassistant
Copy link

CLAassistant commented Mar 23, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #846 (9b17e71) into main (0e469c9) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 9b17e71 differs from pull request most recent head 385cc5f. Consider uploading reports for the commit 385cc5f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
+ Coverage   65.89%   65.98%   +0.08%     
==========================================
  Files          13       13              
  Lines        1173     1176       +3     
  Branches      343      344       +1     
==========================================
+ Hits          773      776       +3     
  Misses        334      334              
  Partials       66       66              
Impacted Files Coverage Δ
src/App.ts 83.63% <ø> (ø)
src/middleware/builtin.ts 84.09% <100.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e469c9...385cc5f. Read the comment docs.

@seratch
Copy link
Member

seratch commented Mar 23, 2021

@itowlson You're so quick! Thanks a lot for taking the time to make this pull request 🙇

I was not sure how the tests fitted together. I would welcome feedback or guidance on what I've done.

The added tests cover all the possible patterns. Perfect 👍

I followed the behaviour of shortcut here, where if it runs the listener for all matching regexes. This may be surprising/unwelcome but it is at least consistent.

This is intended behavior in Bolt for JS. Thanks for clarifying this.

I documented the shortcut behaviour as part of this; but it may be that you prefer to leave this undefined or say it's unsupported so people don't take a dependency on the current behaviour. Note this is distinct from #761 where the regexes could be passed in an array in a single message call (and therefore were looped over within a single piece of middleware). Again I'd welcome feedback or guidance on this!

The changes look good to me. In the future, we may want to improve the duplication but we can do it separately.

If you don't mind, could you send another pull request for the document changes? Merging document changes updates the live website immediately. We would like to merge the document updates right after releasing a new version including your change. Once you move the doc changes to another PR, we will merge the code changes!

@seratch seratch added enhancement M-T: A feature request for new functionality and removed untriaged labels Mar 23, 2021
@seratch seratch self-assigned this Mar 23, 2021
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this pull request does not include the document changes, we can merge this PR 👍

@itowlson
Copy link
Contributor Author

Sorry about that @seratch, I noticed some remarks about documentation in the contributor's guide but I overlooked them when it came to doing the PR. I'll back those out and create a new one for them.

@itowlson itowlson force-pushed the command-name-regex-match branch from 529e82f to cf298c9 Compare March 23, 2021 01:02
@itowlson itowlson force-pushed the command-name-regex-match branch from cf298c9 to 385cc5f Compare March 23, 2021 01:03
@itowlson itowlson mentioned this pull request Mar 23, 2021
2 tasks
@itowlson
Copy link
Contributor Author

Docs backed out and moved to #847.

@seratch
Copy link
Member

seratch commented Mar 23, 2021

@itowlson Thanks again for your prompt work 👍 I'm sure this is great and already ready for merge but, just in case, I will wait for other maintainers for one business day before merging it.

@seratch seratch added this to the 3.4.0 milestone Mar 23, 2021
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work @itowlson! 🎉 Thanks a lot for contribution!

@seratch seratch merged commit 65019b2 into slackapi:main Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow regex match for slash command handling
4 participants