-
Notifications
You must be signed in to change notification settings - Fork 228
feat: chat stream helper #1545
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
base: main
Are you sure you want to change the base?
feat: chat stream helper #1545
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1545 +/- ##
============================================
+ Coverage 73.32% 73.39% +0.07%
- Complexity 4515 4547 +32
============================================
Files 477 480 +3
Lines 14274 14447 +173
Branches 1487 1498 +11
============================================
+ Hits 10467 10604 +137
- Misses 2917 2946 +29
- Partials 890 897 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
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.
@srtaalej This is awesome to review and test! Thanks for putting it together 👾 ✨
I'm leaving a few comments around how we might surface this for the web client that might make it easier to use in calling code, similar to other SDKs, but please let me know if language limits these idea 🙏
| * MethodsClient client = Slack.getInstance().methods(token); | ||
| * ChatStreamHelper stream = ChatStreamHelper.builder() | ||
| * .client(client) | ||
| * .channel("C0123456789") | ||
| * .threadTs("1700000001.123456") | ||
| * .recipientTeamId("T0123456789") | ||
| * .recipientUserId("U0123456789") | ||
| * .bufferSize(100) | ||
| * .build(); |
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.
👁️🗨️ thought: This works well but I'm wondering if it's possible to attach the chat stream helper to the client for an implementation similar to files upload?
String token = System.getenv("SLACK_BOT_TOKEN");
var client = Slack.getInstance().methods(token);
client.filesUploadV2(req -> req.content("greetings!").channel("C01223456789"));
slack-api-client/src/main/java/com/slack/api/methods/ChatStreamHelper.java
Outdated
Show resolved
Hide resolved
| // Create a response object to return (mimicking the append response structure) | ||
| response = new ChatAppendStreamResponse(); | ||
| response.setOk(startResponse.isOk()); | ||
| response.setChannel(startResponse.getChannel()); | ||
| response.setTs(startResponse.getTs()); | ||
| response.setWarning(startResponse.getWarning()); | ||
| response.setError(startResponse.getError()); |
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.
⭐ praise: Nice! I forget if casting between related types might also work here but I think the explicit setting is most clear.
…mHelper.java Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 6.0.0 to 6.0.1. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@1af3b93...8e8c483) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 6.0.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/setup-java](https://github.com/actions/setup-java) from 5.0.0 to 5.1.0. - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](actions/setup-java@dded088...f2beeb2) --- updated-dependencies: - dependency-name: actions/setup-java dependency-version: 5.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5.5.1 to 5.5.2. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@5a10915...671740a) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 5.5.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/stale](https://github.com/actions/stale) from 10.1.0 to 10.1.1. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@5f858e3...9971854) --- updated-dependencies: - dependency-name: actions/stale dependency-version: 10.1.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
zimeg
left a comment
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.
@srtaalej This is testing swell! The one comment I have that we might want to finalize before merging is the naming of this streamer 📸
The other SDKs have a variation of chat_stream or chatStream and IMHO we could omit "helper" from calling code, but please let me know if reason exists otherwise. No blocker to the functionalities so I'm in favor of whatever makes most sense!
slack-api-client/src/main/java/com/slack/api/methods/AsyncMethodsClient.java
Outdated
Show resolved
Hide resolved
slack-api-client/src/main/java/com/slack/api/methods/ChatStreamHelper.java
Outdated
Show resolved
Hide resolved
| ) throws IOException, SlackApiException { | ||
| if (state == State.COMPLETED) { | ||
| throw new SlackChatStreamException("Cannot stop stream: stream state is " + state); | ||
| } |
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.
stop method throw SlackChatStreamException too? I'm not certain so wanted to ask!
slack-api-client/src/main/java/com/slack/api/methods/MethodsClient.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * The state of the chat stream. | ||
| */ | ||
| public enum State { | ||
| STARTING, | ||
| IN_PROGRESS, | ||
| COMPLETED | ||
| } |
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.
🔭 question(non-blocking): Can we make this private? I don't think this is something that should be changed from outside of this class!
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.
📚 thought(non-blocking): Changing State to StreamState might be kind in reading later, but also personal preference.
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 agree!
…mHelper.java Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
…odsClient.java Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
…ient.java Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
This PR adds a helper to the
chat streamingmethodsCategory (place an
xin each of the[ ])Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.