web-api(fix):Update SearchMessagesArguments to support cursor pagination #2361
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2361 +/- ##
=======================================
Coverage 93.01% 93.01%
=======================================
Files 40 40
Lines 11111 11111
Branches 713 713
=======================================
Hits 10335 10335
Misses 764 764
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
52ed286 to
7350c81
Compare
zimeg
left a comment
There was a problem hiding this comment.
@vegeris LGTM! This seems good to merge when the time is right but it might be nice to add a type test for this before? 🧪
node-slack-sdk/packages/web-api/test/types/methods/search.test-d.ts
Lines 28 to 37 in 893a37d
I left a few other comments too!
| * @description Paginate through collections of data by setting the `cursor` parameter to `*` for the first "page" | ||
| * or a `next_cursor` attribute returned by a previous request's `response_metadata`. | ||
| * Use the `count` parameter to set the number of items to return per page rather than `limit`. |
There was a problem hiding this comment.
🧮 praise: Thank you for including this.
| extends TokenOverridable, | ||
| TraditionalPagingEnabled, | ||
| Searchable, | ||
| SearchMessagesCursorPagination {} |
There was a problem hiding this comment.
👁️🗨️ suggestion(non-blocking): I don't believe a similar cursor is used elsewhere? Might be nice to inline this IMO but no blocker!
There was a problem hiding this comment.
This is how Biome wants it to look 😛
Yeah, I think an API-specific interface makes sense here. The same count param from the TraditionalPagingEnabled interface is reused for cursor pagination
f8936ad to
db6bdbb
Compare
Summary
Requirements ()