Skip to content

Conversation

Huliiiiii
Copy link
Member

@Huliiiiii Huliiiiii commented May 22, 2025

PR Info

  • Closes
  • Dependencies:
  • Dependents:

New Features

Bug Fixes

Breaking Changes

  • Add non_exhaustive to
    • expr::SimpleExpr
    • func::Function
    • types::{ColumnRef, TableRef, UnOper, BinOper, Keyword, SubQueryOper}
    • value::{Value, ValueTuple}
    • extension::mysql::column::MySqlType
    • extension::postgres::func::PgFunction
    • extension::postgres::PgBinOper
    • extension::postgres::select::SampleMethod
    • extension::postgres::types::TypeAs
    • extension::sqlite::SqliteBinOper

Changes

@Expurple
Copy link
Member

Hi, thank you. cargo test passes without warnings, everything seems OK.

Although, I was thinking about marking all necessary enums in one PR. I love small PRs, but a 1-line PR for each enum could be too small :D Especially given that, before merging, we also need to make a draft sea_orm PR that imports this branch and fixes the breakage (if there is any).

Can you include other enums in this PR? Here's my reasoning from #795, which enums to include:

Specifically, enums that correspond to SQL features / types / functions. It's impossible to exhaustively list these, because databases always keep gaining new features. Adding a new variant shouldn't be a breaking change. See #828 (comment) for an example of an SQL feature that can't be merged without a major release.

Also, can you make a PR for sea_orm?

- func::Function
- types::{ColumnRef, TableRef, UnOper, BinOper, Keyword, SubQueryOper}
- value::{Value, ValueTuple}
- extension::mysql::column::MySqlType
- extension::postgres::func::PgFunction
- extension::postgres::PgBinOper
- extension::postgres::select::SampleMethod
- extension::postgres::types::TypeAs
- extension::sqlite::SqliteBinOper
@Huliiiiii Huliiiiii changed the title Add non_exhaustive to SimpleExpr Add non_exhaustive to enums May 23, 2025
Copy link
Member

@Expurple Expurple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. cargo test passes without warnings. Your choice of enums looks good to me. I agree with all of these.

I took my time to search for pub enum and review all enums in the workspace. I would extend your list to also include Error and these SQL features: ArrayType, BinaryType, ColumnSpec, ForeignKeyAction, ForeignKeyStatement, Frame, FrameType, IndexColumn, IndexHintScope, IndexHintType, IndexOrder, IndexStatement, IndexType, JoinOn, JoinType, LockBehavior, LockType, OnConflictAction, OnConflictTarget, OnConflictUpdate, Order, PgDateTruncUnit, PgInterval, QueryStatement, ReturningClause, SchemaStatement, SearchOrder, SelectDistinct, StringLen, SubQueryStatement, TableAlterOption, TableDropOpt, TableOpt, TablePartition, TableStatement, TypeAlterAddOpt, TypeAlterOpt, TypeDropOpt, TypeRef, UnionType, WindowSelectType. Perhaps, Mode and Token too. What do you think?

After that, can you open a draft PR to check and fix the breakage in sea_orm?

Copy link
Member

@Expurple Expurple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for going through all of these.

Now, to proceed further, we need to discover all the missing arms in SeaQL/sea-orm#2607 and decide what to do with them.

@Expurple
Copy link
Member

The CI has detected missing arms in this repo too. Can you try to fix these arrors by adding an arm or moving the match into sea_query?

@tyt2y3
Copy link
Member

tyt2y3 commented May 29, 2025

moving the match into sea_query

umm, I don't understand this part

@Expurple
Copy link
Member

Expurple commented May 29, 2025

@tyt2y3 it's a reference to my earlier suggestions under the dependent sea_orm PR: SeaQL/sea-orm#2607 (comment)

When you match a non_exhaustive enum, you don't need the wildcard arm if the match is in the same crate as the enum.

@Huliiiiii
Copy link
Member Author

Well, I can't find a way to fix errors. Unless we move the implementation to sea-query.

@Huliiiiii
Copy link
Member Author

Or we can add a corresponding enum that is only used for internal crates.

@Expurple
Copy link
Member

Expurple commented Jun 3, 2025

Unless we move the implementation to sea-query.

We could merge these crates into sea_query altogether and turn them into feature-gated modules. That's just a quick idea, I don't know if it actually makes sense. I'll look into these errors in more detail later.

@tyt2y3 tyt2y3 changed the base branch from master to non_exhaustive June 8, 2025 19:47
@tyt2y3 tyt2y3 merged commit fc6b65d into SeaQL:non_exhaustive Jun 8, 2025
9 of 13 checks passed
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 8, 2025

I'll continue working on the branch

tyt2y3 added a commit that referenced this pull request Jun 8, 2025
Add `non_exhaustive` to enums
@Huliiiiii Huliiiiii deleted the expr-3 branch June 9, 2025 05:02
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.

3 participants