sea-query icon indicating copy to clipboard operation
sea-query copied to clipboard

Add `ExprTrait` (#771)

Open Expurple opened this issue 1 year ago • 4 comments

Edited PR Info

  • (partially) Closes #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 PgExpr for FunctionCall, ColumnRef, Keyword, LikeExpr, Value.
    • This could be a separate PR, but it's a small change that's very in line with ExprTrait: adding "operator" methods to all "smaller" types.
  • impl SqliteExpr for FunctionCall, ColumnRef, Keyword, LikeExpr, Value.
    • This could be a separate PR, but it's a small change that's very in line with ExprTrait: adding "operator" methods to all "smaller" types.
  • 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

None

Changes

None

Unedited original 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.

Expurple avatar Jul 07 '24 18:07 Expurple

Oh wow, thank you for coming back! This looks really neat!

I would say, there are two options going forward, and also depends on you choice:

  1. try not to break anything. this definitely will be a compromise, but at least some of the features will be available in SeaORM 1.0
  2. breaking everything as much as you like, and we'll ship it in the next major release. and probably backport some bits that's useful

tyt2y3 avatar Jul 18 '24 20:07 tyt2y3

Ok, if I'm too late for SeaORM 1.0, then I'll go with the backwards-compatible path now. I'll finish with this PR first, then I explore the opportunities to impl for ColumnTrait in SeaORM, and also prepare notes for later breaking cleanups for 2.0.

UPD: the notes are at #795.

To finish this PR, I'm going to:

  • Revert the breaking changes to impl PgExpr and impl SqliteExpr.
  • Copy-paste the documentation from the original inherent methods to the trait methods. This is good enough, right?

Expurple avatar Jul 18 '24 21:07 Expurple

I reverted the breaking impls and updated the PR description.

I'll add the documentation in the following days, and then the PR should be ready to merge.

Expurple avatar Jul 18 '24 22:07 Expurple

I've added documentation/doctests. The PR should be ready to merge now.

The docs are copy-pasted from the original inherent methods of Expr/SimpleExpr, but adjusted to actually call the trait method. I also linked to the trait methods from their inherent alternatives.

And I've noticed that the doctest for Expr::not_equals only calls equals. Someone probably copy-pasted it and forgot to edit. I fixed that separately in #796.

Expurple avatar Jul 22 '24 13:07 Expurple

:tada: Released In 0.32.0-rc.2 :tada:

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 ⭐.

github-actions[bot] avatar Oct 05 '24 10:10 github-actions[bot]

:tada: Released In 0.32.0 :tada:

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 ⭐.

github-actions[bot] avatar Oct 17 '24 07:10 github-actions[bot]