-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ENH] Export types from chroma crate
#5676
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
Conversation
|
Export Additional Types from This pull request enhances the ergonomics of the Key Changes• Added a Affected Areas• This summary was automatically generated by @propel-code-bot |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
| pub use chroma_types::{ | ||
| plan::SearchPayload, AddCollectionRecordsRequest, AddCollectionRecordsResponse, Collection, | ||
| DeleteCollectionRecordsRequest, DeleteCollectionRecordsResponse, ForkCollectionRequest, | ||
| GetRequest, GetResponse, IncludeList, InternalSchema, Metadata, QueryRequest, QueryResponse, | ||
| SearchRequest, SearchResponse, UpdateCollectionRecordsRequest, UpdateCollectionRecordsResponse, | ||
| UpdateMetadata, UpsertCollectionRecordsRequest, UpsertCollectionRecordsResponse, Where, | ||
| }; |
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.
[BestPractice]
For improved readability and easier maintenance, consider formatting this long list of re-exported types with one item per line. This follows the official Rust Style Guide which states: "If an import does require multiple lines, then break after the opening brace and before the closing brace, use a trailing comma, and block indent the names." This formatting convention helps in producing cleaner diffs when items are added or removed in the future and makes the code more maintainable.
Suggested Change
| pub use chroma_types::{ | |
| plan::SearchPayload, AddCollectionRecordsRequest, AddCollectionRecordsResponse, Collection, | |
| DeleteCollectionRecordsRequest, DeleteCollectionRecordsResponse, ForkCollectionRequest, | |
| GetRequest, GetResponse, IncludeList, InternalSchema, Metadata, QueryRequest, QueryResponse, | |
| SearchRequest, SearchResponse, UpdateCollectionRecordsRequest, UpdateCollectionRecordsResponse, | |
| UpdateMetadata, UpsertCollectionRecordsRequest, UpsertCollectionRecordsResponse, Where, | |
| }; | |
| pub use chroma_types::{ | |
| plan::SearchPayload, | |
| AddCollectionRecordsRequest, | |
| AddCollectionRecordsResponse, | |
| Collection, | |
| DeleteCollectionRecordsRequest, | |
| DeleteCollectionRecordsResponse, | |
| ForkCollectionRequest, | |
| GetRequest, | |
| GetResponse, | |
| IncludeList, | |
| InternalSchema, | |
| Metadata, | |
| QueryRequest, | |
| QueryResponse, | |
| SearchRequest, | |
| SearchResponse, | |
| UpdateCollectionRecordsRequest, | |
| UpdateCollectionRecordsResponse, | |
| UpdateMetadata, | |
| UpsertCollectionRecordsRequest, | |
| UpsertCollectionRecordsResponse, | |
| Where, | |
| }; |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**BestPractice**]
For improved readability and easier maintenance, consider formatting this long list of re-exported types with one item per line. This follows the official Rust Style Guide which states: "If an import does require multiple lines, then break after the opening brace and before the closing brace, use a trailing comma, and block indent the names." This formatting convention helps in producing cleaner diffs when items are added or removed in the future and makes the code more maintainable.
<details>
<summary>Suggested Change</summary>
```suggestion
pub use chroma_types::{
plan::SearchPayload,
AddCollectionRecordsRequest,
AddCollectionRecordsResponse,
Collection,
DeleteCollectionRecordsRequest,
DeleteCollectionRecordsResponse,
ForkCollectionRequest,
GetRequest,
GetResponse,
IncludeList,
InternalSchema,
Metadata,
QueryRequest,
QueryResponse,
SearchRequest,
SearchResponse,
UpdateCollectionRecordsRequest,
UpdateCollectionRecordsResponse,
UpdateMetadata,
UpsertCollectionRecordsRequest,
UpsertCollectionRecordsResponse,
Where,
};
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: rust/chroma/src/types.rs
Line: 9There 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.
If Rust people wanted me to do better, they'd not have encoded bad behavior in rustfmt.
| GetRequest, GetResponse, IncludeList, InternalSchema, Metadata, QueryRequest, QueryResponse, | ||
| SearchRequest, SearchResponse, UpdateCollectionRecordsRequest, UpdateCollectionRecordsResponse, | ||
| UpdateMetadata, UpsertCollectionRecordsRequest, UpsertCollectionRecordsResponse, Where, | ||
| }; |
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.
should we just pub use chroma_types::*? seems easy to forget to add a type here when modifying the client
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.
I'd rather not. chroma_types::* is everything, which is too much to export. I'd rather miss something and fix it than have people depend on our internals.
Description of changes
For ergonomics, export every type referenced by the chroma::collection crate from chroma::types::*.
Test plan
CI
Migration plan
N/A
Observability plan
N/A
Documentation Changes
N/A