Adds DTO array transformation and JSON serialization #30
Conversation
There was a problem hiding this comment.
@JasonTheAdams Some early thoughts here before I look more closely. FWIW, the feedback is probably the most crucial for us to discuss, to shape the remainder of the PR accordingly.
JasonTheAdams
changed the title
There was a problem hiding this comment.
@JasonTheAdams I think at a high level this looks great, but there are many things in this PR which shows the typical weaknesses of AI assisted code generation - such as overkill checks, or random patterns applied in some places but not others.
I left some of the concrete feedback below, let's discuss as needed, but most importantly decide for an approach and stick with it. This is the type of thing where AI generated code still requires a lot of revision.
JasonTheAdams
force-pushed
the
dto-serlialization
branch
from
b49cf6d to
b6890c8
Compare
|
Pardon the force push. I accidentally added commits to the wrong branch. 😆 |
|
This is ready for review, again, @felixarntz! |
| ], | ||
| ], | ||
| 'required' => ['message', 'finishReason', 'tokenCount'], | ||
| 'required' => [self::KEY_MESSAGE, self::KEY_FINISH_REASON, self::KEY_TOKEN_COUNT], |
There was a problem hiding this comment.
I'm not sure every provider returns a token_count on an individual candidate? Should we leave that optional? Or would we prefer consistency and either set to 0 or try some estimation if it's not provided?
There was a problem hiding this comment.
Ohh, good question. How would token count be used downstream? Would folks be more likely to prefer to have something? Or would it be important to know that the model didn't provide one? The use case isn't clear enough for me to make a suggestion. 🤔
There was a problem hiding this comment.
I'm not sure yet myself 😆
Let's leave this for a later discussion, if it becomes more relevant.
Follow up from: #28 (comment)
This adds support for serializing and unserializing DTOs to and from JSON. It introduces a
WithJsonSerializationinterface that all DTOs implement. This interfaces extends the built-in JsonSerializable interface so thatjson_encode()can be used on DTOs. It also adds afromJson()static method so DTOs can be constructed from the same JSON data.