-
Notifications
You must be signed in to change notification settings - Fork 45
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: default operator in getEntities of the sdk #299
feat: default operator in getEntities of the sdk #299
Conversation
WalkthroughThe pull request introduces modifications to enhance the Changes
Possibly related PRs
Poem
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 (3)
packages/sdk/src/execute.ts (1)
Line range hint
18-18
: LGTM! Consider future-proofing the Direction type.The addition of the custom type mapping for "dojo_starter::models::Direction" enhances type safety and aligns well with the PR objectives. It provides more specific type information for direction-related operations in the SDK.
For future-proofing, consider using an enum or a const object for the Direction type. This would make it easier to add new directions or modify existing ones in the future. For example:
export const Direction = { Left: "Left", Right: "Right", Up: "Up", Down: "Down", } as const; type Direction = typeof Direction[keyof typeof Direction]; // Then update the type mapping: "dojo_starter::models::Direction": Direction;This approach would centralize the definition of directions and make it easier to maintain consistency throughout the codebase.
🧰 Tools
🪛 Biome
[error] 72-72: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/sdk/src/__tests__/convertQueryToClause.test.ts (1)
186-221
: Excellent addition of the "Or" operator test case.The new test case effectively covers the functionality of using the "Or" operator in composite queries, which aligns well with the PR objectives. The structure and expectations are consistent with other test cases in the file.
To further improve clarity, consider adding a brief comment explaining the purpose of this test case at the beginning of the
it
block.Here's a suggested addition:
it("should convert multiple model queries using or operator", () => { + // This test ensures that the convertQueryToClause function correctly + // handles queries with multiple models when using the "Or" operator const query: QueryType<MockSchemaType> = { // ... (rest of the test case) }); });packages/sdk/src/convertQuerytoClause.ts (1)
17-18
: Approve changes with a minor suggestion for type safetyThe addition of the
defaultOperator
parameter and its usage in theComposite
clause construction is a good enhancement. It aligns well with the PR objectives by allowing users to specify the logical operator for composite queries while maintaining backward compatibility.Consider using a union type for better type safety:
type LogicalOperator = "And" | "Or"; export function convertQueryToClause<T extends SchemaType>( query: QueryType<T>, schema: T, defaultOperator: LogicalOperator = "And" ): torii.Clause | undefined { // ... rest of the function }This change would make the code more maintainable and less prone to errors if new operators are added in the future.
Also applies to: 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/sdk/src/tests/convertQueryToClause.test.ts (1 hunks)
- packages/sdk/src/convertQuerytoClause.ts (2 hunks)
- packages/sdk/src/execute.ts (1 hunks)
- packages/sdk/src/getEntities.ts (1 hunks)
- packages/sdk/src/parseEntities.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/src/parseEntities.ts
🧰 Additional context used
🔇 Additional comments (6)
packages/sdk/src/getEntities.ts (2)
42-42
: Approve the update toconvertQueryToClause
function callThe
convertQueryToClause
function call has been correctly updated to include the newschema
anddefaultOperator
parameters. This change is consistent with the function signature modifications and implements the intended enhancement.
Line range hint
1-89
: Verify test coverage and documentation updatesWhile the changes to
getEntities
function appear to meet the PR objectives, please ensure that:
- Appropriate tests have been added to cover the new functionality, especially for different
defaultOperator
values.- The function's JSDoc comments are updated to reflect the new
defaultOperator
andschema
parameters.- Any relevant documentation outside this file is updated to explain the new query capabilities.
To check for test coverage, run the following script:
packages/sdk/src/execute.ts (2)
74-75
: LGTM! Improved formatting enhances readability.The formatting change in the
MapOutputType
type definition improves code readability without altering the underlying logic. This adjustment aligns with typical TypeScript formatting conventions and helps maintain consistency in complex type definitions.
Line range hint
1-180
: Overall assessment: Changes improve type safety and readability.The modifications in this file, including the addition of the custom type mapping for directions and the formatting improvement, align well with the PR objectives. They enhance type safety, improve readability, and contribute to a more robust SDK. No major issues were identified during the review.
🧰 Tools
🪛 Biome
[error] 72-72: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/sdk/src/__tests__/convertQueryToClause.test.ts (1)
Line range hint
1-221
: Overall approval of changes to the test file.The addition of the new test case for the "Or" operator integrates seamlessly with the existing test structure. The file maintains a consistent and comprehensive approach to testing the
convertQueryToClause
function across various scenarios.Great job on expanding the test coverage while maintaining the file's organization and clarity.
packages/sdk/src/convertQuerytoClause.ts (1)
Line range hint
1-338
: Verify implementation ofgetEntities
method enhancementThe changes in this file successfully implement the ability to specify a default logical operator for composite queries in the
convertQueryToClause
function. However, the PR objectives mention enhancing thegetEntities
method, which is not present in this file.To ensure full implementation of the PR objectives, please verify that the
getEntities
method has been updated in the appropriate file to utilize this new functionality. Run the following script to locate and inspect thegetEntities
method:Please ensure that the
getEntities
method has been updated to accept and pass thedefaultOperator
parameter toconvertQueryToClause
.✅ Verification successful
getEntities
method enhancement verified successfully.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate and display the getEntities method implementation # Search for the getEntities method echo "Searching for getEntities method:" rg --type typescript -n 'function getEntities' # Display the content of the file containing getEntities echo "\nDisplaying the content of the file containing getEntities:" rg --type typescript -A 20 'function getEntities'Length of output: 378
Script:
#!/bin/bash # Description: Locate and display the getEntities method implementation # Search for the getEntities method in .ts and .tsx files echo "Searching for getEntities method:" rg -g '*.ts' -g '*.tsx' -n 'function getEntities' # Display the content of the file containing getEntities echo -e "\nDisplaying the content of the file containing getEntities:" rg -g '*.ts' -g '*.tsx' -A 20 'function getEntities'Length of output: 1787
packages/sdk/src/getEntities.ts
Outdated
@@ -36,9 +36,10 @@ export async function getEntities<T extends SchemaType>( | |||
}) => void, | |||
limit: number = 100, // Default limit | |||
offset: number = 0, // Default offset | |||
options?: { logging?: boolean } // Logging option | |||
options?: { logging?: boolean }, // Logging option | |||
defaultOperator: "And" | "Or" = "And" |
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.
this should be first in the params
@@ -14,7 +14,8 @@ import { convertQueryToEntityKeyClauses } from "./convertQueryToEntityKeyClauses | |||
*/ | |||
export function convertQueryToClause<T extends SchemaType>( | |||
query: QueryType<T>, | |||
schema: T | |||
schema: T, | |||
defaultOperator: "And" | "Or" = "And" |
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.
move above schema
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.
Looks good - can you update the typedoc above the functions with the changes.
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 (4)
packages/sdk/src/getEntities.ts (1)
Line range hint
11-22
: Approve documentation update, but request documentation forschema
parameter.The documentation has been correctly updated to include the new
defaultOperator
parameter. However, theschema
parameter is missing from the documentation. Please add documentation for theschema
parameter, including its purpose and expected type.Add documentation for the
schema
parameter, for example:* @param {T} schema - The schema definition for the entities being queried.Please adjust the description based on the actual purpose of the
schema
parameter.packages/sdk/src/__example__/index.ts (1)
145-145
: Approve change and suggest improvementsThe addition of "And" as the first argument to
db.getEntities
successfully implements the new feature allowing users to specify the logical operator for composite queries. This change aligns well with the PR objectives.To further improve the code:
- Consider adding a comment explaining the purpose of this new parameter:
// Specify "And" as the logical operator for combining multiple model queries
It would be beneficial to add another example using the "Or" operator to demonstrate the new flexibility in querying. This could be done by duplicating the existing
getEntities
call and changing the operator to "Or".Don't forget to update the SDK documentation to reflect this new parameter and its usage.
packages/sdk/src/index.ts (1)
71-83
: LGTM with a minor suggestionThe
getEntities
method signature and implementation have been correctly updated to include the newdefaultOperator
parameter. The changes are consistent with the documentation and maintain backward compatibility.For consistency with other methods in this file, consider using object destructuring for the parameters. This would make it easier to add or remove parameters in the future without changing the method signature. Here's a suggested refactor:
getEntities: ({ defaultOperator = 'And', query, callback, limit = 100, offset = 0, options }) => getEntities( defaultOperator, client, query, schema, callback, limit, offset, options ),This approach would also allow you to specify default values directly in the method signature.
packages/sdk/src/types.ts (1)
Line range hint
294-304
: LGTM! Consider a minor documentation improvement.The addition of the
defaultOperator
parameter aligns well with the PR objectives, allowing users to specify the logical operator for composite queries. The implementation maintains backward compatibility by defaulting to "And".Consider updating the parameter description to clarify its usage:
- @param {string} defaultOperator - The operator used for the query. Default is And + @param {string} defaultOperator - The logical operator used to combine conditions in composite queries. Can be "And" or "Or". Default is "And".This change would provide more context about when and how the parameter is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/sdk/src/example/index.ts (1 hunks)
- packages/sdk/src/tests/convertQueryToClause.test.ts (5 hunks)
- packages/sdk/src/convertQuerytoClause.ts (2 hunks)
- packages/sdk/src/getEntities.ts (3 hunks)
- packages/sdk/src/index.ts (1 hunks)
- packages/sdk/src/types.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk/src/tests/convertQueryToClause.test.ts
- packages/sdk/src/convertQuerytoClause.ts
🧰 Additional context used
🔇 Additional comments (6)
packages/sdk/src/getEntities.ts (3)
Line range hint
1-89
: Overall assessment: Approved with minor improvements needed.The changes successfully implement the ability to specify a default operator for queries, which aligns with the PR objectives. The implementation is correct and the documentation has been partially updated. However, there are a few points to address:
- Reorder the function parameters to put
defaultOperator
first, as suggested in a previous review.- Provide clarification and documentation for the new
schema
parameter.- Ensure that the
convertQueryToClause
function correctly uses the new parameters.Once these minor issues are addressed, the changes will be fully ready for merging.
43-43
: Approve the usage ofdefaultOperator
, but request clarification onschema
.The
defaultOperator
is correctly passed to theconvertQueryToClause
function, which aligns with the PR objectives. However, the purpose and usage of theschema
parameter remain unclear. Could you provide more information on howschema
is used withinconvertQueryToClause
and why it's necessary?Let's check the implementation of
convertQueryToClause
to understand how it uses these parameters:#!/bin/bash # Description: Examine the convertQueryToClause function rg --type typescript -A 10 'function convertQueryToClause' packages/sdk/src/
Line range hint
31-39
: Approve the addition ofdefaultOperator
, but suggest parameter reordering and clarification.The addition of the
defaultOperator
parameter aligns well with the PR objectives, providing flexibility in query operations. However, there are a few points to address:
- As per the previous review comment, consider moving
defaultOperator
to be the first parameter in the function signature for consistency.- The new
schema
parameter wasn't mentioned in the PR objectives. Could you please clarify its purpose and how it's used within the function?To ensure the
schema
parameter is used correctly, let's check its usage:packages/sdk/src/index.ts (2)
63-70
: LGTM: Documentation updated correctlyThe documentation for the
getEntities
method has been properly updated to include the newdefaultOperator
parameter. The description is clear, and the default value is correctly specified.
63-83
: Summary: Successful implementation of thedefaultOperator
featureThe changes to the
getEntities
method successfully implement the newdefaultOperator
feature as described in the PR objectives. The modifications:
- Add a new
defaultOperator
parameter to the method signature.- Update the documentation to reflect this new parameter.
- Adjust the implementation to pass the
defaultOperator
to the underlying function.These changes enhance the flexibility of the SDK by allowing users to specify the logical operator used in composite queries. The implementation maintains backward compatibility, ensuring that existing code will continue to work without modification.
To ensure that these changes are properly integrated and don't introduce any unintended side effects, please run the following verification script:
This script will help ensure that the
defaultOperator
parameter is used consistently throughout the SDK, that function calls are updated correctly, and that appropriate test coverage is in place.packages/sdk/src/types.ts (1)
Line range hint
1-391
: Overall impact of changes is minimal and well-contained.The modification to the
getEntities
method is well-isolated and doesn't introduce any inconsistencies with other parts of the file. TheQueryType<T>
and other related types remain unchanged, which is appropriate as the logical operator is a separate concern from the query structure.To ensure that this change doesn't affect other parts of the codebase unintentionally, let's verify the usage of
getEntities
:This will help us identify any existing calls to
getEntities
that might need to be updated to include the newdefaultOperator
parameter.✅ Verification successful
Verified: The
defaultOperator
parameter ingetEntities
is optional with a default value, ensuring existing calls remain unaffected. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of getEntities in the codebase rg --type typescript 'getEntities\(' -A 5Length of output: 78
Script:
#!/bin/bash # Search for all usages of getEntities in .ts and .tsx files rg 'getEntities\(' --glob '*.ts' --glob '*.tsx' -A 5Length of output: 7757
Script:
#!/bin/bash # Search for all usages of getEntities that do not include the defaultOperator parameter rg 'getEntities\([^,]*\)' --glob '*.ts' --glob '*.tsx'Length of output: 349
Script:
#!/bin/bash # Locate the definition of getEntities in the codebase rg 'function getEntities' --glob '*.ts' --glob '*.tsx'Length of output: 145
Script:
#!/bin/bash # Retrieve the full definition of the getEntities function to inspect its parameters rg 'export async function getEntities' --glob '*.ts' --glob '*.tsx' -A 10Length of output: 791
Thinking about this more we should have the Operator within the Query itself, this makes the most sense. It is rather strange it is outside of the Query. Can you make adjustments for this. It should exist within the Query Type The functions should default to one or the other. Use the types where possible. |
Alright, that sounds better |
closing as will open specific issue - i have made default OR. So to achieve your desire right now you can just use the default, or do a nested query. |
Introduced changes
Currently, the output query when retrieving multiple models using a composite query looks like this:
The problem here is that the logical operator
And
is being used to combine the clauses, which only retrieves entities where both models (DeckCard
andRound
) exist together in the same query result. However, this is not always the desired behavior.To address this, I introduced the ability to specify the logical operator (e.g., And, Or) in the getEntities function.
Checklist
Summary by CodeRabbit
New Features
Documentation