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 ExprTrait (#771) #791

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

Add ExprTrait (#771) #791

wants to merge 1 commit into from

Conversation

Expurple
Copy link

@Expurple Expurple commented Jul 7, 2024

PR Info

  • (partially) Closes Unify `Expr` and `SimpleExpr`, add `ExprTrait`? #771
  • Dependencies: none.
  • Dependents: currently none, but I have plans for the following things after the merge:
    • If you don't mind a small breaking change, I'd really like to get rid of some inherent methods on Expr and SimpleExpr, which now simply forward to ExprTrait methods. I call it small, because all signatures are exactly the same and the compiler will simply suggest adding use sea_query::ExprTrait, which is enough to fix the breakage.
    • Something like impl ExprTrait for C: ColumnTrait in sea_orm.
    • If you don't mind the breakage, I'd like to then remove some similar ColumnTrait methods. Their signatures don't match exactly, so there's going to be more breakage than in case of sea_query.

New Features

  • trait ExprTrait
    • It gathers together Expr- and SimpleExpr-like "operator" methods.
  • impl<T> ExprTrait for T where T: Into<SimpleExpr>
    • Implements these methods for Expr, SimpleExpr, and also all other Into<SimpleExpr> types, like Value or FunctionCall.
  • impl From<LikeExpr> for SimpleExpr
    • For me, this makes a lot more sense than a private fn like_like. But this part is optional and I don't mind reverting this if you want.

Bug Fixes

None

Breaking Changes

  • PgExpr and SqliteExpr are now implemented for broad T: ExprTrait, rather than only Expr and SimpleExpr.

Having this implementation makes a lot of sense, see impl ExprTrait above in "New Features". This is really elegant, but I think this is a breaking change. If you dislike this idea, I can revert this part easily.

Changes

None, except for the proposed breaking change above.

Notes

This is a "draft" PR with only implementation and almost no new documentation or tests. I'd like to hear some feedback before commiting to doing that.

I added a very minimal doctest on ExprTrait definition, as a showcase of expressions which are now possible to write without wrapping everything in layers of Expr::something(...). If it's not impressive or hard to understand, please let me know, I'll add more examples/comparisons. See also examples in #771 and #770. It's hard to understate: THIS IS A HUGE USABILITY WIN. I use SeaORM at work, I'm a relatively experienced user (and a contributor) at this point, but the Expr/SimpleExpr stuff still drives me crazy from time to time.

Apart from that doctest, there are no new tests, but note that ExprTrait methods are actually 100% covered by existing doctests of inherent Expr/SimpleExpr methods, which now call the trait under the hood.

This PR proposes a small breaking change, and I plan a few more, described in "PR Info". Let's also discuss this. Are you open to merging these? I'd really like to see them included. It makes things SO ELEGANT, compared to what we had. I think, now is a great time for this, as you go through unstable release candidates before major SeaORM 1.0.0.

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

1 participant