-
Notifications
You must be signed in to change notification settings - Fork 155
feat: type extension #1460
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
feat: type extension #1460
Conversation
62b7c8b to
034c54c
Compare
ghost
left a comment
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.
LGTM in general, but encoding is tricky, so we need a bit more tests.
Types from aliased packages will contain the name of the alias in the derived types e.g.
OptDecimal1Decimal. For unique package names they will work the same as other types e.g.OptDecimal. Happy to remove this functionality though and resort to forcing users to usex-ogen-namewhen they have clashes if this feels too strange.
Let's remove it, then.
728c62f to
2ecbdfd
Compare
2ecbdfd to
f6fecc8
Compare
8ece4b7 to
f562a71
Compare
53816aa to
a51770d
Compare
e454143 to
dff62fb
Compare
|
@tdakkota I made a bunch of changes and improved the (un)marshalling of external values to avoid the reflection cost in the stdlib |
|
Actually hold off I just found a bug. Will fix and report back when done. |
5158f45 to
c1fbc88
Compare
|
@tdakkota I've resolved the bugs mentioned in the previous message and this is again ready for review. |
c1fbc88 to
bb12d74
Compare
70061c0 to
a239b94
Compare
a239b94 to
cc6435b
Compare
|
@tdakkota sorry for the delay but I have just added another commit to improve the encode/decode by removing the pointless string functions and added support for binary types. |
6f35464 to
51d5e76
Compare
abfb8ae to
6c991e5
Compare
6c991e5 to
19fdbd3
Compare
|
@tdakkota I've made the final small changes and squashed the comments so it's ready for merging unless you have any other concerns. |
ghost
left a comment
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.
Well, it was a long road.
Huge thanks for all your contributions to the ogen.
|
Thank you! |
Add support for the extension
x-ogen-type. E.g.x-ogen-type: github.com/shopspring/decimal.Decimalwill result in the Go type of the schema beingdecimal.Decimal.A few gotchas regarding the use:
github.com/ogen-go/ogen/json.Marshaler,github.com/ogen-go/ogen/json.Unmarshaler,json.Marshaler,json.Unmarshaler,encoding.TextMarshaler,encoding.TextUnmarshaler,encoding.BinaryMarshaler,encoding.BinaryUnmarshaler) and will be (un)marshaled using these interfaces. If they don't implement any of them it will fall back to using theMarshalandUnmarshalfunctions from stdlibjsonpackage.decimal,decimal2,decimal3etc.x-ogen-type.*pkg.Type) will result in a non-pointer (e.g.pkg.Type). I made an attempt at adding support for pointers but it got pretty complex pretty quickly and it wouldn't be possible to simply hijackPrimitiveTypeto achieve this. I did also try wrapping the type inir.Pointerbut for optional pointers this results in a pointer to a pointer (e.g.**pkg.Type) which I feel is just as bad as using a value instead of a pointer. If you think this is a requirement I can add a new typeKindExternalwith custom templates to handle this.string,intetc.) but on second thought, do we even want this? All builtin types can be generated using standard schema.This PR also fixes an issue where setting default values for aliases was broken if the alias had a format (e.g.
date-time,ipetc.)Closes #1335