Unify Expr and SimpleExpr

[Huliiiiii]

PR Info

Unify Expr and SimpleExpr

Close: #771 (partially)

Also tracked/explained in #795.

New Features

Bug Fixes

Breaking Changes

• Remove Expr and rename SimpleExpr to Expr
• Add non_exhaustive to Expr
• Rename new_with_left to new
• Remove method expr because we can use new
• Remove inherent methods of Expr from ExprTrait

Changes

• Replace generic with impl syntax on some method to simplify code

[Huliiiiii]

We unify cust_with_values and cust_with_exprs because we implemented From<T> for Expr where T: Into<Value>

Just change the function declaration to

pub fn cust_with_exprs<T, I, E>(s: T, v: I) -> Self
where
  T: Into<String>,
  I: IntoIterator<Item = E>,
  E: Into<Expr>;

[Huliiiiii]

Maybe the deprecated methods should be removed as well?

[Expurple]

If you mean the deprecated methods in expr.rs (asterisk and table_asterisk), IMO these aren't harmful to the users or the health of the codebase. These methods make sense to me, behave well and only take like 10 lines of code. I'm not even sure why they're deprecated in the first place!

[Expurple]

Hi. First of all, thank you. It's great to see my idea come to life.

Before I properly review the diff, I want to address a few issues that I immediately see in the changelog, and also make the PR easier to review:

1. Don't rename SimpleExpr -> Expr in this PR. Let's keep this PR small and focused on the effects of type unification. Add a pub type Expr = SimpleExpr for compatibility. I planned to do that anyway, to avoid needlessly breaking the users. It's good that you want to clean up the codebase and rename the usages of SimpleExpr into Expr (so that only one name is used consistently). But that should be a separate PR afterwards, IMO.

2. Removal of inherent methods from the one remaining type should be a separate PR afterwards.

3. #[non_exhaustive] is unrelated to the type unification and should be a separate PR.

4. IMO, these breaking changes aren't important to the health of the codebase:

   • Rename new_with_left to new
   • Remove method expr because we can use new
   • Replace generic with impl syntax on some method to simplify code

   So, I would keep these features around to avoid needlessly breaking the users for little benefit.

   I agree that new_with_left and expr no longer make sense semantically. But I would mark them as deprecated instead.

Aside from being easier to review, small pull requests have another important benefit. I want to test each sea_query PR on sea_orm and my app, to see its effects and unexpected problems before merging it. This means that for every sea_query PR, we need to open a corresponding sea_orm PR that deals with the breaking changes. And that PR could be large in itself. It's better to localize the impact and make it easier to deal with the breakage in sea_orm.

If you want to contribute multiple changes, you don't have to sit idle until this first PR is merged. You can open multiple PRs that depend on each other, and we'll iterate on these in parallel. You can expect quick responses from me. I'm interested in landing these beautiful breaking changes!

[Expurple]

In the description, I would reword from

Close: #771

to

Close: #771 (partially)

Also tracked/explained in #795.

Your PR doesn't fully resolve #771, because that issue also needs another PR to remove methods from sea_orm::ColumnTrait and make ColumnTrait: Into<SimpleExpr> (implies ColumnTrait: ExprTrait, which will provide the replacement methods)

[Huliiiiii]

See #889 #890

[Expurple]

Thank you for splitting. Are you going to update this PR to do something not included in the others? Or do you want to close this one now? I think, we should close, because the "unification" from the name is now done in #889