Skip to content
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

[Feature Request] CLI / Entity generator: Optionally derive serde::{Serialize/Deserialize} #236

Closed
Tracked by #229
elbart opened this issue Oct 10, 2021 · 3 comments · Fixed by #237
Closed
Tracked by #229

Comments

@elbart
Copy link
Contributor

elbart commented Oct 10, 2021

Dear SeaORM team,

another question / request from my side.

I just generated Sea ORM entities from my existing database structure using the sea-orm-cli tool. This worked like a charm. however it does not have an option to also add the the derive attributes for serde::{Serialize,Deserialize} traits. I know there is JSON support using into_json() however a serde_json::Value is nothing to handle nicely after the query executed for post query modifications. Now there is two questions:

  1. I saw in all your examples for actix and rocket, that the Serialize/Desrialize attributes are there (e.g. https://github.com/SeaQL/sea-orm/blob/master/examples/actix4_example/src/post.rs#L4). Are there cases where Serialize/Deserialize would not work on the generated Entity struct?
  2. Would it be ok to start a PR to propose such a change to the sea-orm-cli (add another clap option) and the sea-orm-codegen or was there a general decision to not go this way at all?

Best regards!

@elbart elbart changed the title [Feature Request] CLI / Entity generator: Optionally derive serde::{Serialize/Deserialize} [Feature Request] CLI / Entity generator: Optionally derive serde::{Serialize/Deserialize} Oct 10, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Oct 11, 2021

Hi Tim, thanks a lot.

This worked like a charm.

I am glad to hear that! (It still needs some polishing)

Yes. I just don't want to force every attribute to impl serde for now, as it will be some extra burden for custom types. It seems that right now it is perfectly doable (expect for the extra compile time).

So it's a good idea to add it as an optional argument, and we might turn it on by default at some point.

@baoyachi
Copy link
Contributor

Hi Tim, thanks a lot.

This worked like a charm.

I am glad to hear that! (It still needs some polishing)

Yes. I just don't want to force every attribute to impl serde for now, as it will be some extra burden for custom types. It seems that right now it is perfectly doable (expect for the extra compile time).

So it's a good idea to add it as an optional argument, and we might turn it on by default at some point.

I gree

@elbart
Copy link
Contributor Author

elbart commented Oct 11, 2021

Hi Tim, thanks a lot.

This worked like a charm.

I am glad to hear that! (It still needs some polishing)

Yes. I just don't want to force every attribute to impl serde for now, as it will be some extra burden for custom types. It seems that right now it is perfectly doable (expect for the extra compile time).

So it's a good idea to add it as an optional argument, and we might turn it on by default at some point.

Thanks for your feedback! I hope I prepared the PR in #237 as expected...

@tyt2y3 tyt2y3 linked a pull request Oct 14, 2021 that will close this issue
@elbart elbart closed this as completed Oct 14, 2021
@billy1624 billy1624 mentioned this issue Oct 15, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants