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

Introduce optional serde support for model code generation #237

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

elbart
Copy link
Contributor

@elbart elbart commented Oct 11, 2021

This introduces several things to optionally support automatic derive attributes for serde::{Deserialize, Serialize} for the generated models:

  • introduces a feature flag for sea-orm-codegen which optionally includes the serde dependency (can be activated via cargo build / install --features with-serde),
  • introduces a WithSerde enum to indicate if Serialize, Deserialize, or even both should be derived from,
  • introduces a feature flag for sea-orm-cli which optionally activates the sea-orm-codegen feature flag with-serde (can also be activated via cargo build / install --features with-serde),
  • adds an optional cli argument --with-serde [none: default, serialize, deserialize, both]
  • adds test harness for both compact and expanded generation

Potential TODOs:

  • Add conditional compilation for the cli argument --with-serde
  • Make it more beautiful, but I wanted to wait for the first signal if this is the right direction to go for...

This PR is related to #236

@baoyachi
Copy link
Contributor

Wow.

@elbart
Copy link
Contributor Author

elbart commented Oct 11, 2021

I guess we should change the documentation, too. Could somebody point me to where I could propose such changes?
I also just spotted that the sea-orm-codegen test are not executed automatically in the github actions.

Edit: Sorry, the GH actions indeed run the codegen tests (cargo test --all) includes this. Won't need to add anything.

@billy1624
Copy link
Member

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

@elbart
Copy link
Contributor Author

elbart commented Oct 11, 2021

@baoyachi @billy1624 do you guys have any advice on how to add additional compilation for this --with-serde CLI arg in sea-orm-cli? Never did that before (c31fb3c#diff-a07d755f2f721c1b06b1c33b45a507b6c55cc99746e69b036ac7b08788aaac98R68)

@billy1624
Copy link
Member

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.

@billy1624
Copy link
Member

@baoyachi @billy1624 do you guys have any advice on how to add additional compilation for this --with-serde CLI arg in sea-orm-cli? Never did that before (c31fb3c#diff-a07d755f2f721c1b06b1c33b45a507b6c55cc99746e69b036ac7b08788aaac98R68)

Then this won't be so complicated :P

@elbart
Copy link
Contributor Author

elbart commented Oct 11, 2021

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 WithSerde enum in some kind of Configuration struct within the codegen project. It's less complicated, but also less "neat" :)

@elbart
Copy link
Contributor Author

elbart commented Oct 11, 2021

I re-iterated on that and you are absolutely right:

  • sea-orm-codegen and even sea-orm-cli do not need any serde dependencies since we only generate text ond not really depend on serde during compile or runtime. I will remove the cargo feature stuff
  • also regarding the tests i can reduce the amount of files! I dont have to retest each entity, one per enum variant should be enough... sorry for that - will do the optimizations tomorrow!

@billy1624
Copy link
Member

It's perfectly fine! Again, thank you so much for the contributions!!

@billy1624
Copy link
Member

sea-orm-codegen and even sea-orm-cli do not need any serde dependencies since we only generate text ond not really depend on serde during compile or runtime. I will remove the cargo feature stuff

Yes, I guess it's the case. Just now I want to double check how you use serde dependency loll

also regarding the tests i can reduce the amount of files! I dont have to retest each entity, one per enum variant should be enough... sorry for that - will do the optimizations tomorrow!

Sure, this should be enough. Thanks!!

@elbart
Copy link
Contributor Author

elbart commented Oct 11, 2021

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 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

@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
Copy link
Member

@billy1624 billy1624 left a 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!

sea-orm-cli/Cargo.toml Outdated Show resolved Hide resolved
sea-orm-codegen/src/entity/writer.rs Outdated Show resolved Hide resolved
sea-orm-codegen/src/entity/writer.rs Outdated Show resolved Hide resolved
sea-orm-codegen/src/entity/writer.rs Outdated Show resolved Hide resolved
sea-orm-codegen/src/entity/writer.rs Show resolved Hide resolved
@billy1624
Copy link
Member

OMG... sorry for such a long code review

@elbart
Copy link
Contributor Author

elbart commented Oct 12, 2021

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 quote!() macro. Thank you!

@billy1624 billy1624 self-requested a review October 12, 2021 14:38
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring :P

@elbart
Copy link
Contributor Author

elbart commented Oct 14, 2021

@billy1624 is there any chance this thing gets merged, or not? I obviously can't do this.

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2021

I am on it. Thanks everyone!

@tyt2y3 tyt2y3 merged commit fad881b into SeaQL:master Oct 14, 2021
@elbart
Copy link
Contributor Author

elbart commented Oct 14, 2021

Thank you @tyt2y3 ! Will there be a release happening in the near future?

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2021

Yes! Probably 0.3 at the end of this week. Stay tuned )

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 this pull request may close these issues.

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