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

Depending only on sea-orm, not sea-query, breaks with derive(Iden) #1473

Closed
nitnelave opened this issue Feb 10, 2023 · 5 comments · Fixed by #1740 or SeaQL/sea-query#655
Closed

Depending only on sea-orm, not sea-query, breaks with derive(Iden) #1473

nitnelave opened this issue Feb 10, 2023 · 5 comments · Fixed by #1740 or SeaQL/sea-query#655
Milestone

Comments

@nitnelave
Copy link

Description

In order to keep sea-orm and sea-query in sync and not have different versions of traits, one can remove sea-query from the direct dependencies (from Cargo.toml) and only rely on the re-export of sea_query in sea_orm.

However, that breaks the following code:

use sea_orm::Iden;
#[derive(Iden)]
pub enum Users {
  Table
}

Expected Behavior

Compiles.

Actual Behavior

use of undeclared crate or module `sea_query`

Versions

sea-orm 0.11
Implied sea-query.

Additional Information

I'm guessing the derive macro expands to sea_query::Iden. Maybe the sea_orm::Iden version should expand to sea_orm::sea_query::Iden?

A workaround is to also use sea_orm::sea_query::self;.

@tyt2y3
Copy link
Member

tyt2y3 commented Feb 11, 2023

Thanks for the report.
Right now, the Iden is just a re-export of the sea_query macro, that's why it isn't aware of sea_orm.
We can maintain a different impl in sea_orm, which should remove some unnecessary stuff from sea_query's version as well.
The custom, iden = , flatten etc can all be omitted.
I think the only relevant attribute to keep is the iden(rename = ).

@billy1624
Copy link
Member

I think it's a bad idea to have a Iden lite in sea-orm. What if an existing sea-query user want their Iden (with custom, iden = , flatten etc customization) to be used in sea-orm? They might be surprised to find the behaviour of the Iden changed after importing sea_orm::Iden.

So, maybe importing sea-query into the scope is a better "solution", i.e. use sea_orm::sea_query::{self};

@billy1624
Copy link
Member

Thanks for the report. Right now, the Iden is just a re-export of the sea_query macro, that's why it isn't aware of sea_orm. We can maintain a different impl in sea_orm, which should remove some unnecessary stuff from sea_query's version as well. The custom, iden = , flatten etc can all be omitted. I think the only relevant attribute to keep is the iden(rename = ).

We will do this

@billy1624
Copy link
Member

And, add a feature flag to sea-query-derive crate to conditionally alter the sea_query:: path as sea_orm::sea_query::

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 13, 2023

And, add a feature flag to sea-query-derive crate to conditionally alter the sea_query:: path as sea_orm::sea_query::

We discovered this won't work. However the final resolution has been to remove sea-query's Iden macro altogether, while providing a new DeriveIden macro.

https://github.com/SeaQL/sea-orm/blob/master/tests/derive_iden_tests.rs

@tyt2y3 tyt2y3 closed this as completed Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment