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

Add default_expr support for ColumnDef #1474

Merged
merged 17 commits into from
Jul 13, 2023

Conversation

Diwakar-Gupta
Copy link
Contributor

@Diwakar-Gupta Diwakar-Gupta commented Feb 12, 2023

PR Info

Fixes
#1398 and #826

File Changed

  1. sea-orm-macros/src/derives/entity_model.rs parse attribute and generate function call.
  2. src/entity/column.rs updated ColumnDef to store SimpleExpr instead of Value and updated default_value function.

Tests

Have removed PartialEq from ColumnDef because SimpleExpr does not implement PartialEq, thats why test using assert_eq on ColumnDef are failing.

To make a temporarily fix we can implementing eq method for ColumnDef for test only and for supported features only.

@Diwakar-Gupta
Copy link
Contributor Author

I had certain questions:

  1. Should I add a test for default_expr and may be default_value?
  2. Should I comment test for now.

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 @Diwakar-Gupta, thanks for contributing!!

  1. Should I add a test for default_expr and may be default_value?

Yes, please!

  1. Should I comment test for now.

I think it's possible to implement PartialEq for sea_query::SimpleExpr but it's not trivial. I can try though.

@Diwakar-Gupta
Copy link
Contributor Author

Thanks I will do that, additionally if you are going to impl PartialEq I had done some research for that it will be helpfull.

**
I went through all the fields in SimpleExpr and the problem is caused by pub type DynIden = SeaRc<dyn Iden>; in sea-query/src/types.rs. This Iden is created by macro_rules! iden_trait https://github.com/SeaQL/sea-query/blob/94a1ab7290075e8a8bbb7a10e5207350def3455c/src/types.rs#L18.
If we can make it implement PartialEq it will solve the problem.
But SeaRc<dyn Iden> is actually Rc from standard library which does not implement PartialEq and we cannot implement that in other crate than rust itself. I am stuck here.
**

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 8, 2023

I think it's possible to implement PartialEq for sea_query::SimpleExpr but it's not trivial. I can try though.

We discussed a while before. The gist is DynIden is a foreign type we can't simply impl PartialEq for. May be we should have our own wrapper type in SeaQuery.

But I imagine it could be implemented as follows:

(*self).get_type_id() == (*other).get_type_id() && self.to_string() == other.to_string()

Checking type id is necessary, because two types with the same name is not deemed to be equal.

@billy1624
Copy link
Member

Hey @Diwakar-Gupta @tyt2y3, I just did a POC, SeaQL/sea-query#620.

@ikrivosheev
Copy link
Member

Hello! Why do we need PartialEq for ColumnDef? Does something using this?

@billy1624
Copy link
Member

billy1624 commented Mar 23, 2023

Hello! Why do we need PartialEq for ColumnDef? Does something using this?

Here. It's used during tests.
https://github.com/SeaQL/sea-orm/actions/runs/4362547441/jobs/7627571099#step:4:364

@billy1624
Copy link
Member

Hey @Diwakar-Gupta, I just upgraded SeaQuery dependency to SeaQL/sea-query#620

@billy1624
Copy link
Member

Hey @Diwakar-Gupta, don't worry about the error at https://github.com/SeaQL/sea-orm/actions/runs/4500149841/jobs/7918900727?pr=1474. The root cause is that SeaORM and SeaSchema depend on two different version of SeaQuery. We can fix it after SeaQL/sea-query#620 has been merged.

@lazulit3
Copy link

Hey @Diwakar-Gupta, don't worry about the error at https://github.com/SeaQL/sea-orm/actions/runs/4500149841/jobs/7918900727?pr=1474. The root cause is that SeaORM and SeaSchema depend on two different version of SeaQuery. We can fix it after SeaQL/sea-query#620 has been merged.

I see that this PR was merged, is there still integration / release / deps work remaining? Am just curious what to keep an eye on regarding blockers / next-steps. :D

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.

Thanks again for the PR!! @Diwakar-Gupta

@billy1624 billy1624 requested a review from tyt2y3 June 20, 2023 03:41
@billy1624 billy1624 linked an issue Jun 20, 2023 that may be closed by this pull request
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you all

@tyt2y3 tyt2y3 merged commit 1ba37b6 into SeaQL:master Jul 13, 2023
72 checks passed
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

🎉 Released In 0.12.1 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
5 participants