-
Notifications
You must be signed in to change notification settings - Fork 94
initial port of dartantic_interface types #619
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
base: main
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request introduces a new package genai_primitives by porting several types from dartantic_interface. The changes include new data models for chat messages, message parts (text, data, links, tools), and tool definitions, along with a comprehensive example and unit tests. The overall structure is good, but I've found several critical issues with equality checks and hashCode implementations that could lead to incorrect behavior, particularly when using these objects in hash-based collections. Additionally, there are some concerns with JSON serialization and a dependency on a private API. My review comments provide specific suggestions to address these points.
| /// Gets the extension for a MIME type. | ||
| static String? extensionFromMimeType(String mimeType) { | ||
| final String ext = defaultExtensionMap.entries | ||
| .firstWhere( | ||
| (e) => e.value == mimeType, | ||
| orElse: () => const MapEntry('', ''), | ||
| ) | ||
| .key; | ||
| return ext.isNotEmpty ? ext : null; | ||
| } |
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 method relies on default_extension_map from package:mime/src/default_extension_map.dart, which is a private implementation detail of the mime package (as indicated by the src directory and the // ignore: implementation_imports). Relying on private APIs is risky as they can change without notice in minor or patch versions, potentially breaking your code. Since the mime package doesn't seem to offer a public reverse lookup API, a safer approach would be to define a local map of the common MIME types to extensions that your package needs to support.
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.
it's worse to duplicate a half-assed version of the list of extensions that isn't properly maintained
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.
Agreed. Really the mime package should be fixed to export a reverse lookup.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| } | ||
|
|
||
| /// The role of a message author. | ||
| enum ChatMessageRole { |
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.
Can you add more details to doc strings for enum and its items?
I can represent a confused user here:
-
doc for enum is saying that each item is a role of a message author, while doc for each item is describing type of message, not author
-
can you elaborate what is 'system'?
-
documentation is saying 'from', but not 'to'. Is it correct that system always send messages to model? If yes, should we specify it here? And the same about model: is it correct that model's messages are intended to be handled by system and then system will show to the user whatever is intended for user?
-
or my assumptions above are completely wrong and the enum relates to difference between 'user prompt' and 'system prompt' somehow?
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.
Maybe in this library instead of ChatMessage having field ChatMessageRole role, it make sense for it to have the field 'String type', so that every dependent can define their set of message types.
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.
Another option:
class ChatMessage<T>{
...
final T type;
...
}Then users will be able to pass their enums for the message type. Maybe some of them will have more than one model and more than one system.
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.
Or, we can remove type completely, so that users just derive from ChatMessage and add whatever fields they want to add. Or users can compose ChatMessage into another object, that contains envelope information.
This seems to be cleanest option for me.
| final ToolPartKind kind; | ||
|
|
||
| /// The unique identifier for this tool interaction. | ||
| final String id; |
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.
How about renaming it to interactionId, to avoid confusion and make it clear how to name this id, when it is copied outside of this class?
Is it correct that id of tool call is expected to be equal to id of tool call result? If yes, should it be documented here?
| : ''; | ||
|
|
||
| @override | ||
| bool operator ==(Object other) => |
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.
Would you mind updating it to use this boilerplate, so that readers do not have to detect if it is identical to what is recommended in style guide:
|
Thank you! It looks good. Left some naming and documentation related comments. |
| final String id; | ||
|
|
||
| /// The name of the tool. | ||
| final String name; |
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 it be named 'toolName'? Unlike other properties 'name', this 'name' is not name of the enclosing object (ToolCall or ToolResult).
|
Yes you're correct about the tool result id matching the tool can id. I like the idea of updating the name and the docs to reflect that, although I don't know what that name should be. I'll provide an update ASAP. |
| final Map<String, dynamic>? arguments; | ||
|
|
||
| /// The result of a tool execution (null for calls). | ||
| final dynamic result; |
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 use Object? instead of dynamic everywhere?
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 second that. Dynamic turns off type checking.
I suggest |
|
@csells, I raised many questions about minor details. There are options how to proceed:
If you want to actively participate in discussion, let's go with (2). |
Or 'requestId'? |
Description
fixes #626
Adds the following types ported from dartantic_interface and migrated to json_schema_builder:
Pre-launch Checklist
///).