Conversation
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams This looks great! A few notes, most of which should be relatively quick to resolve though.
Some feedback may still apply here from #30, and I'd say let's merge #30 first, so then we can finalize this here.
| * Represents a required configuration option for an AI model. | ||
| * | ||
| * This class defines an option that must be set when using a model, | ||
| * including its name and the required value. |
There was a problem hiding this comment.
This documentation is incorrect, likely a result of Claude Code misunderstanding RequiredOption. This is not an option that must be set when using a model, it's an option that the implementing code requires the model to support. We should clarify that.
I think there are other instances in the docs here as well where we simply refer to "required value" etc., would be good to clarify what that actually means.
There was a problem hiding this comment.
I believe I improved this, but let me know if you have even clearer documentation in mind.
0569ad4 to
7c13ac1
Compare
|
Pardon the force push. I accidentally added commits to the wrong branch. 😆 |
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams Looks great - a few last bits and then I think we can merge this!
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams Thank you for the updates, LGTM!
Depends on #30
This introduces the DTOs distinct to the Extender API. In the domains, this is the Providers and Models areas.