-
Notifications
You must be signed in to change notification settings - Fork 24
Adds Extender API DTOs #31
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
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😆 |
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.
@JasonTheAdams Looks great - a few last bits and then I think we can merge this!
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.
@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.