-
Notifications
You must be signed in to change notification settings - Fork 0
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 splits support #216
Add splits support #216
Conversation
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AutoblocksAPIClient
participant API
Client->>AutoblocksAPIClient: Call method with splits
AutoblocksAPIClient->>API: Make GET request with splits query parameter
API-->>AutoblocksAPIClient: Return response with splits data
AutoblocksAPIClient-->>Client: Return promise with items including splits
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/client.ts (1)
288-290
: Consider simplifying thesplitsQueryParam
constructionThe current implementation is correct, but it can be simplified for better readability and to avoid potential issues with
undefined
values.Consider using this alternative implementation:
- const splitsQueryParam = args.splits - ? `?splits=${args.splits?.map(encodeURIComponent).join(',')}` - : ''; + const splitsQueryParam = args.splits && args.splits.length > 0 + ? `?splits=${args.splits.map(encodeURIComponent).join(',')}` + : '';This change ensures that we only add the query parameter when
splits
is both defined and non-empty, which is likely the intended behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/client.ts (1 hunks)
🔇 Additional comments (3)
src/client.ts (3)
275-275
: LGTM: Method signature and return type updatesThe changes to the method signature and return type are correct and consistent with the PR objective of adding splits support.
Also applies to: 282-283
295-295
: LGTM: Correct usage ofsplitsQueryParam
The
splitsQueryParam
is correctly appended to both URL constructions, ensuring that the splits are included in the request regardless of whether a specific revision is being fetched or not.Also applies to: 300-300
275-300
: Summary: Successful implementation of splits supportThe changes made to the
getDataset
method effectively implement the splits support as intended in the PR objective. The method signature, return type, and URL construction have been updated consistently to handle the newsplits
parameter. This enhancement allows for more flexible querying of datasets by supporting split-based filtering.A few key points:
- The
splits
parameter is optional, maintaining backward compatibility.- The return type now includes
splits
information for each item, providing more detailed data to the caller.- The URL construction logic correctly handles the presence or absence of the
splits
parameter.These changes enhance the functionality of the
AutoblocksAPIClient
class without breaking existing usage, which is a good practice for API evolution.
Summary by CodeRabbit