-
-
Notifications
You must be signed in to change notification settings - Fork 207
IF ELSE statements #828
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
base: master
Are you sure you want to change the base?
IF ELSE statements #828
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't reviewed the rest yet. Consider running cargo semver-checks
to find and document the breaking changes.
Disclaimer: I'm not a maintainer
@@ -35,6 +35,7 @@ pub enum SimpleExpr { | |||
AsEnum(DynIden, Box<SimpleExpr>), | |||
Case(Box<CaseStatement>), | |||
Constant(Value), | |||
IfElse(Box<IfElseStatement>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should mention that this is a breaking change (adding a new variant to a public enum not marked as #[non_exhaustive]
).
Perhaps, we should add #[non_exhaustive]
in the next major version. That's also a breaking change. But after that, changes like yours can me merged anytime with no problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the hint, I've updated the PR body accordingly.
Perhaps, we should add
#[non_exhaustive]
in the next major version. That's also a breaking change. But after that, changes like yours can me merged anytime with no problems
At some point the pool of statements will be exhausted. I agree though that for the time being, we can expect more variants to appear. Without even noticing that introducing a new variant is of course a breaking change, I've opened this as a draft to collect feedback.
For context, when combined with triggers #824 and custom errors #829 it becomes possible to implement fully custom constraints with logic. Maybe it is justified group such breaking changes into one.
Instead of adding #[non_exhaustive]
to the enum, I could also imagine creating a Milestone on GitHub with an exhaustive list of remaining missing variants across the supported backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding
#[non_exhaustive]
to the enum, I could also imagine creating a Milestone on GitHub with an exhaustive list of remaining missing variants across the supported backends.
This doesn't solve the problem with frequent breaking changes as each variant is implemented, unless you batch features into rare big major releases. But implementing every single database feature is unrealistic, especially in any short period of time. And these databases are going to gain new features in the future, anyway. Generally, I expect ORMs to always lag slightly behind in terms of database features. And I think, we need a way to continuously add new features without breaking changes. This is better than leaving unmerged PRs to rot until the next major release.
Since the list of database features is always a moving target, I don't see the point in keeping this enum exhaustive. But maybe a maintainer will clarify things. Maybe there's some practical benefit to exhaustiveness here. E.g. some external crate (like sea_orm
) could rely on supporting sea_query
exhaustively and doing something specific for each variant. And if SimpleExpr
becomes #[non_exhaustive]
, that other crate would have to either change its signatures to be fallible, or lock the sea_query
version to guarantee exhaustive coverage (and always remember to manually maintain it when bumping sea_query
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #[non_exhaustive]
to my list of wanted breaking changes (#795)
PR Info
New Features
Breaking Changes
SimpleExpr::IfElse
variant to public enum