-
Notifications
You must be signed in to change notification settings - Fork 8
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
Customer entity types #121
Conversation
# Conflicts: # src/api/resources/document/client/Client.ts # src/api/resources/graph/client/Client.ts # src/api/resources/graph/resources/edge/client/Client.ts # src/api/resources/graph/resources/episode/client/Client.ts # src/api/resources/graph/resources/node/client/Client.ts # src/api/resources/group/client/Client.ts # src/api/resources/memory/client/Client.ts # src/api/resources/user/client/Client.ts # src/serialization/types/AddMemoryResponse.ts
# Conflicts: # package.json # reference.md # src/api/resources/document/client/Client.ts # src/api/resources/graph/client/Client.ts # src/api/resources/graph/resources/edge/client/Client.ts # src/api/resources/graph/resources/episode/client/Client.ts # src/api/resources/graph/resources/node/client/Client.ts # src/api/resources/group/client/Client.ts # src/api/resources/memory/client/Client.ts # src/api/resources/user/client/Client.ts # src/api/types/index.ts # src/serialization/types/index.ts # src/version.ts
# Conflicts: # package.json
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.
❌ Changes requested. Reviewed everything up to a71cc7d in 2 minutes and 36 seconds
More details
- Looked at
2889
lines of code in34
files - Skipped
0
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. src/Client.ts:32
- Draft comment:
The client getters (document, graph, memory, group, user) all create new instances on each call. In other parts of the code, lazy initialization is used with the '??=' operator. Consider using lazy initialization consistently if caching is intended. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/wrapper/ontology.ts:101
- Draft comment:
The function entityModelToAPISchema uses 'entityType.fields'. Ensure that 'fields' is defined on the entity model. The type EntityType in wrapper/ontology.ts is inferred from zod schema where 'fields' is required; confirm that it matches API expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that a certain property is defined and matches expectations, which violates the rule against asking the author to confirm or ensure things. It does not provide a specific suggestion or point out a clear issue.
3. src/api/resources/graph/client/requests/GraphSearchQuery.ts:16
- Draft comment:
Field names in the raw schema use snake case (e.g. 'min_fact_rating'). Verify these names match the API spec if any inconsistency between client (camelCase) and server (snake_case) is intended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/version.ts:1
- Draft comment:
Version updated to 2.8.0 consistently across files. Ensure that any external documentation is updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that external documentation is updated, which is similar to asking them to double-check or ensure something is done. This violates the rule against asking the author to ensure something is done.
5. src/wrapper/index.ts:5
- Draft comment:
In ZepClient wrapper, new Memory and Graph instances are created on each getter call. Consider caching if performance is a concern, matching how BaseClient may be used elsewhere. - Reason this comment was not posted:
Comment was on unchanged code.
6. src/wrapper/ontology.ts:71
- Draft comment:
The entityFields helper functions (text, integer, float, boolean) call .parse on a literal object. Consider handling possible parse errors or document that invalid descriptions will throw exceptions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The parse operations here are on hardcoded literal objects where the only variable is the description string. The schema only validates that description is a string, which is already guaranteed by the TypeScript type. The other fields are literals that match the schema exactly. The chance of a parse error is extremely low and would indicate a developer error rather than a runtime issue.
The comment raises a valid point about error handling best practices. Unhandled exceptions could potentially cause issues in production.
However, these are internal helper functions operating on developer-provided values with very simple validation. Adding error handling here would add complexity without meaningful benefit since parse errors would indicate developer mistakes that should fail fast.
The comment should be removed as it suggests adding unnecessary complexity to handle an edge case that would only occur due to developer error.
7. src/api/types/EntityPropertyType.ts:5
- Draft comment:
The EntityPropertyType is defined as a union of literals and then exported as const. This pattern is acceptable; just ensure that all references use the exported type and constant as needed. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
8. src/serialization/resources/graph/client/requests/GraphSearchQuery.ts:17
- Draft comment:
The serializer for 'minFactRating' uses core.serialization.property with 'min_fact_rating'. Verify that naming matches the expected API payload. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to verify if the naming matches the expected API payload. This falls under asking the author to confirm their intention, which is against the rules. Therefore, this comment should not be approved.
9. src/wrapper/graph.ts:10
- Draft comment:
In the setEntityTypes method, the mapping from entity model to API schema is performed. Verify that entityModelToAPISchema handles edge cases (e.g., missing properties) as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify that edge cases are handled as expected, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
10. src/version.ts:1
- Draft comment:
SDK version is updated consistently to '2.8.0'. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_8q3rKBlCQsUoNIxE
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Introduces entity type management in Zep client with new classes, methods, and serialization schemas, updating SDK to version 2.8.0.
EntityType
,EntityProperty
, andEntityPropertyType
insrc/api/types
for defining entity types.listEntityTypes
andsetEntityTypesInternal
methods inGraph
class insrc/api/resources/graph/client/Client.ts
.setEntityTypes
insrc/wrapper/graph.ts
to handle entity type setting with validation.ApidataEntityTypeRequest
,EntityType
, andEntityTypeResponse
insrc/serialization
.entityModelToAPISchema
function insrc/wrapper/ontology.ts
for converting entity models to API schemas.entity_type_example.ts
inexamples/graph
to demonstrate usage of entity types.package.json
andsrc/version.ts
.This description was created by
for a71cc7d. It will automatically update as commits are pushed.