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

Derive value type #1720

Merged
merged 24 commits into from
Jul 10, 2023
Merged

Derive value type #1720

merged 24 commits into from
Jul 10, 2023

Conversation

darkmmon
Copy link
Contributor

@darkmmon darkmmon commented Jun 23, 2023

PR Info

  • Dependents:

New Features

  • added macro to derive for simple value type wrapper

Bug Fixes

Breaking Changes

Changes

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 27, 2023

Looks really good so far!

@darkmmon darkmmon marked this pull request as ready for review June 28, 2023 09:04
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.

Seems we have 3 more debug messages to "fix"

sea-orm-macros/src/derives/value_type.rs Outdated Show resolved Hide resolved
sea-orm-macros/src/derives/value_type.rs Outdated Show resolved Hide resolved
sea-orm-macros/src/derives/value_type.rs Outdated Show resolved Hide resolved
Comment on lines 20 to 22
#[derive(Clone, Debug, PartialEq, Eq, DeriveValueType)]
#[sea_orm(column_type = "String(Some(1))", array_type = "String")]
pub struct StringVec(pub Vec<String>);
Copy link
Member

Choose a reason for hiding this comment

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

Great work. I think it looks really good. One question, is array_type required here? What if we omit it?

Can we actually change the test case in tests/common/features/json_vec.rs to use this DeriveValueType instead of the manual implementation?

Just want to make sure we have had end to end tests.

pub struct Model {
#[sea_orm(primary_key)]
pub id: i32,
pub number: Integer,
Copy link
Member

@tyt2y3 tyt2y3 Jul 5, 2023

Choose a reason for hiding this comment

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

The strange bit here is that the StringVec is not included in the Model for testing.
Albeit it only works on Postgres.

May be we can simply change the test case https://github.com/SeaQL/sea-orm/blob/master/tests/common/features/json_vec.rs, because now DeriveValueType is the new, recommended API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the conversion of DeriveValueType rely on the pre-defined conversion from the field to Value.
I think Vec<String> currently does not have a defined conversion to any variant of Value, which I suppose it should.
(in json_vec, it convert itself to formatted String as Value, which I suppose isn't a good generic approach)
I'm wondering if I should implement From<Vec<T>> for Value or adjust the DeriveValueType implementation only.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Sadly then it means that we can't 'eat our own dog food' for this test case.

tests/json_vec_tests.rs Outdated Show resolved Hide resolved
@tyt2y3
Copy link
Member

tyt2y3 commented Jul 6, 2023

So perhaps it's easier to have a separate test case to test StringVec end to end.

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.

Perfect

# Poem with SeaORM example app
# Salvo with SeaORM example app
Copy link
Member

Choose a reason for hiding this comment

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

laugh :P
Good eyesight!

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!! @darkmmon awesome work!

@billy1624 billy1624 merged commit f5a7311 into SeaQL:master Jul 10, 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
None yet
Development

Successfully merging this pull request may close these issues.

Derive macros for custom wrapper types
4 participants