-
Notifications
You must be signed in to change notification settings - Fork 104
Introducing schema component #2
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
e79a9d8 to
477cdb1
Compare
|
Yep, I agree with this direction. On I’m fine with keeping |
ff9c82c to
88c8067
Compare
|
When thinking about updates over time, I feel keeping in sync If you give me a 👍 I would update the PR like this:
|
a5bc2a5 to
91bc19c
Compare
6dd3786 to
f7bf9a7
Compare
| /** | ||
| * @author Kyrian Obikwelu <koshnawaza@gmail.com> | ||
| */ | ||
| class Parser |
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.
we have this twice still, and also the method string is handled in multiple places, but that's to sort with #3
88c8067 to
ee0dffe
Compare
|
Got merged into main with 6025f89 |
Moving over php-mcp/schema which is based on 2025-03-26/schema.ts
Would still apply changes:
switching toreadonlyclassesconstruct,make,toArray,fromArrayandjsonSerializemy2cents: the array structure is needed for
json_encodeand therefore thejsonSerializethat's the main use case for converting into array - and we have three different ways of constructing those value objects, why?my proposal would be to reduce the amount of methods per class to three:
__construct(...)for developers to instantiatemake(array $data): staticfor creation from array datajsonSerialize(): array(this would reduce the need to array_map/toArray calls on subobjects since jsonSerialize works recursively)WDYT @CodeWithKyrian - am I missing something?
Still WIP