-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor: centralize json schema generation #212
base: main
Are you sure you want to change the base?
Conversation
c838eb5
to
e6f1b7c
Compare
7c98850
to
2e42f1f
Compare
2e42f1f
to
5f39ef1
Compare
@@ -213,17 +213,17 @@ final class MyTool | |||
* @param int $number The number of an object | |||
*/ | |||
public function __invoke( | |||
#[ToolParameter(pattern: '/([a-z0-1]){5}/')] | |||
#[With(pattern: '/([a-z0-1]){5}/')] |
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 propose to use an extra PR for the renaming to #[With]
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.
Besides that, we can also add an internal map, so that you can only use minimum and maximum with an int for example + we can throw an exception if the properties are not typed, or do we already?
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.
so just move it first, rename it later you mean?
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 would rename in a first PR, and see how big this PR still is. It gets messy quite fast when moving and renaming things
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.
Updated my comment, in case you missed it
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.
Yeah, should think of getting this smaller anyways, true. Refactoring wasn't as successful as hoped for
use function Symfony\Component\String\u; | ||
|
||
/** | ||
* @internal |
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.
why?
currently not working with custom types in arrays, see
MathReasoning
->Step[]