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

sea-orm-cli: allow skipping impl ActiveModelBehavior for generated entities #1947

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waynr
Copy link

@waynr waynr commented Oct 30, 2023

This PR adds an option that enables users to skip generating the

impl ActiveModelBehavior for ActiveModel {}

by passing the --gen-impl-active-model-behavior false flag to sea-orm-cli generate entity.

This is intended to allow for an easy and idiomatic-for-rust alternative to #1931 which I described in more detail in a related discussion post

I don't necessarily think this should close out #1931, but I do think that the complexity of the solution described in that issue merits some consideration given the available alternative.

@waynr
Copy link
Author

waynr commented Oct 30, 2023

The current state of this PR only adds the CLI flag that maintains the current default behavior and updates EntityWriter test cases to include the new option. If maintainers are open to this approach I'd be happy to add some new test cases to validate the false code path.

@dave42w
Copy link

dave42w commented Oct 30, 2023

I think this is an excellent solution.

@waynr
Copy link
Author

waynr commented Oct 30, 2023

Something I didn't realize when I opened this PR is that it may be a little problematic since it's all-or-nothing whereas it may be desirable for the empty impl blocks to exist for some of the generated entities since some ActiveModel trait methods have ActiveModelBehavior as a trait bound: https://docs.rs/sea-orm/latest/sea_orm/entity/trait.ActiveModelTrait.html

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 31, 2023

The implementation looks good, and this feature is definitely useful!

@jondot
Copy link
Contributor

jondot commented Nov 27, 2023

Hi this great, much needed, can we merge this?

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 27, 2023

@waynr do you have anything to amend? I can probably merge it as is

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.

None yet

4 participants