Unify Expr and SimpleExpr as one type#889
Conversation
src/expr.rs
Outdated
| /// ); | ||
| /// ``` | ||
| #[allow(clippy::self_named_constructors)] | ||
| #[deprecated(since = "0.32.0", note = "Please use the [`SimpleExpr::new`] method")] |
There was a problem hiding this comment.
since needs to be updated
There was a problem hiding this comment.
Hmm, I'm not sure now to properly deal with this. But the first release with your changes will probably be called 1.0.0-rc1
There was a problem hiding this comment.
I've updated master to 1.0.0-rc1, so this can be changed now 8b7dffb
| pub fn new(expr: impl Into<Self>) -> Self { | ||
| expr.into() | ||
| } |
There was a problem hiding this comment.
new seems the same as from. Is it really needed?
There was a problem hiding this comment.
I think this aligns better with Rust conventions.
new, expr, and from essentially do the same thing.
There was a problem hiding this comment.
I think, new is kinda unnecessary (we already have variant constructors addition to expr and from). But we can have it. I'll leave this up to maintainers to decide
There was a problem hiding this comment.
Thank you. Everything seems good, cargo test passes without warnings.
But before merging, I think we need to check the breakage in sea_orm. Can you make a draft PR in sea_orm that imports this branch of sea_query and fixes the breakage?
Also, I'd reword the changelog a little:
New Features
SimpleExprnow has all methods fromExpr.
Breaking Changes
Expris now an alias ofSimpleExpr.
- How to migrate: if your code doesn't rely on these being two different types, everything should just work. But if you had
impl Foo for Exprandimpl Foo for SimpleExpr, your code won't compile and you need to delete one of the impls.
I assume, we'll need some "migration guide" for sea_query 1.0. So, I'm going to write a "How to migrate" section for every breaking change. You can also do this in your future PRs.
|
Waiting for Meanwhile, @tyt2y3, can you trigger the CI on this PR? After running the CI on both PRs, this PR can be merged (into an appropriate branch for breaking changes) |
|
@Huliiiiii after this PR CI should auto-trigger for you :) |
tyt2y3
changed the title
Expr, re-export Expr as type alias of SimpleExprExpr and SimpleExpr as one type
tyt2y3
changed the title
Expr and SimpleExpr as one typeExpr and SimpleExpr as one type
|
Apart from unrelated clippy complaints, the CI passes on both PRs 🎉 This PR can be merged now. After that, we can finish with merging SeaQL/sea-orm#2604 (comment) |
3 participants
PR Info
New Features
Bug Fixes
Breaking Changes
Changes
SimpleExpr::exprand addSimpleExpr::newwhich is more consistent with Rust conventions.