-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: add nested queries to match book + syntactic sugar #387
Conversation
WalkthroughThe pull request introduces a series of patches across multiple Dojo Engine packages, focusing on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
🧹 Nitpick comments (5)
packages/sdk/src/clauseBuilder.ts (3)
35-49
: Consider validating the arguments more strictly.
Currently,KeysClause
doesn't check whethermodels
andkeys
align or perform any additional validation. If there's a mismatch or invalid input, it may produce surprising behavior.
74-85
: Correct the doc comment to match the method name.
The JSDoc says "Saves some keyboard strokes to get a Composite 'Or' Clause," but the function isAndComposeClause
. Consider updating the documentation to accurately reference "And" instead of "Or."- * Saves some keyboard strokes to get a Composite "Or" Clause + * Saves some keyboard strokes to get a Composite "And" Clause
87-97
: Correct the doc comment to match the method name.
The JSDoc says "Saves some keyboard strokes to get a Composite 'And' Clause," but the function isOrComposeClause
. Consider updating the documentation accordingly.- * Saves some keyboard strokes to get a Composite "And" Clause + * Saves some keyboard strokes to get a Composite "Or" Clausepackages/sdk/src/__tests__/clauseBuilder.test.ts (1)
217-250
: Extend test scenarios for complex composition.
This test reveals impressive support for nested logical clauses. Consider adding negative or edge-case tests (e.g., empty arrays, invalid operators) to strengthen reliability..changeset/selfish-poets-grab.md (1)
15-15
: Revisit the wording to avoid confusion.
If referring to "matchbook" as a single noun, consider adjusting "match book" accordingly. Otherwise, if you’re referencing matching "the book," no change is needed.🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: The noun “matchbook” is normally spelled as one word.
Context: ...atch --- fix: Add nested query test to match book + syntactic sugar(BOOK_COMPOUNDS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/selfish-poets-grab.md
(1 hunks)packages/sdk/src/__tests__/clauseBuilder.test.ts
(2 hunks)packages/sdk/src/clauseBuilder.ts
(3 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/selfish-poets-grab.md
[uncategorized] ~15-~15: The noun “matchbook” is normally spelled as one word.
Context: ...atch --- fix: Add nested query test to match book + syntactic sugar
(BOOK_COMPOUNDS)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (7)
packages/sdk/src/clauseBuilder.ts (4)
11-13
: Interface approach looks good!
TheClauseBuilderInterface
cleanly enforces a uniformbuild()
method across different clause builders.
51-72
: Well-structured helper function.
MemberClause
clearly delegates its logic to thewhere
method, providing a concise and consistent way to build clauses. This is a nice abstraction.
174-174
: Implementation overwrites orClauses.
This method looks correct for setting OR clauses. Take care if multiple calls toor()
within the same composite are intended to accumulate clauses; this code replaces previously stored OR clauses.
179-179
: Implementation overwrites andClauses.
This method sets AND clauses as expected. Similarly toor()
, re-callingand()
replaces previously stored clauses, so ensure that’s the desired behavior.packages/sdk/src/__tests__/clauseBuilder.test.ts (3)
2-7
: New imports extend test coverage.
Bringing inAndComposeClause
,MemberClause
, etc., ensures thorough testing of new clause builder functionalities.
251-312
: Thorough verification of nested composites.
The deeply nestedexpect
block accurately confirms the correctness of the composite structure.
313-359
: Excellent demonstration of syntactic sugar.
This comparison between manual compositions and helper functions boosts readability and confirms feature parity.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Improvements
Testing