-
-
Notifications
You must be signed in to change notification settings - Fork 518
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
Introduce optional serde support for model code generation #237
Conversation
Wow. |
I guess we should change the documentation, too. Could somebody point me to where I could propose such changes? Edit: Sorry, the GH actions indeed run the codegen tests ( |
Hey @elbart, thanks for the PR!! For the documentation we can update this page https://github.com/SeaQL/seaql.github.io/blob/master/SeaORM/docs/03-generate-entity/01-sea-orm-cli.md |
@baoyachi @billy1624 do you guys have any advice on how to add additional compilation for this |
I think we can take a simple boolean value / config object (will be a new struct) in codegen to toggle the serde support, instead of using feature to enable / disable it. |
Then this won't be so complicated :P |
so you want to always depend on serde in sea-orm-codegen? I just found it great that you guys made it quasi optional everywhere (sea-schema, sea-orm), which is why I continued to do it this way. But on the other hand, the CLI / Codegen is not a runtime thing for a piece of server-software but just used on the command line. Up to you guys! Just let me know, I could also wrap the |
I re-iterated on that and you are absolutely right:
|
It's perfectly fine! Again, thank you so much for the contributions!! |
Yes, I guess it's the case. Just now I want to double check how you use
Sure, this should be enough. Thanks!! |
Update: new description, bigger refactoring to match with the actual discussed requirements Introduce optional serde support for model code generation This introduces several things to optionally support automatic derive attributes for
@billy1624 feel free to correct me wherever you think there is bulls**t produced, I am trying to write stuff as idiomatic as possible, but am not working in production environments, yet! Docs PR is here: SeaQL/seaql.github.io#9 |
This introduces several things to optionally support automatic derive attributes for `serde::{Deserialize, Serialize}` for the generated models: - introduces a `WithSerde` enum to indicate if Serialize, Deserialize, or even both should be derived from, - adds an optional cli argument `--with-serde [none: default, serialize, deserialize, both]` - adds test harness for both compact and expanded generation
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.
Hey @elbart, thanks for the support! Nice PR!
OMG... sorry for such a long code review |
I just added a new commit with your requested changes. Thanks so much for the hints. And: no need to say sorry for long code reviews. You are gatekeeping bad/ugly code from the repo and I just learned a few new things about the |
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.
Nice refactoring :P
@billy1624 is there any chance this thing gets merged, or not? I obviously can't do this. |
I am on it. Thanks everyone! |
Thank you @tyt2y3 ! Will there be a release happening in the near future? |
Yes! Probably 0.3 at the end of this week. Stay tuned ) |
This introduces several things to optionally support automatic derive attributes for
serde::{Deserialize, Serialize}
for the generated models:sea-orm-codegen
which optionally includes the serde dependency (can be activated viacargo build / install --features with-serde
),WithSerde
enum to indicate if Serialize, Deserialize, or even both should be derived from,sea-orm-cli
which optionally activates thesea-orm-codegen
feature flagwith-serde
(can also be activated viacargo build / install --features with-serde
),--with-serde [none: default, serialize, deserialize, both]
Potential TODOs:
--with-serde
This PR is related to #236